Network Service: CookieManager observer API refactoring.

This CL aligns CookieManager's interface for listening to cookie changes
(observer pattern) with the proposed addition to
RestrictedCookieManager, which will be used by the Async Cookies API.

Bug: 729800
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I0464b0d1d0388279e549d19c5dbeea2d0342cc60
Reviewed-on: https://chromium-review.googlesource.com/913151Reviewed-by: Randy Smith <rdsmith@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536944}

chrome/browser/extensions/extension_cookie_notifier.cc

@@ -18,7 +18,7 @@
 #include "net/cookies/canonical_cookie.h"
 
 ExtensionCookieNotifier::ExtensionCookieNotifier(Profile* profile)
-    : profile_(profile) {
+    : profile_(profile), binding_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile);
 
@@ -28,14 +28,12 @@ ExtensionCookieNotifier::ExtensionCookieNotifier(Profile* profile)
       ->GetNetworkContext()
       ->GetCookieManager(mojo::MakeRequest(&manager_ptr));
 
-  network::mojom::CookieChangeNotificationPtr notification_ptr;
-  binding_ =
-      std::make_unique<mojo::Binding<network::mojom::CookieChangeNotification>>(
-          this, mojo::MakeRequest(&notification_ptr));
-  manager_ptr->RequestGlobalNotifications(std::move(notification_ptr));
+  network::mojom::CookieChangeListenerPtr listener_ptr;
+  binding_.Bind(mojo::MakeRequest(&listener_ptr));
+  manager_ptr->AddGlobalChangeListener(std::move(listener_ptr));
 }
 
-void ExtensionCookieNotifier::OnCookieChanged(
+void ExtensionCookieNotifier::OnCookieChange(
     const net::CanonicalCookie& cookie,
     network::mojom::CookieChangeCause cause) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

chrome/browser/extensions/extension_cookie_notifier.h

@@ -21,20 +21,18 @@ class CanonicalCookie;
 // Sends cookie-change notifications on the UI thread via
 // chrome::NOTIFICATION_COOKIE_CHANGED_FOR_EXTENSIONS for all cookie
 // changes associated with the given profile.
-class ExtensionCookieNotifier
-    : public network::mojom::CookieChangeNotification {
+class ExtensionCookieNotifier : public network::mojom::CookieChangeListener {
  public:
   explicit ExtensionCookieNotifier(Profile* profile);
   ~ExtensionCookieNotifier() override;
 
  private:
-  // network::mojom::CookieChangeNotification implementation.
-  void OnCookieChanged(const net::CanonicalCookie& cookie,
-                       network::mojom::CookieChangeCause cause) override;
+  // network::mojom::CookieChangeListener implementation.
+  void OnCookieChange(const net::CanonicalCookie& cookie,
+                      network::mojom::CookieChangeCause cause) override;
 
   Profile* profile_;
-  std::unique_ptr<mojo::Binding<network::mojom::CookieChangeNotification>>
-      binding_;
+  mojo::Binding<network::mojom::CookieChangeListener> binding_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionCookieNotifier);
 };

services/network/cookie_manager.cc

@@ -144,9 +144,15 @@ network::mojom::CookieChangeCause ChangeCauseTranslation(
 
 }  // namespace
 
-CookieManager::NotificationRegistration::NotificationRegistration() {}
+CookieManager::ListenerRegistration::ListenerRegistration() {}
 
-CookieManager::NotificationRegistration::~NotificationRegistration() {}
+CookieManager::ListenerRegistration::~ListenerRegistration() {}
+
+void CookieManager::ListenerRegistration::DispatchCookieStoreChange(
+    const net::CanonicalCookie& cookie,
+    net::CookieStore::ChangeCause cause) {
+  listener->OnCookieChange(cookie, ChangeCauseTranslation(cause));
+}
 
 CookieManager::CookieManager(net::CookieStore* cookie_store)
     : cookie_store_(cookie_store) {}
@@ -197,98 +203,74 @@ void CookieManager::DeleteCookies(
       std::move(callback));
 }
 
