Split initial load of cookies by domains

net/base/cookie_monster_unittest.cc

 // Copyright (c) 2011 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.
 
 #include <time.h>
 
 #include <algorithm>
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 19 matching lines @@
 using base::TimeDelta;
 using base::Thread;
 
 namespace {
 
 // TODO(erikwright): Replace the pre-existing MockPersistentCookieStore (and
 // brethren) with this one, and remove the 'New' prefix.
 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));
   MOCK_METHOD1(DeleteCookie, void(const CookieMonster::CanonicalCookie& cc));
   MOCK_METHOD1(SetClearLocalStateOnExit, void(bool clear_local_state));
   MOCK_METHOD1(Flush, void(Task* completion_task));
 };
 
 const int kTimeout = 1000;
 
@@ skipping 952 matching lines @@
   cookie_monster->GetAllCookiesForURLWithOptionsAsync(
       url, CookieOptions(), callback->AsCallback());
 }
 
 ACTION_P3(GetAllCookiesForUrlAction, cookie_monster, url, callback) {
   cookie_monster->GetAllCookiesForURLAsync(url, callback->AsCallback());
 }
 
 }  // namespace
 
 // 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 {
  protected:
   DeferredCookieTaskTest() {
     persistent_store_ = new NewMockPersistentCookieStore();
     cookie_monster_ = new CookieMonster(persistent_store_.get(), NULL);
   }
 
   // Defines a cookie to be returned from PersistentCookieStore::Load
   void DeclareLoadedCookie(const std::string& key,
                       const std::string& cookie_line,
                       const base::Time& creation_time) {
     AddCookieToList(key, cookie_line, creation_time, &loaded_cookies_);
   }
 
   // Runs the message loop, waiting until PersistentCookieStore::Load is called.
   // Call CompleteLoadingAndWait to cause the load to complete.
   void WaitForLoadCall() {
     RunFor(kTimeout);
 
     // Verify that PeristentStore::Load was called.
     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));
 
   // Returns the CookieMonster instance under test.
   CookieMonster& cookie_monster() { return *cookie_monster_; }
 
  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.
   scoped_refptr<NewMockPersistentCookieStore> persistent_store_;
 };
 

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

Some of the new functionality is untested:

(1) Add a test that Begins with multiple actions for the same domain (you could
just define an ACTION_P3(GetCookies3TimesAction, ...)) and verifies that
LoadCookiesForDomain is called once but the get_cookies_callback is called 3
times.

(2) Add a test (or modify an existing test) to verify that the
get_cookies_callback is invoked before the Load callback is invoked (i.e., break
CompleteLoadingAndWait into two parts, CompleteKeyLoading and
CompleteGlobalLoading, hook up the last expected get_cookie_callback call to
call CompleteGlobalLoading).

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

