Pass a RemovalCause to CookieChangedCallback

net/cookies/cookie_store.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Brought to you by number 42.
 
 #ifndef NET_COOKIES_COOKIE_STORE_H_
 #define NET_COOKIES_COOKIE_STORE_H_
 
 #include <memory>
@@ skipping 15 matching lines @@
 
 // An interface for storing and retrieving cookies. Implementations are not
 // thread safe, as with most other net classes. All methods must be invoked on
 // the network thread, and all callbacks will be calle there.
 //
 // All async functions may either invoke the callback asynchronously, or they
 // may be invoked immediately (prior to return of the asynchronous function).
 // Destroying the CookieStore will cancel pending async callbacks.
 class NET_EXPORT CookieStore {
  public:
+  // The publicly relevant reasons a cookie might be changed.
+  enum ChangeCause {
+    // The cookie was inserted.
+    CHANGE_COOKIE_INSERTED,

[mmenke, 2016/09/26 18:29:26]

Can we make this an enum class (And maybe get rid of the prefix)?

i.e. CookieStore::ChangeCause::INSERTED

[nharper, 2016/09/26 21:09:10]

Done.

+    // The cookie was changed directly by a consumer's action.
+    CHANGE_COOKIE_EXPLICIT,
+    // The cookie was deleted, but no more details are known.
+    CHANGE_COOKIE_UNKNOWN_DELETION,
+    // The cookie was automatically removed due to an insert operation that
+    // overwrote it.
+    CHANGE_COOKIE_OVERWRITE,
+    // The cookie was automatically removed as it expired.
+    CHANGE_COOKIE_EXPIRED,
+    // The cookie was automatically evicted during garbage collection.
+    CHANGE_COOKIE_EVICTED,
+    // The cookie was overwritten with an already-expired expiration date.
+    CHANGE_COOKIE_EXPIRED_OVERWRITE
+  };
+
+  static bool ChangeCauseIsDeletion(ChangeCause cause);
+
   // Callback definitions.
   typedef base::Callback<void(const CookieList& cookies)> GetCookieListCallback;
   typedef base::Callback<void(const std::string& cookie)> GetCookiesCallback;
   typedef base::Callback<void(bool success)> SetCookiesCallback;
   typedef base::Callback<void(int num_deleted)> DeleteCallback;
-  typedef base::Callback<void(const CanonicalCookie& cookie, bool removed)>
+  typedef base::Callback<void(const CanonicalCookie& cookie, ChangeCause cause)>
       CookieChangedCallback;
-  typedef base::CallbackList<void(const CanonicalCookie& cookie, bool removed)>
+  typedef base::CallbackList<void(const CanonicalCookie& cookie,
+                                  ChangeCause cause)>

[Mike West, 2016/09/26 07:55:37]

Can you add some tests for this change? It looks like the changes to the
existing unit tests fold all deletion types together to replicate the old
interface. It would be great to verify that you're getting the correct values
out based upon what's passed in.

[nharper, 2016/09/26 21:09:10]

I've updated cookie_monster_unittest.cc to check the types. I could also make
the same change to cookie_store_ios_unittest.mm if you'd like.

       CookieChangedCallbackList;
   typedef CookieChangedCallbackList::Subscription CookieChangedSubscription;
   typedef base::Callback<bool(const CanonicalCookie& cookie)> CookiePredicate;
 
   virtual ~CookieStore();
 
   // Returns the cookie line (e.g. "cookie1=value1; cookie2=value2") represented
   // by |cookies|. The string is built in the same order as the given list.
   //
   // TODO(mkwst): We really should standardize on either
@@ skipping 136 matching lines @@
   // passed the respective |cookie| which was added to or removed from the
   // cookies and a boolean indicating if the cookies was removed or not.
   //
   // Note that |callback| is called twice when a cookie is updated: once for
   // the removal of the existing cookie and once for the adding the new cookie.
   //
   // Note that this method consumes memory and CPU per (url, name) pair ever
   // registered that are still consumed even after all subscriptions for that
   // (url, name) pair are removed. If this method ever needs to support an
   // unbounded amount of such pairs, this contract needs to change and
   // implementors need to be improved to not behave this way.

[mmenke, 2016/09/26 18:29:26]

Maybe add a warning that the callback must not synchronously modify another
cookie, while we're here?

[nharper, 2016/09/26 21:09:10]

Done.

   virtual std::unique_ptr<CookieChangedSubscription> AddCallbackForCookie(
       const GURL& url,
       const std::string& name,
       const CookieChangedCallback& callback) = 0;
 
   // Returns true if this cookie store is ephemeral, and false if it is backed
   // by some sort of persistence layer.
   // TODO(nharper): Remove this method once crbug.com/548423 has been closed.
   virtual bool IsEphemeral() = 0;
   void SetChannelIDServiceID(int id);
   int GetChannelIDServiceID();
 
  protected:
   CookieStore();
   int channel_id_service_id_;
 };
 
 }  // namespace net
 
 #endif  // NET_COOKIES_COOKIE_STORE_H_