REVIEW: let browser context manage cookie change sub list

atom/browser/api/atom_api_cookies.cc

@@ -238,12 +238,11 @@ void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
 }  // namespace
 
 Cookies::Cookies(v8::Isolate* isolate, AtomBrowserContext* browser_context)
-    : request_context_getter_(browser_context->url_request_context_getter()) {
+    : browser_context_(browser_context),
+      request_context_getter_(browser_context->url_request_context_getter()) {
   Init(isolate);
-  cookie_change_subscription_ =
+  cookie_change_subscription_ = browser_context->RegisterCookieChangeCallback(
-      browser_context->url_request_context_getter()
+      base::Bind(&Cookies::OnCookieChanged, base::Unretained(this)));
-          ->RegisterCookieChangeCallback(
-              base::Bind(&Cookies::OnCookieChanged, base::Unretained(this)));
 }
 
 Cookies::~Cookies() {}
@@ -281,7 +280,7 @@ void Cookies::FlushStore(const base::Closure& callback) {
       base::Bind(FlushCookieStoreOnIOThread, getter, callback));
 }
 
-void Cookies::OnCookieChanged(const brightray::CookieDetails* details) {
+void Cookies::OnCookieChanged(const CookieDetails* details) {
   Emit("changed", *(details->cookie), details->cause, details->removed);
 }
 

atom/browser/api/atom_api_cookies.h

@@ -8,8 +8,8 @@
 #include <string>
 
 #include "atom/browser/api/trackable_object.h"
+#include "atom/browser/net/cookie_details.h"
 #include "base/callback.h"
-#include "brightray/browser/net/cookie_details.h"
 #include "native_mate/handle.h"
 #include "net/cookies/canonical_cookie.h"
 
@@ -54,15 +54,17 @@ class Cookies : public mate::TrackableObject<Cookies> {
   void Set(const base::DictionaryValue& details, const SetCallback& callback);
   void FlushStore(const base::Closure& callback);
 
-  // brightray::URLRequestContextGetter subscription:
+  // AtomBrowserContext::RegisterCookieChangeCallback subscription:
-  void OnCookieChanged(const brightray::CookieDetails*);
+  void OnCookieChanged(const CookieDetails*);
 
  private:
-  net::URLRequestContextGetter* request_context_getter_;
+  // Store a reference to ensure this class gets destroyed before the context.
-  std::unique_ptr<
+  scoped_refptr<AtomBrowserContext> browser_context_;
-      base::CallbackList<void(const brightray::CookieDetails*)>::Subscription>
+  std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
       cookie_change_subscription_;
 
+  net::URLRequestContextGetter* request_context_getter_;
+
   DISALLOW_COPY_AND_ASSIGN(Cookies);
 };
 

atom/browser/atom_browser_context.cc

@@ -15,6 +15,7 @@
 #include "atom/browser/net/atom_cert_verifier.h"
 #include "atom/browser/net/atom_network_delegate.h"
 #include "atom/browser/net/atom_url_request_job_factory.h"
+#include "atom/browser/net/cookie_details.h"
 #include "atom/browser/net/http_protocol_handler.h"
 #include "atom/browser/web_view_manager.h"
 #include "atom/common/atom_version.h"
@@ -102,6 +103,12 @@ void AtomBrowserContext::SetUserAgent(const std::string& user_agent) {
   user_agent_ = user_agent;
 }
 
+std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
+AtomBrowserContext::RegisterCookieChangeCallback(
+    const base::Callback<void(const CookieDetails*)>& cb) {
+  return cookie_change_sub_list_.Add(cb);
+}
+
 std::unique_ptr<net::NetworkDelegate>
 AtomBrowserContext::CreateNetworkDelegate() {
   return base::MakeUnique<AtomNetworkDelegate>();
@@ -198,6 +205,14 @@ std::vector<std::string> AtomBrowserContext::GetCookieableSchemes() {
   return default_schemes;
 }
 
+void AtomBrowserContext::NotifyCookieChange(
+    const net::CanonicalCookie& cookie,
+    bool removed,
+    net::CookieStore::ChangeCause cause) {
+  CookieDetails cookie_details(&cookie, removed, cause);
+  cookie_change_sub_list_.Notify(&cookie_details);
+}
+
 void AtomBrowserContext::RegisterPrefs(PrefRegistrySimple* pref_registry) {
   pref_registry->RegisterFilePathPref(prefs::kSelectFileLastDirectory,
                                       base::FilePath());

atom/browser/atom_browser_context.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_list.h"
 #include "brightray/browser/browser_context.h"
 
 namespace atom {
@@ -17,6 +18,7 @@ class AtomDownloadManagerDelegate;
 class AtomNetworkDelegate;
 class AtomPermissionManager;
 class WebViewManager;
+struct CookieDetails;
 
 class AtomBrowserContext : public brightray::BrowserContext {
  public:
@@ -28,6 +30,10 @@ class AtomBrowserContext : public brightray::BrowserContext {
       const base::DictionaryValue& options = base::DictionaryValue());
 
   void SetUserAgent(const std::string& user_agent);
+  // Register callbacks that needs to notified on any cookie store changes.
+  std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
+  RegisterCookieChangeCallback(
+      const base::Callback<void(const CookieDetails*)>& cb);
 
   // brightray::URLRequestContextGetter::Delegate:
   std::unique_ptr<net::NetworkDelegate> CreateNetworkDelegate() override;
@@ -39,6 +45,9 @@ class AtomBrowserContext : public brightray::BrowserContext {
   std::unique_ptr<net::CertVerifier> CreateCertVerifier(
       brightray::RequireCTDelegate* ct_delegate) override;
   std::vector<std::string> GetCookieableSchemes() override;
+  void NotifyCookieChange(const net::CanonicalCookie& cookie,
+                          bool removed,
+                          net::CookieStore::ChangeCause cause) override;
 
   // content::BrowserContext:
   content::DownloadManagerDelegate* GetDownloadManagerDelegate() override;
@@ -63,6 +72,8 @@ class AtomBrowserContext : public brightray::BrowserContext {
   std::string user_agent_;
   bool use_cache_;
 
+  base::CallbackList<void(const CookieDetails*)> cookie_change_sub_list_;
+
   DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext);
 };
 

atom/browser/net/cookie_details.h

@@ -2,13 +2,13 @@
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
-#ifndef BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_
+#ifndef ATOM_BROWSER_NET_COOKIE_DETAILS_H_
-#define BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_
+#define ATOM_BROWSER_NET_COOKIE_DETAILS_H_
 
 #include "base/macros.h"
 #include "net/cookies/cookie_store.h"
 
-namespace brightray {
+namespace atom {
 
 struct CookieDetails {
  public:
@@ -22,6 +22,6 @@ struct CookieDetails {
   net::CookieStore::ChangeCause cause;
 };
 
-}  // namespace brightray
+}  // namespace atom
 
-#endif  // BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_
+#endif  // ATOM_BROWSER_NET_COOKIE_DETAILS_H_

brightray/browser/url_request_context_getter.cc

@@ -13,7 +13,6 @@
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/worker_pool.h"
 #include "brightray/browser/browser_client.h"
-#include "brightray/browser/net/cookie_details.h"
 #include "brightray/browser/net/devtools_network_controller_handle.h"
 #include "brightray/browser/net/devtools_network_transaction_factory.h"
 #include "brightray/browser/net/require_ct_delegate.h"
@@ -155,30 +154,19 @@ URLRequestContextGetter::URLRequestContextGetter(
 URLRequestContextGetter::~URLRequestContextGetter() {
 }
 
-std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
-URLRequestContextGetter::RegisterCookieChangeCallback(
-    const base::Callback<void(const CookieDetails*)>& cb) {
-  return cookie_change_sub_list_.Add(cb);
-}
-
-void URLRequestContextGetter::NotifyCookieChange(
-    const net::CanonicalCookie& cookie,
-    bool removed,
-    net::CookieStore::ChangeCause cause) {
-  CookieDetails cookie_details(&cookie, removed, cause);
-  cookie_change_sub_list_.Notify(&cookie_details);
-}
-
 void URLRequestContextGetter::OnCookieChanged(
     const net::CanonicalCookie& cookie,
     net::CookieStore::ChangeCause cause) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
+  if (!delegate_)
+    return;
+
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&URLRequestContextGetter::NotifyCookieChange, this, cookie,
+      base::BindOnce(
-                     !(cause == net::CookieStore::ChangeCause::INSERTED),
+          &Delegate::NotifyCookieChange, base::Unretained(delegate_), cookie,
-                     cause));
+          !(cause == net::CookieStore::ChangeCause::INSERTED), cause));
 }
 
 net::HostResolver* URLRequestContextGetter::host_resolver() {

brightray/browser/url_request_context_getter.h

@@ -8,7 +8,6 @@
 #include <string>
 #include <vector>
 
-#include "base/callback_list.h"
 #include "base/files/file_path.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
@@ -38,7 +37,6 @@ namespace brightray {
 class RequireCTDelegate;
 class DevToolsNetworkControllerHandle;
 class NetLog;
-struct CookieDetails;
 
 class URLRequestContextGetter : public net::URLRequestContextGetter {
  public:
@@ -59,6 +57,9 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
         RequireCTDelegate* ct_delegate);
     virtual net::SSLConfigService* CreateSSLConfigService();
     virtual std::vector<std::string> GetCookieableSchemes();
+    virtual void NotifyCookieChange(const net::CanonicalCookie& cookie,
+                                    bool removed,
+                                    net::CookieStore::ChangeCause cause) {}
   };
 
   URLRequestContextGetter(
@@ -72,13 +73,6 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
       content::URLRequestInterceptorScopedVector protocol_interceptors);
   virtual ~URLRequestContextGetter();
 
-  // Register callbacks that needs to notified on any cookie store changes.
-  std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
-  RegisterCookieChangeCallback(
-      const base::Callback<void(const CookieDetails*)>& cb);
-  void NotifyCookieChange(const net::CanonicalCookie& cookie,
-                          bool removed,
-                          net::CookieStore::ChangeCause cause);
   // net::CookieStore::CookieChangedCallback implementation.
   void OnCookieChanged(const net::CanonicalCookie& cookie,
                        net::CookieStore::ChangeCause cause);
@@ -119,7 +113,6 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
   content::ProtocolHandlerMap protocol_handlers_;
   content::URLRequestInterceptorScopedVector protocol_interceptors_;
 
-  base::CallbackList<void(const CookieDetails*)> cookie_change_sub_list_;
   net::URLRequestJobFactory* job_factory_;  // weak ref
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextGetter);