Switch cookie async mechanism over to using callbacks.

net/cookies/cookie_monster.cc

 // Copyright (c) 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.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 65 matching lines @@
 
 // In steady state, most cookie requests can be satisfied by the in memory
 // cookie monster store. If the cookie request cannot be satisfied by the in
 // memory store, the relevant cookies must be fetched from the persistent
 // store. The task is queued in CookieMonster::tasks_pending_ if it requires
 // all cookies to be loaded from the backend, or tasks_pending_for_key_ if it
 // only requires all cookies associated with an eTLD+1.
 //
 // On the browser critical paths (e.g. for loading initial web pages in a
 // session restore) it may take too long to wait for the full load. If a cookie
-// request is for a specific URL, DoCookieTaskForURL is called, which triggers a
-// priority load if the key is not loaded yet by calling PersistentCookieStore
-// :: LoadCookiesForKey. The request is queued in
+// request is for a specific URL, DoCookieCallbackForURL is called, which
+// triggers a priority load if the key is not loaded yet by calling
+// PersistentCookieStore::LoadCookiesForKey. The request is queued in
 // CookieMonster::tasks_pending_for_key_ and executed upon receiving
 // notification of key load completion via CookieMonster::OnKeyLoaded(). If
 // multiple requests for the same eTLD+1 are received before key load
 // completion, only the first request calls
 // PersistentCookieStore::LoadCookiesForKey, all subsequent requests are queued
 // in CookieMonster::tasks_pending_for_key_ and executed upon receiving
 // notification of key load completion triggered by the first request for the
 // same eTLD+1.
 
 static const int kMinutesInTenYears = 10 * 365 * 24 * 60;
 
 namespace {
 
 const char kFetchWhenNecessaryName[] = "FetchWhenNecessary";
 const char kAlwaysFetchName[] = "AlwaysFetch";
 const char kCookieMonsterFetchStrategyName[] = "CookieMonsterFetchStrategy";
 
+void WrapGetCookieListCallback(
+    base::OnceCallback<net::CookieList()> function,
+    base::OnceCallback<void(const net::CookieList&)> response) {
+  std::move(response).Run(std::move(function).Run());

[mmenke, 2017/07/07 16:54:58]

The old code null checked all of these callbacks.  Should this as well?

[Randy Smith (Not in Mondays), 2017/07/08 15:02:08]

Yeah, it should; in fact, try jobs fail if it doesn't.  Done.

+}
+
+void WrapSetCookiesCallback(base::OnceCallback<bool()> function,
+                            base::OnceCallback<void(bool)> response) {
+  std::move(response).Run(std::move(function).Run());
+}
+
+void WrapGetCookiesCallback(
+    base::OnceCallback<std::string()> function,
+    base::OnceCallback<void(const std::string&)> response) {
+  std::move(response).Run(std::move(function).Run());
+}
+
+void ConditionalDeleteCallback(base::WeakPtr<net::CookieMonster> cookie_monster,
+                               base::OnceClosure callback) {
+  if (cookie_monster)
+    std::move(callback).Run();
+}
+
+void WrapDeleteCallback(base::WeakPtr<net::CookieMonster> cookie_monster,
+                        base::OnceCallback<uint32_t()> function,
+                        base::OnceCallback<void(uint32_t)> response) {
+  uint32_t num_deleted = std::move(function).Run();
+
+  // This funciton is always called directly from a CookieMonster routine,
+  // so unconditional use of the weak pointer here is safe.
+  cookie_monster->FlushStore(
+      base::BindOnce(&ConditionalDeleteCallback, cookie_monster,
+                     base::BindOnce(std::move(response), num_deleted)));
+}
+
+void WrapDeleteCookieCallback(base::WeakPtr<net::CookieMonster> cookie_monster,
+                              base::OnceCallback<void()> function,
+                              base::OnceCallback<void()> response) {
+  std::move(function).Run();
+
+  // This funciton is always called directly from a CookieMonster routine,
+  // so unconditional use of the weak pointer here is safe.
+  cookie_monster->FlushStore(base::BindOnce(
+      &ConditionalDeleteCallback, cookie_monster, std::move(response)));
+}
+
 }  // namespace
 
 namespace net {
 
 // See comments at declaration of these variables in cookie_monster.h
 // for details.
 const size_t CookieMonster::kDomainMaxCookies = 180;
 const size_t CookieMonster::kDomainPurgeCookies = 30;
 const size_t CookieMonster::kMaxCookies = 3300;
 const size_t CookieMonster::kPurgeCookies = 300;
@@ skipping 263 matching lines @@
       channel_id_service_(channel_id_service),
       last_statistic_record_time_(base::Time::Now()),
       persist_session_cookies_(false),
       weak_ptr_factory_(this) {
   InitializeHistograms();
   cookieable_schemes_.insert(
       cookieable_schemes_.begin(), kDefaultCookieableSchemes,
       kDefaultCookieableSchemes + kDefaultCookieableSchemesCount);
 }
 
-// Task classes for queueing the coming request.
-
-class CookieMonster::CookieMonsterTask
-    : public base::RefCountedThreadSafe<CookieMonsterTask> {
- public:
-  // Runs the task and invokes the client callback on the thread that
-  // originally constructed the task.
-  virtual void Run() = 0;
-
- protected:
-  explicit CookieMonsterTask(CookieMonster* cookie_monster);
-  virtual ~CookieMonsterTask();
-
-  CookieMonster* cookie_monster() { return cookie_monster_; }
-
- private:
-  friend class base::RefCountedThreadSafe<CookieMonsterTask>;
-
-  CookieMonster* cookie_monster_;
-
-  DISALLOW_COPY_AND_ASSIGN(CookieMonsterTask);
-};
-
-CookieMonster::CookieMonsterTask::CookieMonsterTask(
-    CookieMonster* cookie_monster)
-    : cookie_monster_(cookie_monster) {}
-
-CookieMonster::CookieMonsterTask::~CookieMonsterTask() {
-}
-
-// Task class for SetCookieWithDetails call.
-class CookieMonster::SetCookieWithDetailsTask : public CookieMonsterTask {
- public:
-  SetCookieWithDetailsTask(CookieMonster* cookie_monster,
-                           const GURL& url,
-                           const std::string& name,
-                           const std::string& value,
-                           const std::string& domain,
-                           const std::string& path,
-                           base::Time creation_time,
-                           base::Time expiration_time,
-                           base::Time last_access_time,
-                           bool secure,
-                           bool http_only,
-                           CookieSameSite same_site,
-                           CookiePriority priority,
-                           SetCookiesCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        url_(url),
-        name_(name),
-        value_(value),
-        domain_(domain),
-        path_(path),
-        creation_time_(creation_time),
-        expiration_time_(expiration_time),
-        last_access_time_(last_access_time),
-        secure_(secure),
-        http_only_(http_only),
-        same_site_(same_site),
-        priority_(priority),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~SetCookieWithDetailsTask() override {}
-
- private:
-  GURL url_;
-  std::string name_;
-  std::string value_;
-  std::string domain_;
-  std::string path_;
-  base::Time creation_time_;
-  base::Time expiration_time_;
-  base::Time last_access_time_;
-  bool secure_;
-  bool http_only_;
-  CookieSameSite same_site_;
-  CookiePriority priority_;
-  SetCookiesCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(SetCookieWithDetailsTask);
-};
-
-void CookieMonster::SetCookieWithDetailsTask::Run() {
-  bool success = this->cookie_monster()->SetCookieWithDetails(
-      url_, name_, value_, domain_, path_, creation_time_, expiration_time_,
-      last_access_time_, secure_, http_only_, same_site_, priority_);
-  if (!callback_.is_null())
-    std::move(callback_).Run(success);
-}
-
-// Task class for GetAllCookies call.
-class CookieMonster::GetAllCookiesTask : public CookieMonsterTask {
- public:
-  GetAllCookiesTask(CookieMonster* cookie_monster,
-                    GetCookieListCallback callback)
-      : CookieMonsterTask(cookie_monster), callback_(std::move(callback)) {}
-
-  // CookieMonsterTask
-  void Run() override;
-
- protected:
-  ~GetAllCookiesTask() override {}
-
- private:
-  GetCookieListCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetAllCookiesTask);
-};
-
-void CookieMonster::GetAllCookiesTask::Run() {
-  if (!callback_.is_null()) {
-    CookieList cookies = this->cookie_monster()->GetAllCookies();
-    std::move(callback_).Run(cookies);
-  }
-}
-
-// Task class for GetCookieListWithOptionsAsync call.
-class CookieMonster::GetCookieListWithOptionsTask : public CookieMonsterTask {
- public:
-  GetCookieListWithOptionsTask(CookieMonster* cookie_monster,
-                               const GURL& url,
-                               const CookieOptions& options,
-                               GetCookieListCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        url_(url),
-        options_(options),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~GetCookieListWithOptionsTask() override {}
-
- private:
-  GURL url_;
-  CookieOptions options_;
-  GetCookieListCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetCookieListWithOptionsTask);
-};
-
-void CookieMonster::GetCookieListWithOptionsTask::Run() {
-  if (!callback_.is_null()) {
-    CookieList cookies =
-        this->cookie_monster()->GetCookieListWithOptions(url_, options_);
-    std::move(callback_).Run(cookies);
-  }
-}
-
-template <typename Result>
-struct CallbackType {
-  typedef base::OnceCallback<void(Result)> Type;
-};
-
-template <>
-struct CallbackType<void> {
-  typedef base::OnceClosure Type;
-};
-
-// Base task class for Delete*Task.
-template <typename Result>
-class CookieMonster::DeleteTask : public CookieMonsterTask {
- public:
-  DeleteTask(CookieMonster* cookie_monster,
-             typename CallbackType<Result>::Type callback)
-      : CookieMonsterTask(cookie_monster), callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~DeleteTask() override;
-
- private:
-  // Runs the delete task and returns a result.
-  virtual Result RunDeleteTask() = 0;
-  // Runs the delete task and then returns a callback to be called after
-  // flushing the persistent store.
-  // TODO(mmenke): This seems like a pretty ugly and needlessly confusing API.
-  // Simplify it?
-  base::OnceClosure RunDeleteTaskAndBindCallback();
-
-  typename CallbackType<Result>::Type callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeleteTask);
-};
-
-template <typename Result>
-CookieMonster::DeleteTask<Result>::~DeleteTask() {
-}
-
-template <typename Result>
-base::OnceClosure
-CookieMonster::DeleteTask<Result>::RunDeleteTaskAndBindCallback() {
-  Result result = RunDeleteTask();
-  if (callback_.is_null())
-    return base::OnceClosure();
-  return base::BindOnce(std::move(callback_), result);
-}
-
-template <>
-base::OnceClosure
-CookieMonster::DeleteTask<void>::RunDeleteTaskAndBindCallback() {
-  RunDeleteTask();
-  return std::move(callback_);
-}
-
-template <typename Result>
-void CookieMonster::DeleteTask<Result>::Run() {
-  base::OnceClosure callback = RunDeleteTaskAndBindCallback();
-  if (!callback.is_null()) {
-    callback =
-        base::BindOnce(&CookieMonster::RunCallback,
-                       this->cookie_monster()->weak_ptr_factory_.GetWeakPtr(),
-                       std::move(callback));
-  }
-  this->cookie_monster()->FlushStore(std::move(callback));
-}
-
-// Task class for DeleteAllCreatedBetween call.
-class CookieMonster::DeleteAllCreatedBetweenTask : public DeleteTask<uint32_t> {
- public:
-  DeleteAllCreatedBetweenTask(CookieMonster* cookie_monster,
-                              const Time& delete_begin,
-                              const Time& delete_end,
-                              DeleteCallback callback)
-      : DeleteTask<uint32_t>(cookie_monster, std::move(callback)),
-        delete_begin_(delete_begin),
-        delete_end_(delete_end) {}
-
-  // DeleteTask:
-  uint32_t RunDeleteTask() override;
-
- protected:
-  ~DeleteAllCreatedBetweenTask() override {}
-
- private:
-  Time delete_begin_;
-  Time delete_end_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeleteAllCreatedBetweenTask);
-};
-
-uint32_t CookieMonster::DeleteAllCreatedBetweenTask::RunDeleteTask() {
-  return this->cookie_monster()->DeleteAllCreatedBetween(delete_begin_,
-                                                         delete_end_);
-}
-
-// Task class for DeleteAllCreatedBetweenWithPredicate call.
-class CookieMonster::DeleteAllCreatedBetweenWithPredicateTask
-    : public DeleteTask<uint32_t> {
- public:
-  DeleteAllCreatedBetweenWithPredicateTask(
-      CookieMonster* cookie_monster,
-      Time delete_begin,
-      Time delete_end,
-      base::Callback<bool(const CanonicalCookie&)> predicate,
-      DeleteCallback callback)
-      : DeleteTask<uint32_t>(cookie_monster, std::move(callback)),
-        delete_begin_(delete_begin),
-        delete_end_(delete_end),
-        predicate_(predicate) {}
-
-  // DeleteTask:
-  uint32_t RunDeleteTask() override;
-
- protected:
-  ~DeleteAllCreatedBetweenWithPredicateTask() override {}
-
- private:
-  Time delete_begin_;
-  Time delete_end_;
-  base::Callback<bool(const CanonicalCookie&)> predicate_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeleteAllCreatedBetweenWithPredicateTask);
-};
-
-uint32_t
-CookieMonster::DeleteAllCreatedBetweenWithPredicateTask::RunDeleteTask() {
-  return this->cookie_monster()->DeleteAllCreatedBetweenWithPredicate(
-      delete_begin_, delete_end_, predicate_);
-}
-
-// Task class for DeleteCanonicalCookie call.
-class CookieMonster::DeleteCanonicalCookieTask : public DeleteTask<uint32_t> {
- public:
-  DeleteCanonicalCookieTask(CookieMonster* cookie_monster,
-                            const CanonicalCookie& cookie,
-                            DeleteCallback callback)
-      : DeleteTask<uint32_t>(cookie_monster, std::move(callback)),
-        cookie_(cookie) {}
-
-  // DeleteTask:
-  uint32_t RunDeleteTask() override;
-
- protected:
-  ~DeleteCanonicalCookieTask() override {}
-
- private:
-  CanonicalCookie cookie_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeleteCanonicalCookieTask);
-};
-
-uint32_t CookieMonster::DeleteCanonicalCookieTask::RunDeleteTask() {
-  return this->cookie_monster()->DeleteCanonicalCookie(cookie_);
-}
-
-// Task class for SetCanonicalCookie call.
-class CookieMonster::SetCanonicalCookieTask : public CookieMonsterTask {
- public:
-  SetCanonicalCookieTask(CookieMonster* cookie_monster,
-                         std::unique_ptr<CanonicalCookie> cookie,
-                         bool secure_source,
-                         bool modify_http_only,
-                         SetCookiesCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        cookie_(std::move(cookie)),
-        secure_source_(secure_source),
-        modify_http_only_(modify_http_only),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~SetCanonicalCookieTask() override {}
-
- private:
-  std::unique_ptr<CanonicalCookie> cookie_;
-  bool secure_source_;
-  bool modify_http_only_;
-  SetCookiesCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(SetCanonicalCookieTask);
-};
-
-void CookieMonster::SetCanonicalCookieTask::Run() {
-  bool result = this->cookie_monster()->SetCanonicalCookie(
-      std::move(cookie_), secure_source_, modify_http_only_);
-  if (!callback_.is_null())
-    std::move(callback_).Run(result);
-}
-
-// Task class for SetCookieWithOptions call.
-class CookieMonster::SetCookieWithOptionsTask : public CookieMonsterTask {
- public:
-  SetCookieWithOptionsTask(CookieMonster* cookie_monster,
-                           const GURL& url,
-                           const std::string& cookie_line,
-                           const CookieOptions& options,
-                           SetCookiesCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        url_(url),
-        cookie_line_(cookie_line),
-        options_(options),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~SetCookieWithOptionsTask() override {}
-
- private:
-  GURL url_;
-  std::string cookie_line_;
-  CookieOptions options_;
-  SetCookiesCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(SetCookieWithOptionsTask);
-};
-
-void CookieMonster::SetCookieWithOptionsTask::Run() {
-  bool result = this->cookie_monster()->SetCookieWithOptions(url_, cookie_line_,
-                                                             options_);
-  if (!callback_.is_null())
-    std::move(callback_).Run(result);
-}
-
-// Task class for SetAllCookies call.
-class CookieMonster::SetAllCookiesTask : public CookieMonsterTask {
- public:
-  SetAllCookiesTask(CookieMonster* cookie_monster,
-                    const CookieList& list,
-                    SetCookiesCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        list_(list),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~SetAllCookiesTask() override {}
-
- private:
-  CookieList list_;
-  SetCookiesCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(SetAllCookiesTask);
-};
-
-void CookieMonster::SetAllCookiesTask::Run() {

[Randy Smith (Not in Mondays), 2017/07/07 20:09:54]

Location of the code I refer to in the other comment.

-  CookieList positive_diff;
-  CookieList negative_diff;
-  CookieList old_cookies = this->cookie_monster()->GetAllCookies();
-  this->cookie_monster()->ComputeCookieDiff(&old_cookies, &list_,
-                                            &positive_diff, &negative_diff);
-
-  for (CookieList::const_iterator it = negative_diff.begin();
-       it != negative_diff.end(); ++it) {
-    this->cookie_monster()->DeleteCanonicalCookie(*it);
-  }
-
-  bool result = true;
-  if (positive_diff.size() > 0)
-    result = this->cookie_monster()->SetAllCookies(list_);
-
-  if (!callback_.is_null())
-    std::move(callback_).Run(result);
-}
-
-// Task class for GetCookiesWithOptions call.
-class CookieMonster::GetCookiesWithOptionsTask : public CookieMonsterTask {
- public:
-  GetCookiesWithOptionsTask(CookieMonster* cookie_monster,
-                            const GURL& url,
-                            const CookieOptions& options,
-                            GetCookiesCallback callback)
-      : CookieMonsterTask(cookie_monster),
-        url_(url),
-        options_(options),
-        callback_(std::move(callback)) {}
-
-  // CookieMonsterTask:
-  void Run() override;
-
- protected:
-  ~GetCookiesWithOptionsTask() override {}
-
- private:
-  GURL url_;
-  CookieOptions options_;
-  GetCookiesCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetCookiesWithOptionsTask);
-};
-
-void CookieMonster::GetCookiesWithOptionsTask::Run() {
-  std::string cookie =
-      this->cookie_monster()->GetCookiesWithOptions(url_, options_);
-  if (!callback_.is_null())
-    std::move(callback_).Run(cookie);
-}
-
-// Task class for DeleteCookie call.
-class CookieMonster::DeleteCookieTask : public DeleteTask<void> {
- public:
-  DeleteCookieTask(CookieMonster* cookie_monster,
-                   const GURL& url,
-                   const std::string& cookie_name,
-                   base::OnceClosure callback)
-      : DeleteTask<void>(cookie_monster, std::move(callback)),
-        url_(url),
-        cookie_name_(cookie_name) {}
-
-  // DeleteTask:
-  void RunDeleteTask() override;
-
- protected:
-  ~DeleteCookieTask() override {}
-
- private:
-  GURL url_;
-  std::string cookie_name_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeleteCookieTask);
-};
-
-void CookieMonster::DeleteCookieTask::RunDeleteTask() {
-  this->cookie_monster()->DeleteCookie(url_, cookie_name_);
-}
-
-// Task class for DeleteSessionCookies call.
-class CookieMonster::DeleteSessionCookiesTask : public DeleteTask<uint32_t> {
- public:
-  DeleteSessionCookiesTask(CookieMonster* cookie_monster,
-                           DeleteCallback callback)
-      : DeleteTask<uint32_t>(cookie_monster, std::move(callback)) {}
-
-  // DeleteTask:
-  uint32_t RunDeleteTask() override;
-
- protected:
-  ~DeleteSessionCookiesTask() override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(DeleteSessionCookiesTask);
-};
-
-uint32_t CookieMonster::DeleteSessionCookiesTask::RunDeleteTask() {
-  return this->cookie_monster()->DeleteSessionCookies();
-}
-
 // Asynchronous CookieMonster API
 
 void CookieMonster::SetCookieWithDetailsAsync(const GURL& url,
                                               const std::string& name,
                                               const std::string& value,
                                               const std::string& domain,
                                               const std::string& path,
                                               Time creation_time,
                                               Time expiration_time,
                                               Time last_access_time,
                                               bool secure,
                                               bool http_only,
                                               CookieSameSite same_site,
                                               CookiePriority priority,
                                               SetCookiesCallback callback) {
-  scoped_refptr<SetCookieWithDetailsTask> task = new SetCookieWithDetailsTask(
-      this, url, name, value, domain, path, creation_time, expiration_time,
-      last_access_time, secure, http_only, same_site, priority,
-      std::move(callback));
-  DoCookieTaskForURL(task, url);
+  DoCookieCallbackForURL(
+      base::BindOnce(

[mmenke, 2017/07/07 16:54:58]

Not suggesting it in this CL, but wonder if we could get rid of this double-bind
in the future by, for instance, having SetCookieWithDetails take + invoke the
callback itself.

[Randy Smith (Not in Mondays), 2017/07/08 15:02:08]

I need a word for "Yeah, but if I go in that direction the slippery slope will
catch me and I'll end up at the bottom of the hill" :-}.

So I thought about this and ended up with:
* I think this would work fine.
* It isn't needed for performance (at least according to CookieMonsterPerftest,
which is a completely reliable source of information on cookie performance :-z.)
* After some consideration I concluded that it would indeed be better in terms
of code size and comprehensibility.
* I started looking at the interfaces and realized that if I did this,
*CookieWith* and *CookieWith*Async would have the same signatures, which make me
think that then I should really combine those routines and conditionalize their
action based on the state of the cookie loading (which I think would be a win
for performance).
* I then realized that that mean I'd need to deal with reentrancy in the
callbacks, and PostTask the response if I wasn't actually queueing the callback
pending loading. 
* I then threw up my hands and decided that going further than what I've done
wasn't actually what I wanted to do in this CL, and by proxy, in the immediate
or planned future.

Let me know if you want to try and twist my arm, or if you want a TODO()
sketching out the above somewhere.  But for the moment I'm disinclined to go in
this, perfectly reasonable, direction.

[mmenke, 2017/07/10 16:27:28]

I'm not going to twist your arm on this.

Note that the current code (Both before and after this CL) already has to deal
with re-entrancy.  Callbacks for calls to non-delete methods can be called
re-entrantly, and that's expected behavior.

My concern isn't performance, but readability - I find nested binds very hard to
follow, when they're using in anything other than a very basic
post-task-and-reply-ish pattern (You can argue this pattern is somewhat similar,
but I find it sufficiently different that it's confusing).

I had not thought about merging the async / internal methods.

[mmenke, 2017/07/10 16:39:49]

Also note that in one of the delete case, we have 5 callbacks.

We take in a callback, we bind two here, and we bind two more in
WrapDeleteCallback, though the innermost bind isn't too complicated.

[Randy Smith (Not in Mondays), 2017/07/11 15:47:07]

Huh.  Good point.  

Hold off on doing another review for a couple of days--I'll look at how much of
a hassle merging the methods without worrying about reentrancy would be, and if
I decide not to do that I'll take a couple of steps back and think about how I
can make the current callback use less confusing.  I completely agree I'm going
a bit hog-wild with callbacks :-}.

[mmenke, 2017/07/11 15:50:54]

Another option is to land this, and then do another cleanup pass.  Or just land
this and not do another cleanup pass.  :)  Anyhow, I'll defer to you.  Just tell
me when I should look at this again.

+          &WrapSetCookiesCallback,
+          base::BindOnce(
+              // base::Unretained is safe as DoCookieCallbackForURL stores
+              // the callback on |*this|, so the callback will not outlive
+              // the object.
+              &CookieMonster::SetCookieWithDetails, base::Unretained(this), url,
+              name, value, domain, path, creation_time, expiration_time,
+              last_access_time, secure, http_only, same_site, priority),
+          std::move(callback)),
+      url);
 }
 
 void CookieMonster::FlushStore(base::OnceClosure callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (initialized_ && store_.get()) {
     if (channel_id_service_) {
       channel_id_service_->GetChannelIDStore()->Flush();
     }
     store_->Flush(std::move(callback));
   } else if (!callback.is_null()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   std::move(callback));
   }
 }
 
 void CookieMonster::SetForceKeepSessionState() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (store_)
     store_->SetForceKeepSessionState();
 }
 
 void CookieMonster::SetAllCookiesAsync(const CookieList& list,
                                        SetCookiesCallback callback) {
-  scoped_refptr<SetAllCookiesTask> task =
-      new SetAllCookiesTask(this, list, std::move(callback));
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapSetCookiesCallback,
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::SetAllCookies, base::Unretained(this), list),
+      std::move(callback)));
 }
 
 void CookieMonster::SetCanonicalCookieAsync(
     std::unique_ptr<CanonicalCookie> cookie,
     bool secure_source,
     bool modify_http_only,
     SetCookiesCallback callback) {
   DCHECK(cookie->IsCanonical());
-  scoped_refptr<SetCanonicalCookieTask> task =
-      new SetCanonicalCookieTask(this, std::move(cookie), secure_source,
-                                 modify_http_only, std::move(callback));
 
-  // TODO(rdsmith): Switch to DoCookieTaskForURL (or the equivalent).
+  // TODO(rdsmith): Switch to DoCookieCallbackForURL (or the equivalent).
   // This is tricky because we don't have the scheme in this routine
-  // and DoCookieTaskForURL uses cookie_util::GetEffectiveDomain(scheme, host)
+  // and DoCookieCallbackForURL uses
+  // cookie_util::GetEffectiveDomain(scheme, host)
   // to generate the database key to block behind.
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapSetCookiesCallback,
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::SetCanonicalCookie, base::Unretained(this),
+          std::move(cookie), secure_source, modify_http_only),
+      std::move(callback)));
 }
 
 void CookieMonster::SetCookieWithOptionsAsync(const GURL& url,
                                               const std::string& cookie_line,
                                               const CookieOptions& options,
                                               SetCookiesCallback callback) {
-  scoped_refptr<SetCookieWithOptionsTask> task = new SetCookieWithOptionsTask(
-      this, url, cookie_line, options, std::move(callback));
-
-  DoCookieTaskForURL(task, url);
+  DoCookieCallbackForURL(
+      base::BindOnce(
+          &WrapSetCookiesCallback,
+          base::BindOnce(
+              // base::Unretained is safe as DoCookieCallbackForURL stores
+              // the callback on |*this|, so the callback will not outlive
+              // the object.
+              &CookieMonster::SetCookieWithOptions, base::Unretained(this), url,
+              cookie_line, options),
+          std::move(callback)),
+      url);
 }
 
 void CookieMonster::GetCookiesWithOptionsAsync(const GURL& url,
                                                const CookieOptions& options,
                                                GetCookiesCallback callback) {
-  scoped_refptr<GetCookiesWithOptionsTask> task =
-      new GetCookiesWithOptionsTask(this, url, options, std::move(callback));
-
-  DoCookieTaskForURL(task, url);
+  DoCookieCallbackForURL(
+      base::BindOnce(
+          &WrapGetCookiesCallback,
+          base::BindOnce(
+              // base::Unretained is safe as DoCookieCallbackForURL stores
+              // the callback on |*this|, so the callback will not outlive
+              // the object.
+              &CookieMonster::GetCookiesWithOptions, base::Unretained(this),
+              url, options),
+          std::move(callback)),
+      url);
 }
 
 void CookieMonster::GetCookieListWithOptionsAsync(
     const GURL& url,
     const CookieOptions& options,
     GetCookieListCallback callback) {
-  scoped_refptr<GetCookieListWithOptionsTask> task =
-      new GetCookieListWithOptionsTask(this, url, options, std::move(callback));
-
-  DoCookieTaskForURL(task, url);
+  DoCookieCallbackForURL(
+      base::BindOnce(
+          &WrapGetCookieListCallback,
+          base::BindOnce(
+              // base::Unretained is safe as DoCookieCallbackForURL stores
+              // the callback on |*this|, so the callback will not outlive
+              // the object.
+              &CookieMonster::GetCookieListWithOptions, base::Unretained(this),
+              url, options),
+          std::move(callback)),
+      url);
 }
 
 void CookieMonster::GetAllCookiesAsync(GetCookieListCallback callback) {
-  scoped_refptr<GetAllCookiesTask> task =
-      new GetAllCookiesTask(this, std::move(callback));
-
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapGetCookieListCallback,
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::GetAllCookies, base::Unretained(this)),
+      std::move(callback)));
 }
 
 void CookieMonster::DeleteCookieAsync(const GURL& url,
                                       const std::string& cookie_name,
                                       base::OnceClosure callback) {
-  scoped_refptr<DeleteCookieTask> task =
-      new DeleteCookieTask(this, url, cookie_name, std::move(callback));
-
-  DoCookieTaskForURL(task, url);
+  DoCookieCallbackForURL(
+      base::BindOnce(
+          &WrapDeleteCookieCallback, weak_ptr_factory_.GetWeakPtr(),
+          base::BindOnce(
+              // base::Unretained is safe as DoCookieCallbackForURL stores
+              // the callback on |*this|, so the callback will not outlive
+              // the object.
+              &CookieMonster::DeleteCookie, base::Unretained(this), url,
+              cookie_name),
+          std::move(callback)),
+      url);
 }
 
 void CookieMonster::DeleteCanonicalCookieAsync(const CanonicalCookie& cookie,
                                                DeleteCallback callback) {
-  scoped_refptr<DeleteCanonicalCookieTask> task =
-      new DeleteCanonicalCookieTask(this, cookie, std::move(callback));
-
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::DeleteCanonicalCookie, base::Unretained(this),
+          cookie),
+      std::move(callback)));
 }
 
 void CookieMonster::DeleteAllCreatedBetweenAsync(const Time& delete_begin,
                                                  const Time& delete_end,
                                                  DeleteCallback callback) {
-  scoped_refptr<DeleteAllCreatedBetweenTask> task =
-      new DeleteAllCreatedBetweenTask(this, delete_begin, delete_end,
-                                      std::move(callback));
-
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::DeleteAllCreatedBetween, base::Unretained(this),
+          delete_begin, delete_end),
+      std::move(callback)));
 }
 
 void CookieMonster::DeleteAllCreatedBetweenWithPredicateAsync(
     const Time& delete_begin,
     const Time& delete_end,
     const base::Callback<bool(const CanonicalCookie&)>& predicate,
     DeleteCallback callback) {
   if (predicate.is_null()) {
     std::move(callback).Run(0);
     return;
   }
-  scoped_refptr<DeleteAllCreatedBetweenWithPredicateTask> task =
-      new DeleteAllCreatedBetweenWithPredicateTask(
-          this, delete_begin, delete_end, predicate, std::move(callback));
-  DoCookieTask(task);
+
+  DoCookieCallback(base::BindOnce(
+      &WrapDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::DeleteAllCreatedBetweenWithPredicate,
+          base::Unretained(this), delete_begin, delete_end, predicate),
+      std::move(callback)));
 }
 
 void CookieMonster::DeleteSessionCookiesAsync(
     CookieStore::DeleteCallback callback) {
-  scoped_refptr<DeleteSessionCookiesTask> task =
-      new DeleteSessionCookiesTask(this, std::move(callback));
-
-  DoCookieTask(task);
+  DoCookieCallback(base::BindOnce(
+      &WrapDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      base::BindOnce(
+          // base::Unretained is safe as DoCookieCallbackForURL stores
+          // the callback on |*this|, so the callback will not outlive
+          // the object.
+          &CookieMonster::DeleteSessionCookies, base::Unretained(this)),
+      std::move(callback)));
 }
 
 void CookieMonster::SetCookieableSchemes(
     const std::vector<std::string>& schemes) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Calls to this method will have no effect if made after a WebView or
   // CookieManager instance has been created.
   if (initialized_)
     return;
