Split initial load of cookies by domains

net/base/cookie_monster.h

 // 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.
 
 // Brought to you by the letter D and the number 2.
 
 #ifndef NET_BASE_COOKIE_MONSTER_H_
 #define NET_BASE_COOKIE_MONSTER_H_
 #pragma once
 
 #include <map>
 #include <queue>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/task.h"
 #include "base/time.h"
 #include "net/base/cookie_store.h"
 #include "net/base/net_export.h"
 
 class GURL;
 
 namespace base {
 class Histogram;
 class TimeTicks;

[erikwright (departed), 2011/09/17 02:22:10]

forward declare "class Closure;" in namespace base.

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

Done.

 }

[erikwright (departed), 2011/09/17 02:22:10]

}  // namespace base

NB two spaces before the "//".

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

Done.

 
 namespace net {
 
 class CookieList;
 
 // The cookie monster is the system for storing and retrieving cookies. It has
 // an in-memory list of all cookies, and synchronizes non-session cookies to an
 // optional permanent storage that implements the PersistentCookieStore
 // interface.
 //
@@ skipping 400 matching lines @@
     }
   }
 
   // Initializes the backing store and reads existing cookies from it.
   // Should only be called by InitIfNecessary().
   void InitStore();
 
   // 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);
 
   // Invokes deferred calls.
   void InvokeQueue();
 
   // Checks that |cookies_| matches our invariants, and tries to repair any
   // inconsistencies. (In other words, it does not have duplicate cookies).
   void EnsureCookiesMapIsValid();
 
@@ skipping 95 matching lines @@
   void RecordPeriodicStats(const base::Time& current_time);
 
   // Initialize the above variables; should only be called from
   // the constructor.
   void InitializeHistograms();
 
   // The resolution of our time isn't enough, so we do something
   // 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_;
   base::Histogram* histogram_between_access_interval_minutes_;
   base::Histogram* histogram_evicted_last_access_minutes_;
   base::Histogram* histogram_count_;
   base::Histogram* histogram_domain_count_;
   base::Histogram* histogram_etldp1_count_;
   base::Histogram* histogram_domain_per_etldp1_count_;
   base::Histogram* histogram_number_duplicate_db_cookies_;
   base::Histogram* histogram_cookie_deletion_cause_;
   base::Histogram* histogram_time_get_;
   base::Histogram* histogram_time_mac_;
   base::Histogram* histogram_time_load_;
 
   CookieMap cookies_;
 
   // Indicates whether the cookie store has been initialized. This happens
   // lazily in InitStoreIfNecessary().
   bool initialized_;
 
   // Indicates whether loading from the backend store is completed and
   // 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_;
 
   // Indicates whether this cookie monster uses the new effective domain
   // key scheme or not.
   ExpiryAndKeyScheme expiry_and_key_scheme_;
 
   scoped_refptr<PersistentCookieStore> store_;
 
   base::Time last_time_seen_;
 
   // Minimum delay after updating a cookie's LastAccessDate before we will
   // update it again.
   const base::TimeDelta last_access_threshold_;
 
   // Approximate date of access time of least recently accessed cookie
   // in |cookies_|.  Note that this is not guaranteed to be accurate, only a)
   // to be before or equal to the actual time, and b) to be accurate
   // immediately after a garbage collection that scans through all the cookies.
   // 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_;
 
   // Lock for thread-safety
   base::Lock lock_;
 
   base::Time last_statistic_record_time_;
 
   bool keep_expired_cookies_;
@@ skipping 275 matching lines @@
 
 class CookieMonster::PersistentCookieStore
     : public RefcountedPersistentCookieStore {
  public:
   virtual ~PersistentCookieStore() {}
 
   typedef base::Callback<void(const std::vector<
       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;
 
   // Sets the value of the user preference whether the persistent storage
   // must be deleted upon destruction.
   virtual void SetClearLocalStateOnExit(bool clear_local_state) = 0;
 
   // Flush the store and post the given Task when complete.
   virtual void Flush(Task* completion_task) = 0;
 
  protected:
   PersistentCookieStore() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PersistentCookieStore);
 };
 
 class CookieList : public std::vector<CookieMonster::CanonicalCookie> {
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_COOKIE_MONSTER_H_