Split initial load of cookies by domains

net/base/cookie_monster_unittest.cc

Index: net/base/cookie_monster_unittest.cc
===================================================================
--- net/base/cookie_monster_unittest.cc	(revision 103757)
+++ net/base/cookie_monster_unittest.cc	(working copy)
@@ -37,7 +37,9 @@
 class NewMockPersistentCookieStore
     : public CookieMonster::PersistentCookieStore {
  public:
-  MOCK_METHOD1(Load, bool(const LoadedCallback& loaded_callback));
+  MOCK_METHOD1(Load, void(const LoadedCallback& loaded_callback));
+  MOCK_METHOD2(LoadCookiesForKey, void(const std::string& key,
+                                       const LoadedCallback& loaded_callback));
   MOCK_METHOD1(AddCookie, void(const CookieMonster::CanonicalCookie& cc));
   MOCK_METHOD1(UpdateCookieAccessTime,
                void(const CookieMonster::CanonicalCookie& cc));
@@ -1013,7 +1015,8 @@
 // This test suite verifies the task deferral behaviour of the CookieMonster.

[erikwright (departed), 2011/10/04 19:55:02]

To avoid having a vector of raw pointers (that you really ought to be deleting
in your destructor if they are not already deleted, i.e., in the case of a test
failure) I suggest the following:

ACTION_P(PushCallback, callback_vector) {
  callback_vector->push(arg0);
}

...

  void CompleteLoadingAndWait() {
    while (!loaded_for_key_callbacks_.empty()) {
      loaded_for_key_callbacks_.front().Run(loaded_cookies_);
      loaded_cookies_.clear();
      loaded_for_key_callbacks_.pop();
    }

    loaded_callback_.Run(loaded_cookies_);
    RunFor(kTimeout);
  }

...

  void ExpectLoadForKeyCall() {
    EXPECT_CALL(*persistent_store_, LoadCookiesForKey(testing::_, testing::_)).
        WillOnce(testing::DoAll(
                 PushCallback(&loaded_for_key_callbacks_),
                 QuitCurrentMessageLoop()));
  }

...

 private:
  std::queue<CookieMonster::PersistentCookieStore::LoadedCallback>
      loaded_for_key_callbacks_;

...

[guohui, 2011/10/06 15:40:00]

Done.

 // Specifically, for each asynchronous method, verify that:
 // 1. invoking it on an uninitialized cookie store causes the store to begin
-//    loading its backing data.
+//    chainlloading its backing data or loading data for a specific domain key

[erikwright (departed), 2011/10/04 19:55:02]

chainlloading => chain-loading

[guohui, 2011/10/06 15:40:00]

Done.

+//    (eTLD+1).
 // 2. The initial invocation does not complete until the loading completes.
 // 3. Invocations after the loading has completed complete immediately.
 class DeferredCookieTaskTest : public CookieMonsterTest {
@@ -1039,31 +1042,58 @@
     testing::Mock::VerifyAndClear(persistent_store_.get());
   }
 
-  // Invokes the PersistentCookieStore::Load completion callback and waits
+  // Invokes the PersistentCookieStore::LoadCookiesForKey completion callbacks
+  // and PersistentCookieStore::Load completion callback and waits
   // until the message loop is quit.
   void CompleteLoadingAndWait() {
+    std::vector<CookieMonster::PersistentCookieStore::LoadedCallback *>
+      ::iterator it = loaded_for_key_callbacks_.begin();
+    for (; it != loaded_for_key_callbacks_.end(); it++) {
+      (*it)->Run(loaded_cookies_);
+      loaded_cookies_.clear();
+      delete *it;
+    }

[erikwright (departed), 2011/10/04 19:55:02]

Clear the loaded_for_key_callbacks_ after line 1055.

[guohui, 2011/10/06 15:40:00]

Done.

     loaded_callback_.Run(loaded_cookies_);
     RunFor(kTimeout);
   }
 
   // Performs the provided action, expecting it to cause a call to
   // PersistentCookieStore::Load. Call WaitForLoadCall to verify the load call
-  // is received.
+  // is receievd.

[erikwright (departed), 2011/10/04 19:55:02]

receivd => received 

[guohui, 2011/10/06 15:40:00]

Done.

   void BeginWith(testing::Action<void(void)> action) {
     EXPECT_CALL(*this, Begin()).WillOnce(action);
     ExpectLoadCall();
     Begin();
   }
 
+  void BeginWithForDomainKey(testing::Action<void(void)> action) {

[erikwright (departed), 2011/10/04 19:55:02]

Accept a parameter for the domain key, pass it into ExpectLoadForKeyCall, and
then include it in the expectation for LoadCookiesForKey (instead of the first
testing::_ placeholder).

[erikwright (departed), 2011/10/04 19:55:02]

This looks unbalanced to me:

ExpectLoadCall and ExpectLoadForKeyCall will both send a QUIT to the current
message loop, but WaitForLoadCall will only run up to the first QUIT, leaving
the second one in the queue (which can cause problems for later code that
doesn't expect it to be there).

Suggested:

Modify ExpectLoadForKeyCall to set the two expectations (LoadForKey, followed by
Load). Only the second one will quit the loop.

[guohui, 2011/10/06 15:40:00]

Done.

+    EXPECT_CALL(*this, Begin()).WillOnce(action);
+    ExpectLoadCall();
+    ExpectLoadForKeyCall();
+    Begin();
+  }
+
   // Declares an expectation that PersistentCookieStore::Load will be called,
   // saving the provided callback and sending a quit to the message loop.
   void ExpectLoadCall() {
     EXPECT_CALL(*persistent_store_, Load(testing::_)).WillOnce(testing::DoAll(
         testing::SaveArg<0>(&loaded_callback_),
-        QuitCurrentMessageLoop(),
-        testing::Return(true)));
+        QuitCurrentMessageLoop()));
   }
 