Done.

 TEST_F(DeferredCookieTaskTest, DeferredGetCookies) {
   DeclareLoadedCookie("www.google.izzle",
                       "X=1; path=/; expires=Mon, 18-Apr-22 22:50:14 GMT",
                       Time::Now() + TimeDelta::FromDays(3));
 
   MockGetCookiesCallback get_cookies_callback;
 
-  BeginWith(GetCookiesAction(
+  BeginWithForDomainKey(GetCookiesAction(
       &cookie_monster(), url_google_, &get_cookies_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(get_cookies_callback, Invoke("X=1")).WillOnce(
       GetCookiesAction(&cookie_monster(), url_google_, &get_cookies_callback));
   EXPECT_CALL(get_cookies_callback, Invoke("X=1")).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 TEST_F(DeferredCookieTaskTest, DeferredGetCookiesWithInfo) {
   DeclareLoadedCookie("www.google.izzle",
                       "X=1; path=/; expires=Mon, 18-Apr-22 22:50:14 GMT",
                       Time::Now() + TimeDelta::FromDays(3));
 
   MockGetCookieInfoCallback get_cookie_info_callback;
 
-  BeginWith(GetCookiesWithInfoAction(
+  BeginWithForDomainKey(GetCookiesWithInfoAction(
       &cookie_monster(), url_google_, &get_cookie_info_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(get_cookie_info_callback, Invoke("X=1", testing::_)).WillOnce(
       GetCookiesWithInfoAction(
           &cookie_monster(), url_google_, &get_cookie_info_callback));
   EXPECT_CALL(get_cookie_info_callback, Invoke("X=1", testing::_)).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 TEST_F(DeferredCookieTaskTest, DeferredSetCookie) {
   MockSetCookiesCallback set_cookies_callback;
 
-  BeginWith(SetCookieAction(
+  BeginWithForDomainKey(SetCookieAction(
       &cookie_monster(), url_google_, "A=B", &set_cookies_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(set_cookies_callback, Invoke(true)).WillOnce(
       SetCookieAction(
           &cookie_monster(), url_google_, "X=Y", &set_cookies_callback));
   EXPECT_CALL(set_cookies_callback, Invoke(true)).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 TEST_F(DeferredCookieTaskTest, DeferredDeleteCookie) {
   MockClosure delete_cookie_callback;
 
-  BeginWith(DeleteCookieAction(
+  BeginWithForDomainKey(DeleteCookieAction(
       &cookie_monster(), url_google_, "A", &delete_cookie_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(delete_cookie_callback, Invoke()).WillOnce(
       DeleteCookieAction(
           &cookie_monster(), url_google_, "X", &delete_cookie_callback));
   EXPECT_CALL(delete_cookie_callback, Invoke()).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 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));
 
   WaitForLoadCall();
 
   EXPECT_CALL(set_cookies_callback, Invoke(true)).WillOnce(
       SetCookieWithDetailsAction(
           &cookie_monster(), url_google_foo_, "A", "B", std::string(), "/foo",
           base::Time(), false, false, &set_cookies_callback));
   EXPECT_CALL(set_cookies_callback, Invoke(true)).WillOnce(
@@ skipping 22 matching lines @@
   CompleteLoadingAndWait();
 }
 
 TEST_F(DeferredCookieTaskTest, DeferredGetAllForUrlCookies) {
   DeclareLoadedCookie("www.google.izzle",
                       "X=1; path=/; expires=Mon, 18-Apr-22 22:50:14 GMT",
                       Time::Now() + TimeDelta::FromDays(3));
 
   MockGetCookieListCallback get_cookie_list_callback;
 
-  BeginWith(GetAllCookiesForUrlAction(
+  BeginWithForDomainKey(GetAllCookiesForUrlAction(
       &cookie_monster(), url_google_, &get_cookie_list_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(get_cookie_list_callback, Invoke(testing::_)).WillOnce(
       GetAllCookiesForUrlAction(
           &cookie_monster(), url_google_, &get_cookie_list_callback));
   EXPECT_CALL(get_cookie_list_callback, Invoke(testing::_)).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 
 TEST_F(DeferredCookieTaskTest, DeferredGetAllForUrlWithOptionsCookies) {
   DeclareLoadedCookie("www.google.izzle",
                       "X=1; path=/; expires=Mon, 18-Apr-22 22:50:14 GMT",
                       Time::Now() + TimeDelta::FromDays(3));
 
   MockGetCookieListCallback get_cookie_list_callback;
 
-  BeginWith(GetAllCookiesForUrlWithOptionsAction(
+  BeginWithForDomainKey(GetAllCookiesForUrlWithOptionsAction(
       &cookie_monster(), url_google_, &get_cookie_list_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(get_cookie_list_callback, Invoke(testing::_)).WillOnce(
       GetAllCookiesForUrlWithOptionsAction(
           &cookie_monster(), url_google_, &get_cookie_list_callback));
   EXPECT_CALL(get_cookie_list_callback, Invoke(testing::_)).WillOnce(
       QuitCurrentMessageLoop());
 
@@ skipping 30 matching lines @@
           &delete_callback));
   EXPECT_CALL(delete_callback, Invoke(false)).WillOnce(
       QuitCurrentMessageLoop());
 
   CompleteLoadingAndWait();
 }
 
 TEST_F(DeferredCookieTaskTest, DeferredDeleteAllForHostCookies) {
   MockDeleteCallback delete_callback;
 
-  BeginWith(DeleteAllForHostAction(
+  BeginWithForDomainKey(DeleteAllForHostAction(
       &cookie_monster(), url_google_, &delete_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(delete_callback, Invoke(false)).WillOnce(
       DeleteAllForHostAction(
           &cookie_monster(), url_google_, &delete_callback));
   EXPECT_CALL(delete_callback, Invoke(false)).WillOnce(
       QuitCurrentMessageLoop());
 
@@ skipping 35 matching lines @@
   MockGetCookieInfoCallback get_cookie_info_callback;
 
   EXPECT_CALL(*this, Begin()).WillOnce(testing::DoAll(
       GetCookiesAction(
           &cookie_monster(), url_google_, &get_cookies_callback),
       SetCookieAction(
           &cookie_monster(), url_google_, "A=B", &set_cookies_callback),
       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();
 }
 
 TEST_F(CookieMonsterTest, DomainTest) {
   scoped_refptr<MockPersistentCookieStore> store(
       new MockPersistentCookieStore);
   scoped_refptr<CookieMonster> cm(new CookieMonster(store, NULL));
   EXPECT_TRUE(SetCookie(cm, url_google_, "A=B"));
   EXPECT_EQ("A=B", GetCookies(cm, url_google_));
@@ skipping 1605 matching lines @@
   ASSERT_TRUE(cookie_list[0].IsExpired(Time::Now()));
 }
 
 namespace {
 
 // Mock PersistentCookieStore that keeps track of the number of Flush() calls.
 class FlushablePersistentStore : public CookieMonster::PersistentCookieStore {
  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&) {}
   void SetClearLocalStateOnExit(bool clear_local_state) {}
 
   void Flush(Task* completion_callback) {
     ++flush_count_;
     if (completion_callback) {
@@ skipping 499 matching lines @@
   base::Closure task = base::Bind(
       &net::MultiThreadedCookieMonsterTest::DeleteCanonicalCookieTask,
       base::Unretained(this),
       cm, *it, &callback);
   RunOnOtherThread(task);
   EXPECT_TRUE(callback.did_run());
   EXPECT_TRUE(callback.result());
 }
 
 }  // namespace net