REVIEW: Subscribe to cookie store for changes in place of CookieMonsterDelegate

atom/browser/api/atom_api_cookies.cc

@@ -20,7 +20,6 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
-using atom::AtomCookieDelegate;
 using content::BrowserThread;
 
 namespace mate {
@@ -238,17 +237,16 @@ 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()),
-        cookie_delegate_(browser_context->cookie_delegate()) {
+Cookies::Cookies(v8::Isolate* isolate, AtomBrowserContext* browser_context)
+    : request_context_getter_(browser_context->url_request_context_getter()) {
   Init(isolate);
-  cookie_delegate_->AddObserver(this);
+  cookie_change_subscription_ =
+      browser_context->url_request_context_getter()
+          ->RegisterCookieChangeCallback(
+              base::Bind(&Cookies::OnCookieChanged, base::Unretained(this)));
 }
 
-Cookies::~Cookies() {
-  cookie_delegate_->RemoveObserver(this);
-}
+Cookies::~Cookies() {}
 
 void Cookies::Get(const base::DictionaryValue& filter,
                   const GetCallback& callback) {
@@ -283,10 +281,8 @@ void Cookies::FlushStore(const base::Closure& callback) {
       base::Bind(FlushCookieStoreOnIOThread, getter, callback));
 }
 
-void Cookies::OnCookieChanged(const net::CanonicalCookie& cookie,
-                              bool removed,
-                              net::CookieStore::ChangeCause cause) {
-  Emit("changed", cookie, cause, removed);
+void Cookies::OnCookieChanged(const brightray::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/atom_cookie_delegate.h"
 #include "base/callback.h"
+#include "brightray/browser/net/cookie_details.h"
 #include "native_mate/handle.h"
 #include "net/cookies/canonical_cookie.h"
 
@@ -27,8 +27,7 @@ class AtomBrowserContext;
 
 namespace api {
 
-class Cookies : public mate::TrackableObject<Cookies>,
-                public AtomCookieDelegate::Observer {
+class Cookies : public mate::TrackableObject<Cookies> {
  public:
   enum Error {
     SUCCESS,
@@ -55,14 +54,14 @@ class Cookies : public mate::TrackableObject<Cookies>,
   void Set(const base::DictionaryValue& details, const SetCallback& callback);
   void FlushStore(const base::Closure& callback);
 
-  // AtomCookieDelegate::Observer:
-  void OnCookieChanged(const net::CanonicalCookie& cookie,
-                       bool removed,
-                       net::CookieStore::ChangeCause cause) override;
+  // brightray::URLRequestContextGetter subscription:
+  void OnCookieChanged(const brightray::CookieDetails*);
 
  private:
   net::URLRequestContextGetter* request_context_getter_;
-  scoped_refptr<AtomCookieDelegate> cookie_delegate_;
+  std::unique_ptr<
+      base::CallbackList<void(const brightray::CookieDetails*)>::Subscription>
+      cookie_change_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(Cookies);
 };

atom/browser/atom_browser_context.cc

@@ -70,8 +70,7 @@ std::string RemoveWhitespace(const std::string& str) {
 AtomBrowserContext::AtomBrowserContext(const std::string& partition,
                                        bool in_memory,
                                        const base::DictionaryValue& options)
-    : brightray::BrowserContext(partition, in_memory),
-      cookie_delegate_(new AtomCookieDelegate) {
+    : brightray::BrowserContext(partition, in_memory) {
   // Construct user agent string.
   Browser* browser = Browser::Get();
   std::string name = RemoveWhitespace(browser->GetName());
@@ -108,10 +107,6 @@ AtomBrowserContext::CreateNetworkDelegate() {
   return base::MakeUnique<AtomNetworkDelegate>();
 }
 
-net::CookieMonsterDelegate* AtomBrowserContext::CreateCookieDelegate() {
-  return cookie_delegate();
-}
-
 std::string AtomBrowserContext::GetUserAgent() {
   return user_agent_;
 }

atom/browser/atom_browser_context.h

@@ -8,9 +8,7 @@
 #include <string>
 #include <vector>
 
-#include "atom/browser/net/atom_cookie_delegate.h"
 #include "brightray/browser/browser_context.h"
-#include "net/cookies/cookie_monster.h"
 
 namespace atom {
 
@@ -33,7 +31,6 @@ class AtomBrowserContext : public brightray::BrowserContext {
 
   // brightray::URLRequestContextGetter::Delegate:
   std::unique_ptr<net::NetworkDelegate> CreateNetworkDelegate() override;
-  net::CookieMonsterDelegate* CreateCookieDelegate() override;
   std::string GetUserAgent() override;
   std::unique_ptr<net::URLRequestJobFactory> CreateURLRequestJobFactory(
       content::ProtocolHandlerMap* protocol_handlers) override;
@@ -52,9 +49,6 @@ class AtomBrowserContext : public brightray::BrowserContext {
   void RegisterPrefs(PrefRegistrySimple* pref_registry) override;
 
   AtomBlobReader* GetBlobReader();
-  AtomCookieDelegate* cookie_delegate() const {
-    return cookie_delegate_.get();
-  }
 
  protected:
   AtomBrowserContext(const std::string& partition, bool in_memory,
@@ -69,8 +63,6 @@ class AtomBrowserContext : public brightray::BrowserContext {
   std::string user_agent_;
   bool use_cache_;
 
-  scoped_refptr<AtomCookieDelegate> cookie_delegate_;
-
   DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext);
 };
 

atom/browser/net/atom_cookie_delegate.cc

@@ -1,44 +0,0 @@
-// Copyright (c) 2016 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "atom/browser/net/atom_cookie_delegate.h"
-
-#include "content/public/browser/browser_thread.h"
-
-namespace atom {
-
-AtomCookieDelegate::AtomCookieDelegate() {
-}
-
-AtomCookieDelegate::~AtomCookieDelegate() {
-}
-
-void AtomCookieDelegate::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void AtomCookieDelegate::RemoveObserver(Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
-void AtomCookieDelegate::NotifyObservers(
-    const net::CanonicalCookie& cookie,
-    bool removed,
-    net::CookieStore::ChangeCause cause) {
-  for (Observer& observer : observers_)
-    observer.OnCookieChanged(cookie, removed, cause);
-}
-
-void AtomCookieDelegate::OnCookieChanged(
-    const net::CanonicalCookie& cookie,
-    bool removed,
-    net::CookieStore::ChangeCause cause) {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&AtomCookieDelegate::NotifyObservers,
-                 this, cookie, removed, cause));
-}
-
-}  // namespace atom

atom/browser/net/atom_cookie_delegate.h

@@ -1,48 +0,0 @@
-// Copyright (c) 2016 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#ifndef ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_
-#define ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_
-
-#include "base/observer_list.h"
-#include "net/cookies/cookie_monster.h"
-
-namespace atom {
-
-class AtomCookieDelegate : public net::CookieMonsterDelegate {
- public:
-  AtomCookieDelegate();
-  ~AtomCookieDelegate() override;
-
-  class Observer {
-   public:
-    virtual void OnCookieChanged(const net::CanonicalCookie& cookie,
-                                 bool removed,
-                                 net::CookieStore::ChangeCause cause) {}
-   protected:
-    virtual ~Observer() {}
-  };
-
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
-
-  // net::CookieMonsterDelegate:
-  void OnCookieChanged(const net::CanonicalCookie& cookie,
-                       bool removed,
-                       net::CookieStore::ChangeCause cause) override;
-
-
- private:
-  base::ObserverList<Observer> observers_;
-
-  void NotifyObservers(const net::CanonicalCookie& cookie,
-                       bool removed,
-                       net::CookieStore::ChangeCause cause);
-
-  DISALLOW_COPY_AND_ASSIGN(AtomCookieDelegate);
-};
-
-}   // namespace atom
-
-#endif  // ATOM_BROWSER_NET_ATOM_COOKIE_DELEGATE_H_

brightray/browser/net/cookie_details.h

@@ -0,0 +1,27 @@
+// Copyright (c) 2017 GitHub, Inc.
+// 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_
+#define BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_
+
+#include "base/macros.h"
+#include "net/cookies/cookie_store.h"
+
+namespace brightray {
+
+struct CookieDetails {
+ public:
+  CookieDetails(const net::CanonicalCookie* cookie_copy,
+                bool is_removed,
+                net::CookieStore::ChangeCause cause)
+      : cookie(cookie_copy), removed(is_removed), cause(cause) {}
+
+  const net::CanonicalCookie* cookie;
+  bool removed;
+  net::CookieStore::ChangeCause cause;
+};
+
+}  // namespace brightray
+
+#endif  // BRIGHTRAY_BROWSER_NET_COOKIE_DETAILS_H_

brightray/browser/url_request_context_getter.cc

@@ -13,6 +13,7 @@
 #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"
@@ -29,7 +30,6 @@
 #include "net/cert/ct_log_verifier.h"
 #include "net/cert/ct_policy_enforcer.h"
 #include "net/cert/multi_log_ct_verifier.h"
-#include "net/cookies/cookie_monster.h"
 #include "net/dns/mapped_host_resolver.h"
 #include "net/http/http_auth_filter.h"
 #include "net/http/http_auth_handler_factory.h"
@@ -155,6 +155,32 @@ 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);
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&URLRequestContextGetter::NotifyCookieChange, this, cookie,
+                     !(cause == net::CookieStore::ChangeCause::INSERTED),
+                     cause));
+}
+
 net::HostResolver* URLRequestContextGetter::host_resolver() {
   return url_request_context_->host_resolver();
 }
@@ -188,6 +214,12 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
     std::unique_ptr<net::CookieStore> cookie_store =
         content::CreateCookieStore(cookie_config);
     storage_->set_cookie_store(std::move(cookie_store));
+    // Cookie store will outlive notifier by order of declaration
+    // in the header.
+    cookie_change_sub_ =
+        url_request_context_->cookie_store()->AddCallbackForAllChanges(
+            base::Bind(&URLRequestContextGetter::OnCookieChanged, this));
+
     storage_->set_channel_id_service(base::MakeUnique<net::ChannelIDService>(
         new net::DefaultChannelIDStore(nullptr)));
 

brightray/browser/url_request_context_getter.h

@@ -8,10 +8,10 @@
 #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"
-#include "net/cookies/cookie_monster.h"
 #include "net/cookies/cookie_store.h"
 #include "net/http/http_cache.h"
 #include "net/http/transport_security_state.h"
@@ -38,6 +38,7 @@ namespace brightray {
 class RequireCTDelegate;
 class DevToolsNetworkControllerHandle;
 class NetLog;
+struct CookieDetails;
 
 class URLRequestContextGetter : public net::URLRequestContextGetter {
  public:
@@ -71,6 +72,17 @@ 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);
+
   // net::URLRequestContextGetter:
   net::URLRequestContext* GetURLRequestContext() override;
   scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
@@ -102,9 +114,12 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
   std::unique_ptr<net::HostMappingRules> host_mapping_rules_;
   std::unique_ptr<net::HttpAuthPreferences> http_auth_preferences_;
   std::unique_ptr<net::HttpNetworkSession> http_network_session_;
+  std::unique_ptr<net::CookieStore::CookieChangedSubscription>
+      cookie_change_sub_;
   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);

brightray/filenames.gypi

@@ -47,6 +47,7 @@
       'browser/media/media_device_id_salt.h',
       'browser/media/media_stream_devices_controller.cc',
       'browser/media/media_stream_devices_controller.h',
+      'browser/net/cookie_details.h',
       'browser/net/devtools_network_conditions.cc',
       'browser/net/devtools_network_conditions.h',
       'browser/net/devtools_network_controller.cc',

filenames.gypi

@@ -261,8 +261,6 @@
       'atom/browser/net/asar/url_request_asar_job.h',
       'atom/browser/net/atom_cert_verifier.cc',
       'atom/browser/net/atom_cert_verifier.h',
-      'atom/browser/net/atom_cookie_delegate.cc',
-      'atom/browser/net/atom_cookie_delegate.h',
       'atom/browser/net/atom_network_delegate.cc',
       'atom/browser/net/atom_network_delegate.h',
       'atom/browser/net/atom_url_request.cc',