@@ skipping 374 matching lines @@
 
   auto tasks_pending_for_key = tasks_pending_for_key_.find(key);
 
   // TODO(mmenke): Can this be turned into a DCHECK?
   if (tasks_pending_for_key == tasks_pending_for_key_.end())
     return;
 
   // Run all tasks for the key. Note that running a task can result in multiple
   // tasks being added to the back of the deque.
   while (!tasks_pending_for_key->second.empty()) {
-    scoped_refptr<CookieMonsterTask> task =
-        tasks_pending_for_key->second.front();
+    base::OnceClosure task = std::move(tasks_pending_for_key->second.front());
     tasks_pending_for_key->second.pop_front();
-
-    task->Run();
+    std::move(task).Run();
   }
 
   tasks_pending_for_key_.erase(tasks_pending_for_key);
 
   // This has to be done last, in case running a task queues a new task for the
   // key, to ensure tasks are run in the correct order.
   keys_loaded_.insert(key);
 }
 
 void CookieMonster::StoreLoadedCookies(
@@ skipping 59 matching lines @@
 void CookieMonster::InvokeQueue() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Move all per-key tasks into the global queue, if there are any.  This is
   // protection about a race where the store learns about all cookies loading
   // before it learned about the cookies for a key loading.
 
   // Needed to prevent any recursively queued tasks from going back into the
   // per-key queues.
   seen_global_task_ = true;
-  for (const auto& tasks_for_key : tasks_pending_for_key_) {
-    tasks_pending_.insert(tasks_pending_.begin(), tasks_for_key.second.begin(),
-                          tasks_for_key.second.end());
+  for (auto& tasks_for_key : tasks_pending_for_key_) {
+    tasks_pending_.insert(tasks_pending_.begin(),
+                          std::make_move_iterator(tasks_for_key.second.begin()),
+                          std::make_move_iterator(tasks_for_key.second.end()));
   }
   tasks_pending_for_key_.clear();
 
   while (!tasks_pending_.empty()) {
-    scoped_refptr<CookieMonsterTask> request_task = tasks_pending_.front();
+    base::OnceClosure request_task = std::move(tasks_pending_.front());
     tasks_pending_.pop_front();
-    request_task->Run();
+    std::move(request_task).Run();
   }
 
   DCHECK(tasks_pending_for_key_.empty());
 
   finished_fetching_all_cookies_ = true;
   creation_times_.clear();
   keys_loaded_.clear();
 }
 
 void CookieMonster::EnsureCookiesMapIsValid() {
@@ skipping 326 matching lines @@
   // We assume that hopefully setting a cookie will be less common than
   // querying a cookie.  Since setting a cookie can put us over our limits,
   // make sure that we garbage collect...  We can also make the assumption that
   // if a cookie was set, in the common case it will be used soon after,
   // and we will purge the expired cookies in GetCookies().
   GarbageCollect(creation_date, key);
 
   return true;
 }
 
-bool CookieMonster::SetAllCookies(const CookieList& list) {
+bool CookieMonster::SetAllCookies(CookieList list) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  CookieList positive_diff;
+  CookieList negative_diff;
+  CookieList old_cookies = GetAllCookies();
+  ComputeCookieDiff(&old_cookies, &list, &positive_diff, &negative_diff);
+
+  for (CookieList::const_iterator it = negative_diff.begin();
+       it != negative_diff.end(); ++it) {
+    DeleteCanonicalCookie(*it);
+  }
+
+  if (positive_diff.size() == 0)
+    return true;

[mmenke, 2017/07/07 16:54:58]

This doesn't seem to belong in this CL.  I'm not at all sure we have sufficient
testing, in particular, so think this needs review independent of everything
else. I'm also wondering if it's used enough for us to care enough to do this.

[Randy Smith (Not in Mondays), 2017/07/07 20:09:54]

To be clear, this isn't new code--it's just a refactor.  The equivalent code
used to be in SetAllCookiesTask::Run.  Given that, do you still want to make
this statement?  I'm not sure what to do in this CL if I *don't* copy this code
over.

[mmenke, 2017/07/07 20:15:12]

Sorry, completely missed that.

[Randy Smith (Not in Mondays), 2017/07/08 15:02:08]

Yeah, so did I on my first pass, then the tests bopped me on the nose :-}.

+
   for (const auto& cookie : list) {
     const std::string key(GetKey(cookie.Domain()));
     Time creation_time = cookie.CreationDate();
     bool already_expired = cookie.IsExpired(creation_time);
 
     bool result =
         DeleteAnyEquivalentCookie(key, cookie, true, false, already_expired);
     DCHECK(!result);
 
     if (already_expired)
@@ skipping 485 matching lines @@
 }
 
 // The system resolution is not high enough, so we can have multiple
 // set cookies that result in the same system time.  When this happens, we
 // increment by one Time unit.  Let's hope computers don't get too fast.
 Time CookieMonster::CurrentTime() {
   return std::max(Time::Now(), Time::FromInternalValue(
                                    last_time_seen_.ToInternalValue() + 1));
 }
 
-void CookieMonster::DoCookieTask(
-    const scoped_refptr<CookieMonsterTask>& task_item) {
+void CookieMonster::DoCookieCallback(base::OnceClosure callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   MarkCookieStoreAsInitialized();
   FetchAllCookiesIfNecessary();
   seen_global_task_ = true;
 
   if (!finished_fetching_all_cookies_ && store_.get()) {
-    tasks_pending_.push_back(task_item);
+    tasks_pending_.push_back(std::move(callback));
     return;
   }
 
-  task_item->Run();
+  std::move(callback).Run();
 }
 
-void CookieMonster::DoCookieTaskForURL(
-    const scoped_refptr<CookieMonsterTask>& task_item,
-    const GURL& url) {
+void CookieMonster::DoCookieCallbackForURL(base::OnceClosure callback,
+                                           const GURL& url) {
   MarkCookieStoreAsInitialized();
   if (ShouldFetchAllCookiesWhenFetchingAnyCookie())
     FetchAllCookiesIfNecessary();
 
   // If cookies for the requested domain key (eTLD+1) have been loaded from DB
   // then run the task, otherwise load from DB.
   if (!finished_fetching_all_cookies_ && store_.get()) {
     // If a global task has been previously seen, queue the task as a global
     // task. Note that the CookieMonster may be in the middle of executing
     // the global queue, |tasks_pending_| may be empty, which is why another
     // bool is needed.
     if (seen_global_task_) {
-      tasks_pending_.push_back(task_item);
+      tasks_pending_.push_back(std::move(callback));
       return;
     }
 
     // Checks if the domain key has been loaded.
     std::string key(cookie_util::GetEffectiveDomain(url.scheme(), url.host()));
     if (keys_loaded_.find(key) == keys_loaded_.end()) {
-      std::map<std::string,
-               std::deque<scoped_refptr<CookieMonsterTask>>>::iterator it =
+      std::map<std::string, std::deque<base::OnceClosure>>::iterator it =
           tasks_pending_for_key_.find(key);
       if (it == tasks_pending_for_key_.end()) {
         store_->LoadCookiesForKey(
             key, base::Bind(&CookieMonster::OnKeyLoaded,
                             weak_ptr_factory_.GetWeakPtr(), key));
         it = tasks_pending_for_key_
-                 .insert(std::make_pair(
-                     key, std::deque<scoped_refptr<CookieMonsterTask>>()))
+                 .insert(std::make_pair(key, std::deque<base::OnceClosure>()))
                  .first;
       }
-      it->second.push_back(task_item);
+      it->second.push_back(std::move(callback));
       return;
     }
   }
 
-  task_item->Run();
+  std::move(callback).Run();
 }
 
 void CookieMonster::ComputeCookieDiff(CookieList* old_cookies,
                                       CookieList* new_cookies,
                                       CookieList* cookies_to_add,
                                       CookieList* cookies_to_delete) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   DCHECK(old_cookies);
   DCHECK(new_cookies);
@@ skipping 17 matching lines @@
       PartialDiffCookieSorter);
 
   // Select any new cookie for addition (or update) if no old cookie is exactly
   // equivalent.
   std::set_difference(new_cookies->begin(), new_cookies->end(),
                       old_cookies->begin(), old_cookies->end(),
                       std::inserter(*cookies_to_add, cookies_to_add->begin()),
                       FullDiffCookieSorter);
 }
 
-void CookieMonster::RunCallback(base::OnceClosure callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  std::move(callback).Run();
-}
-
 void CookieMonster::RunCookieChangedCallbacks(const CanonicalCookie& cookie,
                                               ChangeCause cause) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CookieOptions opts;
   opts.set_include_httponly();
   opts.set_same_site_cookie_mode(
       CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
   // Note that the callbacks in hook_map_ are wrapped with RunAsync(), so they
   // are guaranteed to not take long - they just post a RunAsync task back to
   // the appropriate thread's message loop and return.
   // TODO(mmenke): Consider running these synchronously?
   for (CookieChangedHookMap::iterator it = hook_map_.begin();
        it != hook_map_.end(); ++it) {
     std::pair<GURL, std::string> key = it->first;
     if (cookie.IncludeForRequestURL(key.first, opts) &&
         cookie.Name() == key.second) {
       it->second->Notify(cookie, cause);
     }
   }
 }
 
 }  // namespace net