Divide CookieStoreIOS into two different classes with different backends

ios/net/cookies/cookie_store_ios.h

 // Copyright 2012 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.
 
 #ifndef IOS_NET_COOKIES_COOKIE_STORE_IOS_H_
 #define IOS_NET_COOKIES_COOKIE_STORE_IOS_H_
 
 #include <map>
 #include <memory>
 #include <string>
@@ skipping 23 matching lines @@
 // Observer for changes on |NSHTTPCookieStorge sharedHTTPCookieStorage|.
 class CookieNotificationObserver {
  public:
   // Called when any cookie is added, deleted or changed in
   // |NSHTTPCookieStorge sharedHTTPCookieStorage|.
   virtual void OnSystemCookiesChanged() = 0;
 };
 
 // The CookieStoreIOS is an implementation of CookieStore relying on
 // NSHTTPCookieStorage, ensuring that the cookies are consistent between the
-// network stack and NSHTTPCookieStorage.
-// CookieStoreIOS is not thread safe.
+// network stack and NSHTTPCookieStorage. CookieStoreIOS is not thread safe.
 //
-// CookieStoreIOS can be created synchronized with the system cookie store (via
-// CreateCookieStore) or not (other constructors). If a CookieStoreIOS is not
-// synchronized with the system store, changes are written back to the backing
-// CookieStore. If a CookieStoreIOS is synchronized with the system store,
+// CookieStoreIOS is created synchronized with the system cookie store -
 // changes are written directly to the system cookie store, then propagated to
 // the backing store by OnSystemCookiesChanged, which is called by the system
 // store once the change to the system store is written back.
+// For not synchronized CookieStore, please see CookieStoreIOSPersistent.
 class CookieStoreIOS : public net::CookieStore,
                        public CookieNotificationObserver {
  public:
-  // Creates a CookieStoreIOS with a default value of
-  // |NSHTTPCookieStorage sharedCookieStorage| as the system's cookie store.
-  explicit CookieStoreIOS(
-      net::CookieMonster::PersistentCookieStore* persistent_store);
+  // Create an instance of CookieStoreIOS that is generated from the cookies

[Eugene But (OOO till 7-30), 2017/01/23 17:37:47]

nit: s/Create/Creates

I realize that this is just moving the code, but still would be great to fix.
From Style Guide:
These comments should be descriptive ("Opens the file") rather than imperative
("Open the file"); 
https://google.github.io/styleguide/cppguide.html#Function_Comments

[maksims (do not use this acc), 2017/01/24 10:23:46]

Done.

+  // stored in |cookie_storage|. The CookieStoreIOS uses the |cookie_storage|
+  // as its default backend and is initially synchronized with it.
+  // Apple does not persist the cookies' creation dates in NSHTTPCookieStorage,
+  // so callers should not expect these values to be populated.
+  explicit CookieStoreIOS(NSHTTPCookieStorage* cookie_storage);
 
   ~CookieStoreIOS() override;
 
   enum CookiePolicy { ALLOW, BLOCK };
 
-  // Create an instance of CookieStoreIOS that is generated from the cookies
-  // stored in |cookie_storage|. The CookieStoreIOS uses the |cookie_storage|
-  // as its default backend and is initially synchronized with it.
-  // Apple does not persist the cookies' creation dates in NSHTTPCookieStorage,
-  // so callers should not expect these values to be populated.
-  static std::unique_ptr<CookieStoreIOS> CreateCookieStore(

[Eugene But (OOO till 7-30), 2017/01/23 17:37:47]

This will break compilation in close source Chrome for iOS repo:
https://bugs.chromium.org/p/chromium/issues/detail?id=679736#c2

Could you please do these things:
1.) Leave this method for now. Its implementation should call
CookieStoreIOS(NSHTTPCookieStorage*) constructor to be compatible with existing
code.
2.) Add TODO(crbug.com/683964): Remove this method.

Then I will remove CreateCookieStore call from that close-source repo and will
clean up this method.

Thanks!

[maksims (do not use this acc), 2017/01/24 10:23:46]

Oh, that what you meant. I though If it could compile with the try bots, then it
would be safe to remove the function.

-      NSHTTPCookieStorage* cookie_storage);
-
   // Must be called when the state of
   // |NSHTTPCookieStorage sharedHTTPCookieStorage| changes.
   // Affects only those CookieStoreIOS instances that are backed by
   // |NSHTTPCookieStorage sharedHTTPCookieStorage|.
   static void NotifySystemCookiesChanged();
 
   // Only one cookie store may enable metrics.
   void SetMetricsEnabled();
 
   // Inherited CookieStore methods.
@@ skipping 40 matching lines @@
   void DeleteSessionCookiesAsync(const DeleteCallback& callback) override;
   void FlushStore(const base::Closure& callback) override;
 
   std::unique_ptr<CookieChangedSubscription> AddCallbackForCookie(
       const GURL& url,
       const std::string& name,
       const CookieChangedCallback& callback) override;
 
   bool IsEphemeral() override;
 
