Split initial load of cookies by domains

net/base/cookie_monster.h

Index: net/base/cookie_monster.h
===================================================================
--- net/base/cookie_monster.h	(revision 100935)
+++ net/base/cookie_monster.h	(working copy)
@@ -10,6 +10,7 @@
 
 #include <map>
 #include <queue>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -450,9 +451,17 @@
   // Stores cookies loaded from the backing store and invokes any deferred
   // calls. |beginning_time| should be the moment PersistentCookieStore::Load
   // was invoked and is used for reporting histogram_time_load_.
+  // See PersistentCookieStore::Load for details on the contents of cookies.
   void OnLoaded(base::TimeTicks beginning_time,
                 const std::vector<CanonicalCookie*>& cookies);
 
+  // Stores cookies loaded from the backing store and invokes deferred callback.

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

Could you add something to this comment indicating what is invoked?  ("Requested
queued pending loading of cookies associated with eTLD+1 |key|" or something
like it?)

[guohui, 2011/09/20 18:34:46]

Done.

+  // Called when all cookies for the domain key(eTLD+1) has been loaded from DB.
+  // See PersistentCookieStore::Load for details on the contents of cookies.
+  void CookieMonster::OnKeyLoaded(
+    const base::Closure& request_task, const std::string& key,
+    const std::vector<CanonicalCookie*>& cookies);
+
   // Stores the loaded cookies.
   void StoreLoadedCookies(const std::vector<CanonicalCookie*>& cookies);
 
@@ -568,10 +577,15 @@
   // ugly and increment when we've seen the same time twice.
   base::Time CurrentTime();
 
-  // Run the cookie request task if cookie loaded, otherwise added the task
-  // to task queue.
+  // Runs the task if, or defers the task until, the full cookie database is
+  // loaded.
   void DoCookieTask(const scoped_refptr<CookieMonsterTask>& task_item);
 
+  // Runs the task if, or defers the task until, the cookies for the given URL
+  // are loaded.
+  void DoCookieTask(const scoped_refptr<CookieMonsterTask>& task_item,

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

The chrome style guide discourages overloading method names unless the methods
are doing basically the same thing and the signatures convey the same
information (e.g. a variation with std::string and with char*).  These two
strike me as doing different enough things to warrant different names by the
style guide; can you rename one?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

[guohui, 2011/09/20 18:34:46]

Done.

+    const GURL& url);
+
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
   base::Histogram* histogram_expiration_duration_minutes_;
@@ -597,6 +611,9 @@
   // calls may be immediately processed.
   bool loaded_;
 
+  // List of domain keys that have been loaded from DB
+  std::set<std::string> keys_loaded_;
+
   // Queues calls to CookieMonster until loading from the backend store is
   // completed.
   std::queue<scoped_refptr<CookieMonsterTask> > queue_;
@@ -620,10 +637,16 @@
   // This value is used to determine whether global garbage collection might
   // find cookies to purge.
   // Note: The default Time() constructor will create a value that compares
-  // earlier than any other time value, which is is wanted.  Thus this
+  // earlier than any other time value, which is wanted.  Thus this
   // value is not initialized.
   base::Time earliest_access_time_;
 
+  // During loading, holds the set of all loaded cookie creation times. Used to
+  // avoid ever letting cookies with duplicate creation times into the store;
+  // that way we don't have to worry about what sections of code are safe
+  // to call while it's in that state.
+  std::set<int64> creation_times_;
+
   std::vector<std::string> cookieable_schemes_;
 
   scoped_refptr<Delegate> delegate_;
@@ -919,9 +942,17 @@
       CookieMonster::CanonicalCookie*>&)> LoadedCallback;
 
   // Initializes the store and retrieves the existing cookies. This will be
-  // called only once at startup.
-  virtual bool Load(const LoadedCallback& loaded_callback) = 0;
+  // called only once at startup. The callback will return all the cookies
+  // that are not yet returned to CookieMonster by previous priority loads.
+  virtual void Load(const LoadedCallback& loaded_callback) = 0;
 
+  // Loads all cookies for the domain key(eTLD+1). The callback will return all
+  // the cookies that are not yet returned by previous loads, which includes
+  // cookies for the requested domain key if they are not already returned, plus
+  // all cookies that are chain-loaded and not yet returned to CookieMonster.
+  virtual void LoadCookiesForKey(const std::string& key,
+    const LoadedCallback& loaded_callback) = 0;

[Randy Smith (Not in Mondays), 2011/09/19 00:58:57]

Suggestion: Part of me thinks it would be simpler to have a single callback that
the persistent store is initialized with, that it always uses to return the
cookies.  This would mean that the cookie monster would have to have a list of
tasks (rather than bouncing them off the CookieStore) keyed by domain key.  But
it would mean you could eliminate the callback arguments here, which is a plus
mostly because those callback argument have a lot of subtle information on them
(OnKeyLoaded with a CookieMonsterTask curried in that's associated with the
given key).  I'm ok either way--I only suggest it because it took me several
read throughs to really follow the control flow.

Either way could you include a comment at top of file that describes the full
control flow for a cookie monster request, including (abstract) interaction with
the persistent store?  This'll duplicate material in the design doc, but people
never read those after the code's written anyway (and they tend to rot).

[guohui, 2011/09/20 18:34:46]

Comments added to the top of cookie_monster.h explaining the control flow.

On 2011/09/19 00:58:57, rdsmith wrote:

+
   virtual void AddCookie(const CanonicalCookie& cc) = 0;
   virtual void UpdateCookieAccessTime(const CanonicalCookie& cc) = 0;
   virtual void DeleteCookie(const CanonicalCookie& cc) = 0;