-void CookieManager::RequestNotification(
+void CookieManager::AddCookieChangeListener(
     const GURL& url,
     const std::string& name,
-    network::mojom::CookieChangeNotificationPtr notification_pointer) {
-  std::unique_ptr<NotificationRegistration> notification_registration(
-      std::make_unique<NotificationRegistration>());
-  notification_registration->notification_pointer =
-      std::move(notification_pointer);
+    network::mojom::CookieChangeListenerPtr listener) {
+  auto listener_registration = std::make_unique<ListenerRegistration>();
+  listener_registration->listener = std::move(listener);
 
-  notification_registration->subscription = cookie_store_->AddCallbackForCookie(
+  listener_registration->subscription = cookie_store_->AddCallbackForCookie(
       url, name,
       base::BindRepeating(
-          &CookieManager::CookieChanged,
-          // base::Unretained is safe as destruction of the
-          // CookieManager will also destroy the
-          // notifications_registered list (which this object will be
-          // inserted into, below), which will destroy the
-          // CookieChangedSubscription, unregistering the callback.
-          base::Unretained(this),
+          &CookieManager::ListenerRegistration::DispatchCookieStoreChange,
           // base::Unretained is safe as destruction of the
-          // NotificationRegistration will also destroy the
+          // ListenerRegistration will also destroy the
           // CookieChangedSubscription, unregistering the callback.
-          base::Unretained(notification_registration.get())));
+          base::Unretained(listener_registration.get())));
 
-  notification_registration->notification_pointer.set_connection_error_handler(
-      base::BindOnce(&CookieManager::NotificationPipeBroken,
+  listener_registration->listener.set_connection_error_handler(
+      base::BindOnce(&CookieManager::RemoveChangeListener,
                      // base::Unretained is safe as destruction of the
                      // CookieManager will also destroy the
                      // notifications_registered list (which this object will be
                      // inserted into, below), which will destroy the
-                     // notification_pointer, rendering this callback moot.
+                     // listener, rendering this callback moot.
                      base::Unretained(this),
                      // base::Unretained is safe as destruction of the
-                     // NotificationRegistration will also destroy the
+                     // ListenerRegistration will also destroy the
                      // CookieChangedSubscription, unregistering the callback.
-                     base::Unretained(notification_registration.get())));
+                     base::Unretained(listener_registration.get())));
 
-  notifications_registered_.push_back(std::move(notification_registration));
+  listener_registrations_.push_back(std::move(listener_registration));
 }
 
-void CookieManager::RequestGlobalNotifications(
-    network::mojom::CookieChangeNotificationPtr notification_pointer) {
-  std::unique_ptr<NotificationRegistration> notification_registration(
-      std::make_unique<NotificationRegistration>());
-  notification_registration->notification_pointer =
-      std::move(notification_pointer);
+void CookieManager::AddGlobalChangeListener(
+    network::mojom::CookieChangeListenerPtr listener) {
+  auto listener_registration = std::make_unique<ListenerRegistration>();
+  listener_registration->listener = std::move(listener);
 
-  notification_registration->subscription =
+  listener_registration->subscription =
       cookie_store_->AddCallbackForAllChanges(base::BindRepeating(
-          &CookieManager::CookieChanged,
+          &CookieManager::ListenerRegistration::DispatchCookieStoreChange,
           // base::Unretained is safe as destruction of the
-          // CookieManager will also destroy the
-          // notifications_registered list (which this object will be
-          // inserted into, below), which will destroy the
+          // ListenerRegistration will also destroy the
           // CookieChangedSubscription, unregistering the callback.
-          base::Unretained(this),
-          // base::Unretained is safe as destruction of the
-          // NotificationRegistration will also destroy the
-          // CookieChangedSubscription, unregistering the callback.
-          base::Unretained(notification_registration.get())));
+          base::Unretained(listener_registration.get())));
 
