Remove the old synchronous CookieMonster API.

net/base/cookie_monster.h

Index: net/base/cookie_monster.h
===================================================================
--- net/base/cookie_monster.h	(revision 96178)
+++ net/base/cookie_monster.h	(working copy)
@@ -9,6 +9,7 @@
 #pragma once
 
 #include <map>
+#include <queue>
 #include <string>
 #include <utility>
 #include <vector>
@@ -27,11 +28,13 @@
 
 namespace base {
 class Histogram;
+class MessageLoopProxy;

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

Not used in this file--maybe it should be in cookie_monster.cc?

[erikwright (departed), 2011/09/06 17:34:45]

Done.

 }
 
 namespace net {
 
 class CookieList;
+class CookieMonsterTask;
 
 // 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
@@ -138,19 +141,20 @@
   // i.e. it doesn't begin with a leading '.' character.
   static bool DomainIsHostOnly(const std::string& domain_string);
 
+  // Helper function that adds all cookies from |cookie_monster| into this
+  // instance.
+  bool InitializeFrom(const CookieList& list);
+
+  typedef base::Callback<void(const CookieList& cookies)> GetCookieListCallback;
+  typedef base::Callback<void(int num_deleted)> DeleteCallback;
+  typedef SetCookiesCallback DeleteCookieCallback;
+
   // Sets a cookie given explicit user-provided cookie attributes. The cookie
   // name, value, domain, etc. are each provided as separate strings. This
   // function expects each attribute to be well-formed. It will check for
   // disallowed characters (e.g. the ';' character is disallowed within the
   // cookie value attribute) and will return false without setting the cookie
   // if such characters are found.
-  bool SetCookieWithDetails(const GURL& url,
-                            const std::string& name,
-                            const std::string& value,
-                            const std::string& domain,
-                            const std::string& path,
-                            const base::Time& expiration_time,
-                            bool secure, bool http_only);
   void SetCookieWithDetailsAsync(const GURL& url,
                                  const std::string& name,
                                  const std::string& value,
@@ -160,17 +164,10 @@
                                  bool secure, bool http_only,
                                  const SetCookiesCallback& callback);
 
-
-  // Helper function that adds all cookies from |cookie_monster| into this
-  // instance.
-  bool InitializeFrom(CookieMonster* cookie_monster);
-
   // Returns all the cookies, for use in management UI, etc. This does not mark
   // the cookies as having been accessed.
   // The returned cookies are ordered by longest path, then by earliest
   // creation date.
-  CookieList GetAllCookies();
-  typedef base::Callback<void(const CookieList& cookies)> GetCookieListCallback;
   void GetAllCookiesAsync(const GetCookieListCallback& callback);
 
   // Returns all the cookies, for use in management UI, etc. Filters results
@@ -178,8 +175,6 @@
   // mark the cookies as having been accessed.
   // The returned cookies are ordered by longest path, then earliest
   // creation date.
-  CookieList GetAllCookiesForURLWithOptions(const GURL& url,
-                                            const CookieOptions& options);
   void GetAllCookiesForURLWithOptionsAsync(
       const GURL& url,
       const CookieOptions& options,
@@ -187,38 +182,27 @@
 
   // Invokes GetAllCookiesForURLWithOptions with options set to include HTTP
   // only cookies.
-  CookieList GetAllCookiesForURL(const GURL& url);
   void GetAllCookiesForURLAsync(const GURL& url,
                                 const GetCookieListCallback& callback);
 
   // Deletes all of the cookies.
-  int DeleteAll(bool sync_to_store);
+  void DeleteAllAsync(const DeleteCallback& callback);
+
   // Deletes all of the cookies that have a creation_date greater than or equal
   // to |delete_begin| and less than |delete_end|
   // Returns the number of cookies that have been deleted.
-  int DeleteAllCreatedBetween(const base::Time& delete_begin,
-                              const base::Time& delete_end,
-                              bool sync_to_store);
-  typedef base::Callback<void(int num_deleted)> DeleteCallback;
   void DeleteAllCreatedBetweenAsync(const base::Time& delete_begin,
                                     const base::Time& delete_end,
-                                    bool sync_to_store,
                                     const DeleteCallback& callback);
 
   // Deletes all cookies that match the host of the given URL
   // regardless of path.  This includes all http_only and secure cookies,
   // but does not include any domain cookies that may apply to this host.
   // Returns the number of cookies deleted.
-  int DeleteAllForHost(const GURL& url);
-
   void DeleteAllForHostAsync(const GURL& url,
                              const DeleteCallback& callback);
 
   // Deletes one specific cookie.
-  bool DeleteCanonicalCookie(const CanonicalCookie& cookie);
-
-  typedef SetCookiesCallback DeleteCookieCallback;
-
   void DeleteCanonicalCookieAsync(const CanonicalCookie& cookie,
                                   const DeleteCookieCallback& callback);
 
@@ -260,42 +244,31 @@
 
   // Sets the cookies specified by |cookie_list| returned from |url|
   // with options |options| in effect.
-  virtual bool SetCookieWithOptions(const GURL& url,
-                                    const std::string& cookie_line,
-                                    const CookieOptions& options);
+  virtual void SetCookieWithOptionsAsync(
+      const GURL& url,
+      const std::string& cookie_line,
+      const CookieOptions& options,
+      const SetCookiesCallback& callback) OVERRIDE;
 
-  virtual void SetCookieWithOptionsAsync(const GURL& url,
-                                         const std::string& cookie_line,
-                                         const CookieOptions& options,
-                                         const SetCookiesCallback& callback);
-
   // Gets all cookies that apply to |url| given |options|.
   // The returned cookies are ordered by longest path, then earliest
   // creation date.
-  virtual std::string GetCookiesWithOptions(const GURL& url,
-                                            const CookieOptions& options);
-
   virtual void GetCookiesWithOptionsAsync(
       const GURL& url,
       const CookieOptions& options,
-      const GetCookiesCallback& callback);
+      const GetCookiesCallback& callback) OVERRIDE;
 
-  virtual void GetCookiesWithInfo(const GURL& url,
-                                  const CookieOptions& options,
-                                  std::string* cookie_line,
-                                  std::vector<CookieInfo>* cookie_infos);
+  virtual void GetCookiesWithInfoAsync(
+      const GURL& url,
+      const CookieOptions& options,
+      const GetCookieInfoCallback& callback) OVERRIDE;
 
-  virtual void GetCookiesWithInfoAsync(const GURL& url,
-                                       const CookieOptions& options,
-                                       const GetCookieInfoCallback& callback);
-
   // Deletes all cookies with that might apply to |url| that has |cookie_name|.
-  virtual void DeleteCookie(const GURL& url, const std::string& cookie_name);
   virtual void DeleteCookieAsync(
       const GURL& url, const std::string& cookie_name,
-      const base::Closure& callback);
+      const base::Closure& callback) OVERRIDE;
 