+  // Declares an expectation that PersistentCookieStore::LoadCookiesForKey
+  // will be called, saving the provided callback and sending a quit to the
+  // message loop.
+  void ExpectLoadForKeyCall() {
+    CookieMonster::PersistentCookieStore::LoadedCallback *
+      callback = new CookieMonster::PersistentCookieStore::LoadedCallback();
+    EXPECT_CALL(*persistent_store_, LoadCookiesForKey(testing::_, testing::_)).
+        WillOnce(testing::DoAll(
+                 testing::SaveArg<1>(callback),
+                 QuitCurrentMessageLoop()));
+    loaded_for_key_callbacks_.push_back(callback);
+  }
+
   // Invokes the initial action.
   MOCK_METHOD0(Begin, void(void));
 
@@ -1073,11 +1103,17 @@
  private:
   // Declares that mock expectations in this test suite are strictly ordered.
   testing::InSequence in_sequence_;
-  // Holds cookies to be returned from PersistentCookieStore::Load.
+  // Holds cookies to be returned from PersistentCookieStore::Load or
+  // PersistentCookieStore::LoadCookiesForKey.
   std::vector<CookieMonster::CanonicalCookie*> loaded_cookies_;
   // Stores the callback passed from the CookieMonster to the
-  // PersistentCookieStore
+  // PersistentCookieStore::Load
   CookieMonster::PersistentCookieStore::LoadedCallback loaded_callback_;
+  // Stores the callback passed from the CookieMonster to the
+  // PersistentCookieStore::LoadCookiesForKey
+  std::vector<CookieMonster::PersistentCookieStore::LoadedCallback*>
+    loaded_for_key_callbacks_;
+
   // Stores the CookieMonster under test.
   scoped_refptr<CookieMonster> cookie_monster_;
   // Stores the mock PersistentCookieStore.
@@ -1091,7 +1127,7 @@
 
   MockGetCookiesCallback get_cookies_callback;
 