-  notification_registration->notification_pointer.set_connection_error_handler(
-      base::BindOnce(&CookieManager::NotificationPipeBroken,
+  listener_registration->listener.set_connection_error_handler(
+      base::BindOnce(&CookieManager::RemoveChangeListener,
                      // base::Unretained is safe as destruction of the
                      // CookieManager will also destroy the
                      // notifications_registered list (which this object will be
                      // inserted into, below), which will destroy the
-                     // notification_pointer, rendering this callback moot.
+                     // listener, rendering this callback moot.
                      base::Unretained(this),
                      // base::Unretained is safe as destruction of the
-                     // NotificationRegistration will also destroy the
+                     // ListenerRegistration will also destroy the
                      // CookieChangedSubscription, unregistering the callback.
-                     base::Unretained(notification_registration.get())));
-
-  notifications_registered_.push_back(std::move(notification_registration));
-}
+                     base::Unretained(listener_registration.get())));
 
-void CookieManager::CookieChanged(NotificationRegistration* registration,
-                                  const net::CanonicalCookie& cookie,
-                                  net::CookieStore::ChangeCause cause) {
-  registration->notification_pointer->OnCookieChanged(
-      cookie, ChangeCauseTranslation(cause));
+  listener_registrations_.push_back(std::move(listener_registration));
 }
 