-  virtual CookieMonster* GetCookieMonster();
+  virtual CookieMonster* GetCookieMonster() OVERRIDE;
 
   // Debugging method to perform various validation checks on the map.
   // Currently just checking that there are no null CanonicalCookie pointers
@@ -309,6 +282,19 @@
   static const int kDefaultCookieableSchemesCount;
 
  private:
+  // For queueing the cookie monster calls.
+  friend class DeleteAllCreatedBetweenTask;
+  friend class DeleteAllForHostTask;
+  friend class DeleteAllTask;
+  friend class DeleteCookieTask;
+  friend class DeleteCanonicalCookieTask;
+  friend class GetAllCookiesForURLWithOptionsTask;
+  friend class GetAllCookiesTask;
+  friend class GetCookiesWithOptionsTask;
+  friend class GetCookiesWithInfoTask;
+  friend class SetCookieWithDetailsTask;
+  friend class SetCookieWithOptionsTask;

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

I'm feeling torn about this long list of friend classes, and the creation of all
of the different tasks.  It's both uglier and heresy, but I'm playing around
with the idea of a case statement instead (i.e. have a single object that can be
used for all the different operations that has the set union (literally; I'm not
suggesting a C union) of all the information of all of these objects plus an
enum to indicate which operation a particular object refers to, do a single
"friend class CookieMonster" inside that class, and then pass objects of that
class into a CookieMonster method (which is where the case statement will be)
for execution.  I think that results in a lot less code, many fewer friends of
the CookieMonster (the CookieMonster should have no friends--more cookies for
it!!  :-}); it's not good object oriented polymorphism, but I think it would
still be pretty clear.  I won't insist on the idea, but I wanted to float it by
you in case it appealed to you over this approach.

[erikwright (departed), 2011/09/06 17:34:45]

The case statement, IME is more error prone in addition to being uglier and
heresy :)

I would prefer to reduce the number of methods that actually need to be
asynchronous by extracting different smaller classes out of this one. This will
have the same effect (in fact, probably even a better outcome).

[Randy Smith (Not in Mondays), 2011/09/07 15:46:27]

Ok.

+
   // Testing support.
   // For SetCookieWithCreationTime.
   FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest,
@@ -397,6 +383,44 @@
 
   virtual ~CookieMonster();
 