-  BeginWith(GetCookiesAction(
+  BeginWithForDomainKey(GetCookiesAction(
       &cookie_monster(), url_google_, &get_cookies_callback));
 
   WaitForLoadCall();
@@ -1111,7 +1147,7 @@
 
   MockGetCookieInfoCallback get_cookie_info_callback;
 
-  BeginWith(GetCookiesWithInfoAction(
+  BeginWithForDomainKey(GetCookiesWithInfoAction(
       &cookie_monster(), url_google_, &get_cookie_info_callback));
 
   WaitForLoadCall();
@@ -1128,7 +1164,7 @@
 TEST_F(DeferredCookieTaskTest, DeferredSetCookie) {
   MockSetCookiesCallback set_cookies_callback;
 
-  BeginWith(SetCookieAction(
+  BeginWithForDomainKey(SetCookieAction(
       &cookie_monster(), url_google_, "A=B", &set_cookies_callback));
 
   WaitForLoadCall();
@@ -1145,7 +1181,7 @@
 TEST_F(DeferredCookieTaskTest, DeferredDeleteCookie) {
   MockClosure delete_cookie_callback;
 
-  BeginWith(DeleteCookieAction(
+  BeginWithForDomainKey(DeleteCookieAction(
       &cookie_monster(), url_google_, "A", &delete_cookie_callback));
 
   WaitForLoadCall();
@@ -1162,7 +1198,7 @@
 TEST_F(DeferredCookieTaskTest, DeferredSetCookieWithDetails) {
   MockSetCookiesCallback set_cookies_callback;
 
-  BeginWith(SetCookieWithDetailsAction(
+  BeginWithForDomainKey(SetCookieWithDetailsAction(
       &cookie_monster(), url_google_foo_, "A", "B", std::string(), "/foo",
       base::Time(), false, false, &set_cookies_callback));
 
@@ -1205,7 +1241,7 @@
 
   MockGetCookieListCallback get_cookie_list_callback;
 
-  BeginWith(GetAllCookiesForUrlAction(
+  BeginWithForDomainKey(GetAllCookiesForUrlAction(
       &cookie_monster(), url_google_, &get_cookie_list_callback));
 
   WaitForLoadCall();
@@ -1227,7 +1263,7 @@
 
   MockGetCookieListCallback get_cookie_list_callback;
 
-  BeginWith(GetAllCookiesForUrlWithOptionsAction(
+  BeginWithForDomainKey(GetAllCookiesForUrlWithOptionsAction(
       &cookie_monster(), url_google_, &get_cookie_list_callback));
 
   WaitForLoadCall();
@@ -1278,7 +1314,7 @@
 TEST_F(DeferredCookieTaskTest, DeferredDeleteAllForHostCookies) {
   MockDeleteCallback delete_callback;
 
-  BeginWith(DeleteAllForHostAction(
+  BeginWithForDomainKey(DeleteAllForHostAction(
       &cookie_monster(), url_google_, &delete_callback));
 
   WaitForLoadCall();
@@ -1334,17 +1370,17 @@
       DeleteCookieAction(
           &cookie_monster(), url_google_, "A", &delete_cookie_callback)));
   ExpectLoadCall();
+  ExpectLoadForKeyCall();
   Begin();
 
   WaitForLoadCall();
   EXPECT_CALL(get_cookies_callback, Invoke("X=1")).WillOnce(
       GetCookiesWithInfoAction(
           &cookie_monster(), url_google_, &get_cookie_info_callback));
-
+  EXPECT_CALL(get_cookie_info_callback, Invoke("X=1", testing::_)).WillOnce(
+      QuitCurrentMessageLoop());
   EXPECT_CALL(set_cookies_callback, Invoke(true));
   EXPECT_CALL(delete_cookie_callback, Invoke());
-  EXPECT_CALL(get_cookie_info_callback, Invoke("X=1", testing::_)).WillOnce(
-      QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
@@ -2970,12 +3006,16 @@
  public:
   FlushablePersistentStore() : flush_count_(0) {}
 
-  bool Load(const LoadedCallback& loaded_callback) {
+  void Load(const LoadedCallback& loaded_callback) {
     std::vector<CookieMonster::CanonicalCookie*> out_cookies;
     loaded_callback.Run(out_cookies);
-    return false;
   }
 
+  void LoadCookiesForKey(const std::string& key,
+      const LoadedCallback& loaded_callback) {
+    Load(loaded_callback);
+  }
+
   void AddCookie(const CookieMonster::CanonicalCookie&) {}
   void UpdateCookieAccessTime(const CookieMonster::CanonicalCookie&) {}
   void DeleteCookie(const CookieMonster::CanonicalCookie&) {}