Adding `cause` to the cookie extension API's onchanged event signature.

This makes it simpler for developers to deal with the release/set event pair generated by setting a cookie that already exists, and gives them more information about general cookie removal (e.g. that the cookie wasn't "removed" actively but expired).

BUG=70101
TEST=net_unittests

Review URL: http://codereview.chromium.org/6698023
Patch from Mike West <mkwst@chromium.org>.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79113 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 91 insertions, 15 deletions

diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index 928019e..2a1c30c 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -348,6 +348,38 @@ bool FindLeastRecentlyAccessed(
   return false;
 }
 
+// Mapping between DeletionCause and Delegate::ChangeCause; the mapping also
+// provides a boolean that specifies whether or not an OnCookieChanged
+// notification ought to be generated.
+typedef struct ChangeCausePair_struct {
+  CookieMonster::Delegate::ChangeCause cause;
+  bool notify;
+} ChangeCausePair;
+ChangeCausePair ChangeCauseMapping[] = {
+  // DELETE_COOKIE_EXPLICIT
+  { CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT, true },
+  // DELETE_COOKIE_OVERWRITE
+  { CookieMonster::Delegate::CHANGE_COOKIE_OVERWRITE, true },
+  // DELETE_COOKIE_EXPIRED
+  { CookieMonster::Delegate::CHANGE_COOKIE_EXPIRED, true },
+  // DELETE_COOKIE_EVICTED
+  { CookieMonster::Delegate::CHANGE_COOKIE_EVICTED, true },
+  // DELETE_COOKIE_DUPLICATE_IN_BACKING_STORE
+  { CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT, false },
+  // DELETE_COOKIE_DONT_RECORD
+  { CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT, false },
+  // DELETE_COOKIE_EVICTED_DOMAIN
+  { CookieMonster::Delegate::CHANGE_COOKIE_EVICTED, true },
+  // DELETE_COOKIE_EVICTED_GLOBAL
+  { CookieMonster::Delegate::CHANGE_COOKIE_EVICTED, true },
+  // DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE
+  { CookieMonster::Delegate::CHANGE_COOKIE_EVICTED, true },
+  // DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE
+  { CookieMonster::Delegate::CHANGE_COOKIE_EVICTED, true },
+  // DELETE_COOKIE_LAST_ENTRY
+  { CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT, false }
+};
+
 }  // namespace
 
 // static
@@ -1244,8 +1276,10 @@ void CookieMonster::InternalInsertCookie(const std::string& key,
   if (cc->IsPersistent() && store_ && sync_to_store)
     store_->AddCookie(*cc);
   cookies_.insert(CookieMap::value_type(key, cc));
-  if (delegate_.get())
-    delegate_->OnCookieChanged(*cc, false);
+  if (delegate_.get()) {
+    delegate_->OnCookieChanged(
+        *cc, false, CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT);
+  }
 }
 
 bool CookieMonster::SetCookieWithCreationTimeAndOptions(
@@ -1360,6 +1394,12 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
                                          DeletionCause deletion_cause) {
   lock_.AssertAcquired();
 
+  // Ideally, this would be asserted up where we define ChangeCauseMapping,
+  // but DeletionCause's visibility (or lack thereof) forces us to make
+  // this check here.
+  COMPILE_ASSERT(arraysize(ChangeCauseMapping) == DELETE_COOKIE_LAST_ENTRY + 1,
+                 ChangeCauseMapping_size_not_eq_DeletionCause_enum_size);
+
   // See InitializeHistograms() for details.
   if (deletion_cause != DELETE_COOKIE_DONT_RECORD)
     histogram_cookie_deletion_cause_->Add(deletion_cause);
@@ -1369,8 +1409,12 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
 
   if (cc->IsPersistent() && store_ && sync_to_store)
     store_->DeleteCookie(*cc);
-  if (delegate_.get())
-    delegate_->OnCookieChanged(*cc, true);
+  if (delegate_.get()) {
+    ChangeCausePair mapping = ChangeCauseMapping[deletion_cause];
+
+    if (mapping.notify)
+      delegate_->OnCookieChanged(*cc, true, mapping.cause);
+  }
   cookies_.erase(it);
   delete cc;
 
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index b6649715..026af8c 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -266,8 +266,15 @@ class CookieMonster : public CookieStore {
   FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, GetKey);
   FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, TestGetKey);
 