+  bool SetCookieWithDetails(const GURL& url,

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

Maybe a comment before these indicating how they're used and what the
constraints are (I'm presuming by the async callbacks, only valid to call once
the db is loaded).

[erikwright (departed), 2011/09/06 17:34:45]

Done.

+                            const std::string& name,
+                            const std::string& value,
+                            const std::string& domain,
+                            const std::string& path,
+                            const base::Time& expiration_time,
+                            bool secure, bool http_only);
+
+  CookieList GetAllCookies();
+
+  CookieList GetAllCookiesForURLWithOptions(const GURL& url,
+                                            const CookieOptions& options);
+
+  CookieList GetAllCookiesForURL(const GURL& url);

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

nit: If this is now only a helper call called from a single place, you can
remove it and force callers to use the more detailed version.

[erikwright (departed), 2011/09/06 17:34:45]

It has four callers besides tests. For now I'll leave it I guess.

On 2011/08/19 21:40:05, rdsmith wrote:

+
+  int DeleteAll(bool sync_to_store);
+
+  int DeleteAllCreatedBetween(const base::Time& delete_begin,
+                              const base::Time& delete_end);
+
+  int DeleteAllForHost(const GURL& url);
+
+  bool DeleteCanonicalCookie(const CanonicalCookie& cookie);
+
+  bool SetCookieWithOptions(const GURL& url,
+                            const std::string& cookie_line,
+                            const CookieOptions& options);
+
+  std::string GetCookiesWithOptions(const GURL& url,
+                                    const CookieOptions& options);
+
+  void GetCookiesWithInfo(const GURL& url,
+                          const CookieOptions& options,
+                          std::string* cookie_line,
+                          std::vector<CookieInfo>* cookie_infos);
+
+  void DeleteCookie(const GURL& url, const std::string& cookie_name);
+
   bool SetCookieWithCreationTime(const GURL& url,
                                  const std::string& cookie_line,
                                  const base::Time& creation_time);
@@ -407,8 +431,11 @@
   // Note: this method should always be called with lock_ held.
   void InitIfNecessary() {
     if (!initialized_) {
-      if (store_)
+      if (store_) {
         InitStore();
+      } else {
+        loaded_ = true;
+      }
       initialized_ = true;
     }
   }
@@ -417,6 +444,16 @@
   // Should only be called by InitIfNecessary().
   void InitStore();
 
+  // Reads the existing cookies loaded form the backing store.

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

I'd suggest instead "Called when loading of the existing cookies on the backing
store is complete."

[erikwright (departed), 2011/09/06 17:34:45]

Done.

+  void OnLoaded(const std::vector<CanonicalCookie*>& cookies);
+
+  // Ininitalize the cookies.
+  void StoreLoadedCookies(const std::vector<CanonicalCookie*>& cookies);
+
+  // Invoke the queueing calls of CookieMonster when loading cookies from

[Randy Smith (Not in Mondays), 2011/08/19 21:40:05]

nit: "queueing" -> "queued".

[erikwright (departed), 2011/09/06 17:34:45]

Done.

+  // the back store is complete.
+  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();
@@ -526,6 +563,10 @@
   // 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.
+  void DoCookieTask(scoped_refptr<CookieMonsterTask> task_item);
+
   // Histogram variables; see CookieMonster::InitializeHistograms() in
   // cookie_monster.cc for details.
   base::Histogram* histogram_expiration_duration_minutes_;
@@ -547,6 +588,14 @@
   // lazily in InitStoreIfNecessary().
   bool initialized_;
 
+  // Indicates whether loading from the backend store is completed and
+  // calls may be immediately processed.
+  bool loaded_;
+
+  // Queues calls to CookieMonster until loading from the backend store is
+  // completed.
+  std::queue<base::Closure> queue_;
+
   // Indicates whether this cookie monster uses the new effective domain
   // key scheme or not.
   ExpiryAndKeyScheme expiry_and_key_scheme_;
@@ -617,6 +666,12 @@
 
   // Supports the default copy constructor.
 
+  // Creates a canonical cookie from parsed cookie.
+  // Canonicalizes and validates inputs.  May return NULL if an attribute
+  // value is invalid.
+  static CanonicalCookie* Create(const GURL& url,
+                                 const ParsedCookie& pc);
+
   // Creates a canonical cookie from unparsed attribute values.
   // Canonicalizes and validates inputs.  May return NULL if an attribute
   // value is invalid.
@@ -855,9 +910,12 @@
  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(std::vector<CookieMonster::CanonicalCookie*>* cookies) = 0;
+  virtual bool Load(const LoadedCallback& loaded_callback) = 0;
 
   virtual void AddCookie(const CanonicalCookie& cc) = 0;
   virtual void UpdateCookieAccessTime(const CanonicalCookie& cc) = 0;