- private:
-  CookieStoreIOS(
-      net::CookieMonster::PersistentCookieStore* persistent_store,
-      NSHTTPCookieStorage* system_store);
-
-  // For tests.
-  friend struct CookieStoreIOSTestTraits;
-
+ protected:
   enum SynchronizationState {
     NOT_SYNCHRONIZED,  // Uses CookieMonster as backend.
     SYNCHRONIZED       // Uses NSHTTPCookieStorage as backend.
   };
 
-  // Cookie fliter for DeleteCookiesWithFilter().
-  // Takes a cookie and a creation time and returns true if the cookie must be
+  CookieStoreIOS(SynchronizationState state,
+                 net::CookieMonster::PersistentCookieStore* persistent_store,
+                 NSHTTPCookieStorage* system_store);
+
+  // These three functions are used for wrapping user-supplied callbacks given
+  // to CookieStoreIOS mutator methods. Given a callback, they return a new
+  // callback that invokes UpdateCachesFromCookieMonster() to schedule an
+  // asynchronous synchronization of the cookie cache and then calls the
+  // original callback.
+  SetCookiesCallback WrapSetCallback(const SetCookiesCallback& callback);
+  DeleteCallback WrapDeleteCallback(const DeleteCallback& callback);
+  base::Closure WrapClosure(const base::Closure& callback);
+
+  bool metrics_enabled() { return metrics_enabled_; }
+
+  net::CookieMonster* cookie_monster() { return cookie_monster_.get(); }
+
+  const base::ThreadChecker& thread_checker() { return thread_checker_; }
+
+ private:
+  // For tests.
+  friend struct CookieStoreIOSTestTraits;
+
+  // Cookie filter for DeleteCookiesWithFilter().
+  // Takes a cookie and a creation time and returns true the cookie must be
   // deleted.
   typedef base::Callback<bool(NSHTTPCookie*, base::Time)> CookieFilterFunction;
 
   // Clears the system cookie store.
   void ClearSystemStore();
   // Returns true if the system cookie store policy is
   // |NSHTTPCookieAcceptPolicyAlways|.
   bool SystemCookiesAllowed();
   // Copies the cookies to the backing CookieMonster. If the cookie store is not
   // synchronized with the system store, this is a no-op.
   void WriteToCookieMonster(NSArray* system_cookies);
 
   // Inherited CookieNotificationObserver methods.
   void OnSystemCookiesChanged() override;
 
   void DeleteCookiesWithFilter(const CookieFilterFunction& filter,
                                const DeleteCallback& callback);
 
   std::unique_ptr<net::CookieMonster> cookie_monster_;
   base::scoped_nsobject<NSHTTPCookieStorage> system_store_;
   std::unique_ptr<CookieCreationTimeManager> creation_time_manager_;
   bool metrics_enabled_;
   base::CancelableClosure flush_closure_;
 
   SynchronizationState synchronization_state_;

[Eugene But (OOO till 7-30), 2017/01/23 17:37:47]

I think we can remove this member as well (see comments in the implementation).
If you want to do this in a separate CL, then please add TODOs to the code.

[maksims (do not use this acc), 2017/01/24 10:23:46]

Done.

 
   base::ThreadChecker thread_checker_;
 
   // Cookie notification methods.
   // The cookie cache is updated from both the system store and the
   // CookieStoreIOS' own mutators. Changes when the CookieStoreIOS is
   // synchronized are signalled by the system store; changes when the
   // CookieStoreIOS is not synchronized are signalled by the appropriate
   // mutators on CookieStoreIOS. The cookie cache tracks the system store when
   // the CookieStoreIOS is synchronized and the CookieStore when the
@@ skipping 74 matching lines @@
 
   void UpdateCachesAfterSet(const SetCookiesCallback& callback, bool success);
   void UpdateCachesAfterDelete(const DeleteCallback& callback, int num_deleted);
   void UpdateCachesAfterClosure(const base::Closure& callback);
 
   // Takes an NSArray of NSHTTPCookies as returns a net::CookieList.
   // The returned cookies are ordered by longest path, then earliest
   // creation date.
   net::CookieList CanonicalCookieListFromSystemCookies(NSArray* cookies);
 
-  // These three functions are used for wrapping user-supplied callbacks given
-  // to CookieStoreIOS mutator methods. Given a callback, they return a new
-  // callback that invokes UpdateCachesFromCookieMonster() to schedule an
-  // asynchronous synchronization of the cookie cache and then calls the
-  // original callback.
-
-  SetCookiesCallback WrapSetCallback(const SetCookiesCallback& callback);
-  DeleteCallback WrapDeleteCallback(const DeleteCallback& callback);
-  base::Closure WrapClosure(const base::Closure& callback);
-
   // Cached values of system cookies. Only cookies which have an observer added
   // with AddCallbackForCookie are kept in this cache.
   std::unique_ptr<CookieCache> cookie_cache_;
 
   // Callbacks for cookie changes installed by AddCallbackForCookie.
   std::map<std::pair<GURL, std::string>,
            std::unique_ptr<CookieChangedCallbackList>>
       hook_map_;
 
   base::WeakPtrFactory<CookieStoreIOS> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CookieStoreIOS);
 };
 
 }  // namespace net
 
 #endif  // IOS_NET_COOKIES_COOKIE_STORE_IOS_H_