+  // Internal reasons for deletion, used to populate informative histograms
+  // and to provide a public cause for onCookieChange notifications.
+  //
+  // If you add or remove causes from this list, please be sure to also update
+  // the Delegate::ChangeCause mapping inside InternalDeleteCookie. Moreover,
+  // these are used as array indexes, so please avoid reordering without good
+  // reason.
   enum DeletionCause {
-    DELETE_COOKIE_EXPLICIT,
+    DELETE_COOKIE_EXPLICIT = 0,
     DELETE_COOKIE_OVERWRITE,
     DELETE_COOKIE_EXPIRED,
     DELETE_COOKIE_EVICTED,
@@ -281,7 +288,7 @@ class CookieMonster : public CookieStore {
     DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE,
 
     // Cookies evicted during domain level garbage collection that
-    // were accessed more rencelyt than kSafeFromGlobalPurgeDays
+    // were accessed more recently than kSafeFromGlobalPurgeDays
     // (and thus would have been preserved by global garbage collection).
     DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE,
 
@@ -411,7 +418,8 @@ class CookieMonster : public CookieStore {
   void InternalUpdateCookieAccessTime(CanonicalCookie* cc,
                                       const base::Time& current_time);
 
-  // |deletion_cause| argument is for collecting statistics.
+  // |deletion_cause| argument is used for collecting statistics and choosing
+  // the correct Delegate::ChangeCause for OnCookieChanged notifications.
   void InternalDeleteCookie(CookieMap::iterator it, bool sync_to_store,
                             DeletionCause deletion_cause);
 
@@ -634,11 +642,33 @@ class CookieMonster::CanonicalCookie {
 class CookieMonster::Delegate
     : public base::RefCountedThreadSafe<CookieMonster::Delegate> {
  public:
+  // The publicly relevant reasons a cookie might be changed.
+  enum ChangeCause {
+    // The cookie was changed directly by a consumer's action.
+    CHANGE_COOKIE_EXPLICIT,
+    // 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,
+  };
+
   // Will be called when a cookie is added or removed. The function is passed
   // the respective |cookie| which was added to or removed from the cookies.
-  // If |removed| is true, the cookie was deleted.
+  // If |removed| is true, the cookie was deleted, and |cause| will be set
+  // to the reason for it's removal. If |removed| is false, the cookie was
+  // added, and |cause| will be set to CHANGE_COOKIE_EXPLICIT.
+  //
+  // As a special case, note that updating a cookie's properties is implemented
+  // as a two step process: the cookie to be updated is first removed entirely,
+  // generating a notification with cause CHANGE_COOKIE_OVERWRITE.  Afterwards,
+  // a new cookie is written with the updated values, generating a notification
+  // with cause CHANGE_COOKIE_EXPLICIT.
   virtual void OnCookieChanged(const CookieMonster::CanonicalCookie& cookie,
-                               bool removed) = 0;
+                               bool removed,
+                               ChangeCause cause) = 0;
  protected:
   friend class base::RefCountedThreadSafe<CookieMonster::Delegate>;
   virtual ~Delegate() {}
diff --git a/net/base/cookie_monster_store_test.cc b/net/base/cookie_monster_store_test.cc
index c7f9e61..30125eb 100644
--- a/net/base/cookie_monster_store_test.cc
+++ b/net/base/cookie_monster_store_test.cc
@@ -64,11 +64,12 @@ MockPersistentCookieStore::SetClearLocalStateOnExit(bool clear_local_state) {
 MockCookieMonsterDelegate::MockCookieMonsterDelegate() {}
 
 void MockCookieMonsterDelegate::OnCookieChanged(
-      const net::CookieMonster::CanonicalCookie& cookie,
-      bool removed) {
-    CookieNotification notification(cookie, removed);
-    changes_.push_back(notification);
-  }
+    const net::CookieMonster::CanonicalCookie& cookie,
+    bool removed,
+    net::CookieMonster::Delegate::ChangeCause cause) {
+  CookieNotification notification(cookie, removed);
+  changes_.push_back(notification);
+}
 
 MockCookieMonsterDelegate::~MockCookieMonsterDelegate() {}
 
diff --git a/net/base/cookie_monster_store_test.h b/net/base/cookie_monster_store_test.h
index 951c055..8f39c63 100644
--- a/net/base/cookie_monster_store_test.h
+++ b/net/base/cookie_monster_store_test.h
@@ -91,7 +91,8 @@ class MockCookieMonsterDelegate : public net::CookieMonster::Delegate {
 
   virtual void OnCookieChanged(
       const net::CookieMonster::CanonicalCookie& cookie,
-      bool removed);
+      bool removed,
+      net::CookieMonster::Delegate::ChangeCause cause);
 
  private:
   virtual ~MockCookieMonsterDelegate();