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>
@@ -24,12 +25,13 @@
 #include "net/base/cookie_store.h"
 #include "net/base/net_export.h"
 
+class Closure;

[erikwright (departed), 2011/09/20 19:00:44]

This should go in the 'namespace base' block. You have just declared a new (and
unused) class named ::Closure, in addition to the base::Closure referenced
below.

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

Done.

 class GURL;
 
 namespace base {
 class Histogram;
 class TimeTicks;
-}
+}  // namespace base
 
 namespace net {
 
@@ -43,11 +45,20 @@
 // This class IS thread-safe. Normally, it is only used on the I/O thread, but
 // is also accessed directly through Automation for UI testing.
 //
-// Several methods exist in asynchronous forms. Calls may be deferred if all
-// affected cookies are not yet loaded from the backing store. Otherwise, the
-// callback may be invoked immediately (prior to return of the asynchronous
+// All cookie tasks are handled asynchronously. Tasks may be deferred if
+// all affected cookies, are not yet loaded from the backing store. Otherwise,

[erikwright (departed), 2011/09/20 19:00:44]

nit: this comma ("all affected cookies, ") should be removed.

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

Done.

+// the callback may be invoked immediately (prior to return of the asynchronous
 // function).
 //
+// A cookie task is either pending loading of the entire cookie store, or
+// loading of cookies for a specfic domain key(eTLD+1). In the former case, the
+// cookie task will be queued in queue_ while PersistentCookieStore chain loads
+// the cookie store on DB thread. In the later case, the cookie task will be

[erikwright (departed), 2011/09/20 19:00:44]

nit: later -> latter

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

Done.

+// passed as a callback to PermanentCookieStore:LoadCookiesForKey() which
+// triggers loading of cookies for the specified domain key(eTLD+1) on DB
+// thread. Please refer to the link below for detailed design documents.
+// http://www.chromium.org/developers/design-documents/cookie-split-loading

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

Have you brought the design docs up to date?

I'd recommend not relying on design docs for details; they're less likely to be
kept in sync with the code than comments in the code.  Relying on them for a
high level overview is fine, but if there are details you want to communicate,
it should be in the comments in the code.

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

Done.

+//
 // Callbacks are guaranteed to be invoked on the calling thread.
 //
 // TODO(deanm) Implement CookieMonster, the cookie database.
@@ -450,9 +461,19 @@
   // 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 the deferred
+  // CookieMonsterTask pending loading of cookies associated with the domain key

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

The interface contract are these sentences with "pending loading of cookies
associated with the domain key" elided; the nature of the passed in task isn't
under control of this routine.  If you want to say that the request_task should
be the one that's associated with the domain key handed to the persistent store
from (I don't remember the source), that's fine, but it's not part of the
interface contract and I think the comment here will be better if that's clear.

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

Done.

+  // (eTLD+1). Called when all cookies for the domain key(eTLD+1) have 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 +589,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 DoCookieTaskForURL(const scoped_refptr<CookieMonsterTask>& task_item,
+    const GURL& url);
+
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
   base::Histogram* histogram_expiration_duration_minutes_;
@@ -597,6 +623,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 +649,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 +954,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

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

General note: If you want to refer to arguments to a function you're commented
(or code in general) the convention is to do so by surrounding the code fragment
with vertical bars (e.g. |loaded_callback|).  I haven't seen places where you
literally refer to arguments without doing this convention, so there's no
request for change here, but there's enough conceptual references that I wanted
to make sure you had this tool in your toolbox.

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

Thanks for sharing the information, I was not aware of this convention ^_^

On 2011/09/21 20:52:39, rdsmith wrote:

+  // that are not yet returned to CookieMonster by previous priority loads.

[Randy Smith (Not in Mondays), 2011/09/21 20:52:39]

You use the phrase "priority loads" but don't define it (both here and in the
.cc file).  It's pretty clear what you mean (any cookies loaded through
LoadCookiesForKey count as priority loads) but that clarity has come after
reading the code pretty carefully, so I'd like to surface it better in the
comments.  I'd suggest you just change the description below to be "Do a
priority load of all cookies for the domain key (eTLD+1)."

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

Done.

+  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;
+
   virtual void AddCookie(const CanonicalCookie& cc) = 0;
   virtual void UpdateCookieAccessTime(const CanonicalCookie& cc) = 0;
   virtual void DeleteCookie(const CanonicalCookie& cc) = 0;