-void CookieManager::NotificationPipeBroken(
-    NotificationRegistration* registration) {
-  for (auto it = notifications_registered_.begin();
-       it != notifications_registered_.end(); ++it) {
+void CookieManager::RemoveChangeListener(ListenerRegistration* registration) {
+  for (auto it = listener_registrations_.begin();
+       it != listener_registrations_.end(); ++it) {
     if (it->get() == registration) {
       // It isn't expected this will be a common enough operation for
       // the performance of std::vector::erase() to matter.
-      notifications_registered_.erase(it);
+      listener_registrations_.erase(it);
       return;
     }
   }

services/network/cookie_manager.h

@@ -51,46 +51,45 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CookieManager
                           SetCanonicalCookieCallback callback) override;
   void DeleteCookies(network::mojom::CookieDeletionFilterPtr filter,
                      DeleteCookiesCallback callback) override;
-  void RequestNotification(const GURL& url,
-                           const std::string& name,
-                           network::mojom::CookieChangeNotificationPtr
-                               notification_pointer) override;
-  void RequestGlobalNotifications(network::mojom::CookieChangeNotificationPtr
-                                      notification_pointer) override;
+  void AddCookieChangeListener(
+      const GURL& url,
+      const std::string& name,
+      network::mojom::CookieChangeListenerPtr listener) override;
+  void AddGlobalChangeListener(
+      network::mojom::CookieChangeListenerPtr listener) override;
   void CloneInterface(
       network::mojom::CookieManagerRequest new_interface) override;
 
-  uint32_t GetClientsBoundForTesting() const { return bindings_.size(); }
-  uint32_t GetNotificationsBoundForTesting() const {
-    return notifications_registered_.size();
+  size_t GetClientsBoundForTesting() const { return bindings_.size(); }
+  size_t GetListenersRegisteredForTesting() const {
+    return listener_registrations_.size();
   }
 
  private:
-  struct NotificationRegistration {
-    NotificationRegistration();
-    ~NotificationRegistration();
+  // State associated with a CookieChangeListener.
+  struct ListenerRegistration {
+    ListenerRegistration();
+    ~ListenerRegistration();
+
+    // Translates a CookieStore change callback to a CookieChangeListener call.
+    void DispatchCookieStoreChange(const net::CanonicalCookie& cookie,
+                                   net::CookieStore::ChangeCause cause);
 
     // Owns the callback registration in the store.
     std::unique_ptr<net::CookieStore::CookieChangedSubscription> subscription;
 
-    // Pointer on which to send notifications.
-    network::mojom::CookieChangeNotificationPtr notification_pointer;
+    // The observer receiving change notifications.
+    network::mojom::CookieChangeListenerPtr listener;
 
-    DISALLOW_COPY_AND_ASSIGN(NotificationRegistration);
+    DISALLOW_COPY_AND_ASSIGN(ListenerRegistration);
   };
 
-  // Used to hook callbacks
-  void CookieChanged(NotificationRegistration* registration,
-                     const net::CanonicalCookie& cookie,
-                     net::CookieStore::ChangeCause cause);
-
-  // Handles connection errors on notification pipes.
-  void NotificationPipeBroken(NotificationRegistration* registration);
+  // Handles connection errors on change listener pipes.
+  void RemoveChangeListener(ListenerRegistration* registration);
 
   net::CookieStore* const cookie_store_;
   mojo::BindingSet<network::mojom::CookieManager> bindings_;
-  std::vector<std::unique_ptr<NotificationRegistration>>
-      notifications_registered_;
+  std::vector<std::unique_ptr<ListenerRegistration>> listener_registrations_;
 
   DISALLOW_COPY_AND_ASSIGN(CookieManager);
 };

services/network/public/mojom/cookie_manager.mojom

@@ -137,9 +137,9 @@ struct CookieDeletionFilter {
   CookieDeletionSessionControl session_control = IGNORE_CONTROL;
 };
 
-interface CookieChangeNotification {
+interface CookieChangeListener {
   // TODO(rdsmith): Should this be made a batch interface?
-  OnCookieChanged(CanonicalCookie cookie, CookieChangeCause cause);
+  OnCookieChange(CanonicalCookie cookie, CookieChangeCause cause);
 };
 
 interface CookieManager {
@@ -173,31 +173,31 @@ interface CookieManager {
   // Returns the number of cookies deleted.
   DeleteCookies(CookieDeletionFilter filter) => (uint32 num_deleted);
 
-  // Send a CookieChangeNotification over which notification
-  // for cookie changes will be sent.  When the specified cookie
-  // associated with the domain/path specified in the URL changes, a
-  // notification will be posted to the passed pointer.
+  // Subscribes the given listener to changes to a cookie.
+  //
+  // The subscription is canceled by closing the CookieChangeListener's pipe.
   //
   // Note that if the caller may be racing with other uses of the cookie store,
-  // it should follow the notification request with a probe of the relevant
+  // it should follow the subscription request with a probe of the relevant
   // information about the tracked cookie, to make sure that a change to the
-  // cookie did not happen while the notification was being installed.
+  // cookie did not happen right before the listener was registered.
   //
   // TODO(rdsmith): Should this have a filter to register for a lot of
   // notifications at once?  Maybe combine with the deletion filter?
-  RequestNotification(
+  // TODO(rdsmith): Describe the performance implications of using this meethod.
+  // The comments in CookieMonster::AddCallbackForCookie look pretty scary.
+  AddCookieChangeListener(
       url.mojom.Url url,
       string name,
-      CookieChangeNotification notification_pointer);
+      CookieChangeListener listener);
 
-  // Send a CookieChangeNotification over which notification
-  // for all cookie changes will be sent.  When a cookie associated with
-  // the CookieManager changes, a notification will be posted to the
-  // passed pointer.
+  // Subscribes the given listener to changes to this CookieManager's cookies.
+  //
+  // The subscription is canceled by closing the CookieChangeListener's pipe.
   //
   // TODO(rdsmith): Should this have a filter to register for a lot of
   // notifications at once?  Maybe combine with the deletion filter?
-  RequestGlobalNotifications(CookieChangeNotification notification_pointer);
+  AddGlobalChangeListener(CookieChangeListener notification_pointer);
 
   // Clone the interface for use somewhere else.  After this call,
   // requests to the same implementation may be posted to the other side