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 64 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 ConditionalDeleteCallback(base::WeakPtr<net::CookieMonster> cookie_monster,

[mmenke, 2017/07/12 18:29:04]

Suggest renaming these so it's clearer they call callback, and are not callbacks
themselves.  ConditionallyCallFooCallback? InvokeFooCallback?
MaybeRunFooCallback?

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

+                               base::OnceClosure callback) {
+  if (cookie_monster && !callback.is_null())

[mmenke, 2017/07/12 18:29:04]

nit:  For all of these, can just use callback instead of !callback.is_null(), I
believe.

[Randy Smith (Not in Mondays), 2017/07/12 19:47:34]

Done.

+    std::move(callback).Run();
+}
+
+void ConditionalCookieCallback(base::OnceClosure callback) {
+  if (!callback.is_null())
+    std::move(callback).Run();
+}
+
+template <typename T>
+void ConditionalCookieCallback(base::OnceCallback<void(const T&)> callback,
+                               T result) {

[mmenke, 2017/07/12 18:29:04]

If these two are called with strings, or cookies, will they do an extra copy? 
i.e., should these by const T& result (Or invoked with std::move)

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

+  if (!callback.is_null())
+    std::move(callback).Run(result);
+}
+
+template <typename T>
+void ConditionalCookieCallback(base::OnceCallback<void(T)> callback, T result) {
+  if (!callback.is_null())
+    std::move(callback).Run(result);
+}
+
 }  // 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() {
-  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(
+          // 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(
+      // 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(
+      // 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(
+          // 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(
+          // 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(
+          // 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(
+      // 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(
+          // 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(
+      // 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(
+      // 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);
+    ConditionalCookieCallback(std::move(callback), 0u);
     return;
   }
-  scoped_refptr<DeleteAllCreatedBetweenWithPredicateTask> task =
-      new DeleteAllCreatedBetweenWithPredicateTask(
-          this, delete_begin, delete_end, predicate, std::move(callback));
-  DoCookieTask(task);
+
+  DoCookieCallback(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(
+      // 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 43 matching lines @@
   // CookieChanged callbacks when the CookieStore is destroyed?
   for (CookieMap::iterator cookie_it = cookies_.begin();
        cookie_it != cookies_.end();) {
     CookieMap::iterator current_cookie_it = cookie_it;
     ++cookie_it;
     InternalDeleteCookie(current_cookie_it, false /* sync_to_store */,
                          DELETE_COOKIE_DONT_RECORD);
   }
 }
 
-bool CookieMonster::SetCookieWithDetails(const GURL& url,
+void CookieMonster::SetCookieWithDetails(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) {
+                                         CookiePriority priority,
+                                         SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!HasCookieableScheme(url))
-    return false;
+  if (!HasCookieableScheme(url)) {
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
+  }
 
   // Validate consistency of passed arguments.
   if (ParsedCookie::ParseTokenString(name) != name ||
       ParsedCookie::ParseValueString(value) != value ||
       ParsedCookie::ParseValueString(domain) != domain ||
       ParsedCookie::ParseValueString(path) != path) {
-    return false;
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
   }
 
   std::string cookie_domain;
-  if (!cookie_util::GetCookieDomainWithString(url, domain, &cookie_domain))
-    return false;
+  if (!cookie_util::GetCookieDomainWithString(url, domain, &cookie_domain)) {
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
+  }
 
   std::string cookie_path = CanonicalCookie::CanonPathWithString(url, path);
-  if (!path.empty() && cookie_path != path)
-    return false;
+  if (!path.empty() && cookie_path != path) {
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
+  }
 
   // Canonicalize path again to make sure it escapes characters as needed.
   url::Component path_component(0, cookie_path.length());
   url::RawCanonOutputT<char> canon_path;
   url::Component canon_path_component;
   url::CanonicalizePath(cookie_path.data(), path_component, &canon_path,
                         &canon_path_component);
   cookie_path = std::string(canon_path.data() + canon_path_component.begin,
                             canon_path_component.len);
 
   std::unique_ptr<CanonicalCookie> cc(base::MakeUnique<CanonicalCookie>(
       name, value, cookie_domain, cookie_path, creation_time, expiration_time,
       last_access_time, secure, http_only, same_site, priority));
 
-  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true);
+  SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true,
+                     std::move(callback));
 }
 
-CookieList CookieMonster::GetAllCookies() {
+void CookieMonster::GetAllCookies(GetCookieListCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // This function is being called to scrape the cookie list for management UI
   // or similar.  We shouldn't show expired cookies in this list since it will
   // just be confusing to users, and this function is called rarely enough (and
   // is already slow enough) that it's OK to take the time to garbage collect
   // the expired cookies now.
   //
   // Note that this does not prune cookies to be below our limits (if we've
   // exceeded them) the way that calling GarbageCollect() would.
   GarbageCollectExpired(
       Time::Now(), CookieMapItPair(cookies_.begin(), cookies_.end()), NULL);
 
   // Copy the CanonicalCookie pointers from the map so that we can use the same
   // sorter as elsewhere, then copy the result out.
   std::vector<CanonicalCookie*> cookie_ptrs;
   cookie_ptrs.reserve(cookies_.size());
   for (const auto& cookie : cookies_)
     cookie_ptrs.push_back(cookie.second.get());
   std::sort(cookie_ptrs.begin(), cookie_ptrs.end(), CookieSorter);
 
   CookieList cookie_list;
   cookie_list.reserve(cookie_ptrs.size());
   for (auto* cookie_ptr : cookie_ptrs)
     cookie_list.push_back(*cookie_ptr);
 
-  return cookie_list;
+  ConditionalCookieCallback(std::move(callback), cookie_list);
+  return;

[mmenke, 2017/07/12 18:29:04]

return not needed

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Oy vais.  Thank you.  Fixed through file.

 }
 
-CookieList CookieMonster::GetCookieListWithOptions(
-    const GURL& url,
-    const CookieOptions& options) {
+void CookieMonster::GetCookieListWithOptions(const GURL& url,
+                                             const CookieOptions& options,
+                                             GetCookieListCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CookieList cookies;
-  if (!HasCookieableScheme(url))
-    return cookies;
+  if (HasCookieableScheme(url)) {
+    std::vector<CanonicalCookie*> cookie_ptrs;
+    FindCookiesForHostAndDomain(url, options, &cookie_ptrs);
+    std::sort(cookie_ptrs.begin(), cookie_ptrs.end(), CookieSorter);
 
-  std::vector<CanonicalCookie*> cookie_ptrs;
-  FindCookiesForHostAndDomain(url, options, &cookie_ptrs);
-  std::sort(cookie_ptrs.begin(), cookie_ptrs.end(), CookieSorter);
-
-  cookies.reserve(cookie_ptrs.size());
-  for (std::vector<CanonicalCookie*>::const_iterator it = cookie_ptrs.begin();
-       it != cookie_ptrs.end(); it++)
-    cookies.push_back(**it);
-
-  return cookies;
+    cookies.reserve(cookie_ptrs.size());
+    for (std::vector<CanonicalCookie*>::const_iterator it = cookie_ptrs.begin();
+         it != cookie_ptrs.end(); it++)
+      cookies.push_back(**it);
+  }
+  ConditionalCookieCallback(std::move(callback), cookies);
+  return;

[mmenke, 2017/07/12 18:29:03]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:34]

Done.

 }
 
-uint32_t CookieMonster::DeleteAllCreatedBetween(const Time& delete_begin,
-                                                const Time& delete_end) {
+void CookieMonster::DeleteAllCreatedBetween(const Time& delete_begin,
+                                            const Time& delete_end,
+                                            DeleteCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   uint32_t num_deleted = 0;
   for (CookieMap::iterator it = cookies_.begin(); it != cookies_.end();) {
     CookieMap::iterator curit = it;
     CanonicalCookie* cc = curit->second.get();
     ++it;
 
     if (cc->CreationDate() >= delete_begin &&
         (delete_end.is_null() || cc->CreationDate() < delete_end)) {
       InternalDeleteCookie(curit, true, /*sync_to_store*/
                            DELETE_COOKIE_CREATED_BETWEEN);
       ++num_deleted;
     }
   }
 
-  return num_deleted;
+  FlushStore(base::BindOnce(
+      &ConditionalDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      callback.is_null() ? base::OnceClosure()
+                         : base::BindOnce(std::move(callback), num_deleted)));
+  return;

[mmenke, 2017/07/12 18:29:04]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

 }
 
-uint32_t CookieMonster::DeleteAllCreatedBetweenWithPredicate(
+void CookieMonster::DeleteAllCreatedBetweenWithPredicate(
     const base::Time& delete_begin,
     const base::Time& delete_end,
-    const base::Callback<bool(const CanonicalCookie&)>& predicate) {
+    const base::Callback<bool(const CanonicalCookie&)>& predicate,
+    DeleteCallback callback) {
   uint32_t num_deleted = 0;
   for (CookieMap::iterator it = cookies_.begin(); it != cookies_.end();) {
     CookieMap::iterator curit = it;
     CanonicalCookie* cc = curit->second.get();
     ++it;
 
     if (cc->CreationDate() >= delete_begin &&
         // The assumption that null |delete_end| is equivalent to
         // Time::Max() is confusing.
         (delete_end.is_null() || cc->CreationDate() < delete_end) &&
         predicate.Run(*cc)) {
       InternalDeleteCookie(curit, true, /*sync_to_store*/
                            DELETE_COOKIE_CREATED_BETWEEN_WITH_PREDICATE);
       ++num_deleted;
     }
   }
 
-  return num_deleted;
+  FlushStore(base::BindOnce(
+      &ConditionalDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      callback.is_null() ? base::OnceClosure()
+                         : base::BindOnce(std::move(callback), num_deleted)));
+  return;
 }
 
-bool CookieMonster::SetCookieWithOptions(const GURL& url,
+void CookieMonster::SetCookieWithOptions(const GURL& url,
                                          const std::string& cookie_line,
-                                         const CookieOptions& options) {
+                                         const CookieOptions& options,
+                                         SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!HasCookieableScheme(url)) {
-    return false;
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
   }
 
-  return SetCookieWithCreationTimeAndOptions(url, cookie_line, Time(), options);
+  SetCookieWithCreationTimeAndOptions(url, cookie_line, Time(), options,
+                                      std::move(callback));
+  return;

[mmenke, 2017/07/12 18:29:03]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

 }
 
-std::string CookieMonster::GetCookiesWithOptions(const GURL& url,
-                                                 const CookieOptions& options) {
+void CookieMonster::GetCookiesWithOptions(const GURL& url,
+                                          const CookieOptions& options,
+                                          GetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!HasCookieableScheme(url))
-    return std::string();
+  if (!HasCookieableScheme(url)) {
+    ConditionalCookieCallback(std::move(callback), std::string());
+    return;
+  }
 
   std::vector<CanonicalCookie*> cookies;
   FindCookiesForHostAndDomain(url, options, &cookies);
   std::sort(cookies.begin(), cookies.end(), CookieSorter);
 
   std::string cookie_line = BuildCookieLine(cookies);
 
   VLOG(kVlogGetCookies) << "GetCookies() result: " << cookie_line;
 
-  return cookie_line;
+  ConditionalCookieCallback(std::move(callback), cookie_line);

[mmenke, 2017/07/12 18:29:04]

Optional:  Could combine the paths like you did with GetCookieListWithOptions. 
I don't have strong feelings about it, but may be nice to be consistent.

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

+  return;

[mmenke, 2017/07/12 18:29:04]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

 }
 
 void CookieMonster::DeleteCookie(const GURL& url,
-                                 const std::string& cookie_name) {
+                                 const std::string& cookie_name,
+                                 base::OnceClosure callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!HasCookieableScheme(url))
+  if (!HasCookieableScheme(url)) {
+    // TODO(rdsmith): Would be good to provide a failure indication here.

[mmenke, 2017/07/12 18:29:04]

Could just make all delete callbacks report number of deleted callbacks.  Then
we'd have one fewer callback class, too.

[Randy Smith (Not in Mondays), 2017/07/12 19:47:34]

Perfectly reasonable suggestion, but not in this CL (I want to keep this a pure
refactor) and hence probably not soon (I want to get back to moving forward on
servicification).

[mmenke, 2017/07/12 20:00:11]

Sure, was not suggesting it for this CL.  :)

+    ConditionalCookieCallback(std::move(callback));
     return;
+  }
 
   CookieOptions options;
   options.set_include_httponly();
   options.set_same_site_cookie_mode(
       CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
   // Get the cookies for this host and its domain(s).
   std::vector<CanonicalCookie*> cookies;
   FindCookiesForHostAndDomain(url, options, &cookies);
   std::set<CanonicalCookie*> matching_cookies;
 
   for (auto* cookie : cookies) {
     if (cookie->Name() != cookie_name)
       continue;
     if (!cookie->IsOnPath(url.path()))
       continue;
     matching_cookies.insert(cookie);
   }
 
   for (CookieMap::iterator it = cookies_.begin(); it != cookies_.end();) {
     CookieMap::iterator curit = it;
     ++it;
     if (matching_cookies.find(curit->second.get()) != matching_cookies.end()) {
       InternalDeleteCookie(curit, true, DELETE_COOKIE_SINGLE);
     }
   }
+
+  FlushStore(base::BindOnce(&ConditionalDeleteCallback,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            std::move(callback)));

[mmenke, 2017/07/12 18:29:03]

The next method has a null callback check, but this one does not.  Is there a
reason for that?  ConditionalDeleteCallback also has its own null check.  In
general, I feel we should either consistently have null checks here, and not
have one in ConditionalDeleteCallback, or vice versa.

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

As per our offline conversation, put in a comment as to why this was different
than the surrounding code.

 }
 
-uint32_t CookieMonster::DeleteCanonicalCookie(const CanonicalCookie& cookie) {
+void CookieMonster::DeleteCanonicalCookie(const CanonicalCookie& cookie,
+                                          DeleteCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  uint32_t result = 0u;
   for (CookieMapItPair its = cookies_.equal_range(GetKey(cookie.Domain()));
        its.first != its.second; ++its.first) {
     // The creation date acts as the unique index...
     if (its.first->second->CreationDate() == cookie.CreationDate()) {
       InternalDeleteCookie(its.first, true, DELETE_COOKIE_CANONICAL);
-      return 1u;
+      result = 1u;
+      break;
     }
   }
-  return 0u;
+  FlushStore(base::BindOnce(
+      &ConditionalDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      callback.is_null() ? base::OnceClosure()
+                         : base::BindOnce(std::move(callback), result)));
 }
 
-bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
-                                              const std::string& cookie_line,
-                                              const base::Time& creation_time) {
+void CookieMonster::SetCookieWithCreationTimeForTesting(
+    const GURL& url,
+    const std::string& cookie_line,
+    const base::Time& creation_time,
+    SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!store_.get()) << "This method is only to be used by unit-tests.";
 
   if (!HasCookieableScheme(url)) {
-    return false;
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
   }
 
   MarkCookieStoreAsInitialized();
   if (ShouldFetchAllCookiesWhenFetchingAnyCookie())
     FetchAllCookiesIfNecessary();
 
-  return SetCookieWithCreationTimeAndOptions(url, cookie_line, creation_time,
-                                             CookieOptions());
+  return SetCookieWithCreationTimeAndOptions(
+      url, cookie_line, creation_time, CookieOptions(), std::move(callback));
 }
 
-uint32_t CookieMonster::DeleteSessionCookies() {
+void CookieMonster::DeleteSessionCookies(DeleteCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   uint32_t num_deleted = 0;
   for (CookieMap::iterator it = cookies_.begin(); it != cookies_.end();) {
     CookieMap::iterator curit = it;
     CanonicalCookie* cc = curit->second.get();
     ++it;
 
     if (!cc->IsPersistent()) {
       InternalDeleteCookie(curit, true, /*sync_to_store*/
                            DELETE_COOKIE_EXPIRED);
       ++num_deleted;
     }
   }
 
-  return num_deleted;
+  FlushStore(base::BindOnce(
+      &ConditionalDeleteCallback, weak_ptr_factory_.GetWeakPtr(),
+      callback.is_null() ? base::OnceClosure()
+                         : base::BindOnce(std::move(callback), num_deleted)));
+  return;

[mmenke, 2017/07/12 18:29:04]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:34]

Done.

 }
 
 void CookieMonster::MarkCookieStoreAsInitialized() {
   DCHECK(thread_checker_.CalledOnValidThread());
   initialized_ = true;
 }
 
 void CookieMonster::FetchAllCookiesIfNecessary() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (store_.get() && !started_fetching_all_cookies_) {
@@ skipping 54 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 225 matching lines @@
                             : 0;
   type_sample |= cc_ptr->IsHttpOnly() ? 1 << COOKIE_TYPE_HTTPONLY : 0;
   type_sample |= cc_ptr->IsSecure() ? 1 << COOKIE_TYPE_SECURE : 0;
   histogram_cookie_type_->Add(type_sample);
 
   RunCookieChangedCallbacks(*cc_ptr, CookieStore::ChangeCause::INSERTED);
 
   return inserted;
 }
 
-bool CookieMonster::SetCookieWithCreationTimeAndOptions(
+void CookieMonster::SetCookieWithCreationTimeAndOptions(
     const GURL& url,
     const std::string& cookie_line,
     const Time& creation_time_or_null,
-    const CookieOptions& options) {
+    const CookieOptions& options,
+    SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   VLOG(kVlogSetCookies) << "SetCookie() line: " << cookie_line;
 
   Time creation_time = creation_time_or_null;
   if (creation_time.is_null()) {
     creation_time = CurrentTime();
     last_time_seen_ = creation_time;
   }
 
   std::unique_ptr<CanonicalCookie> cc(
       CanonicalCookie::Create(url, cookie_line, creation_time, options));
 
   if (!cc.get()) {
     VLOG(kVlogSetCookies) << "WARNING: Failed to allocate CanonicalCookie";
-    return false;
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
   }
-  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(),
-                            !options.exclude_httponly());
+  SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(),
+                     !options.exclude_httponly(), std::move(callback));
 }
 
-bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
+void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
                                        bool secure_source,
-                                       bool modify_http_only) {
+                                       bool modify_http_only,
+                                       SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (cc->IsSecure() && !secure_source)
-    return false;
+  if (cc->IsSecure() && !secure_source) {
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
+  }
 
   const std::string key(GetKey(cc->Domain()));
 
   // TODO(mmenke): This class assumes each cookie to have a unique creation
   // time. Allowing the caller to set the creation time violates that
   // assumption. Worth fixing? Worth noting that time changes between browser
   // restarts can cause the same issue.
   base::Time creation_date = cc->CreationDate();
   if (creation_date.is_null()) {
     creation_date = CurrentTime();
     cc->SetCreationDate(creation_date);
     last_time_seen_ = creation_date;
   }
   bool already_expired = cc->IsExpired(creation_date);
 
   if (DeleteAnyEquivalentCookie(key, *cc, secure_source, !modify_http_only,
                                 already_expired)) {
     std::string error;
     error =
         "SetCookie() not clobbering httponly cookie or secure cookie for "
         "insecure scheme";
 
     VLOG(kVlogSetCookies) << error;
-    return false;
+    ConditionalCookieCallback(std::move(callback), false);
+    return;
   }
 
   VLOG(kVlogSetCookies) << "SetCookie() key: " << key
                         << " cc: " << cc->DebugString();
 
   // Realize that we might be setting an expired cookie, and the only point
   // was to delete the cookie which we've already done.
   if (!already_expired) {
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
@@ skipping 21 matching lines @@
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
 
   // 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;
+  ConditionalCookieCallback(std::move(callback), true);
 }
 
-bool CookieMonster::SetAllCookies(const CookieList& list) {
+void CookieMonster::SetAllCookies(CookieList list,
+                                  SetCookiesCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  CookieList positive_diff;
+  CookieList negative_diff;
+
+  CookieList old_cookies;
+  old_cookies.reserve(cookies_.size());
+  for (const auto& cookie : cookies_)
+    old_cookies.push_back(*cookie.second.get());
+
+  ComputeCookieDiff(&old_cookies, &list, &positive_diff, &negative_diff);
+
+  for (const auto& cookie_to_delete : negative_diff) {
+    for (CookieMapItPair its =
+             cookies_.equal_range(GetKey(cookie_to_delete.Domain()));
+         its.first != its.second; ++its.first) {
+      // The creation date acts as the unique index...
+      if (its.first->second->CreationDate() ==
+          cookie_to_delete.CreationDate()) {
+        // TODO(rdsmith): DELETE_COOKIE_CANONICAL is incorrect and should
+        // be changed.
+        InternalDeleteCookie(its.first, true, DELETE_COOKIE_CANONICAL);
+        break;
+      }
+    }
+  }
+
+  if (positive_diff.size() == 0) {

[mmenke, 2017/07/12 18:29:03]

This is old code, and I'm not asking you to fix it, but this seems weird:

If positive_diff is 0, we do nothing, but if it's not, we re-add all cookies,
including those already in the list. It seems like we could just add only the
cookies that were not in the store before.  Or is the issue garbage collecting
non-expired cookies?  In that case, it seems we should be unconditionally
re-adding them to update their last access date.

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

As noted in the review request message, this function is a pile of crap, but as
you note, it is a correctly refactored pile of crap.  I intended to fix its
problems (with a small tactical nuke) in
https://codereview.chromium.org/2974363002, but I pushed the functional changes
out to that CL.  Consider the two CLs in combination and let me know if you want
me to shift code from one to the other, or do something differently at the high
level?

+    ConditionalCookieCallback(std::move(callback), true);
+    return;
+  }
+
   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)
       continue;
 
     if (cookie.IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
           (cookie.ExpiryDate() - creation_time).InMinutes());
     }
 
     InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true);
     GarbageCollect(creation_time, key);
   }
 
   // TODO(rdsmith): If this function always returns the same value, it
   // shouldn't have a return value.  But it should also be deleted (see
   // https://codereview.chromium.org/2882063002/#msg64), which would
   // solve the return value problem.
-  return true;
+  ConditionalCookieCallback(std::move(callback), true);
+  return;

[mmenke, 2017/07/12 18:29:04]

-return

[Randy Smith (Not in Mondays), 2017/07/12 19:47:34]

Done.

 }
 
 void CookieMonster::InternalUpdateCookieAccessTime(CanonicalCookie* cc,
                                                    const Time& current) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Based off the Mozilla code.  When a cookie has been accessed recently,
   // don't bother updating its access time again.  This reduces the number of
   // updates we do during pageload, which in turn reduces the chance our storage
   // backend will hit its batch thresholds and be forced to update.
@@ skipping 442 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();
+  ConditionalCookieCallback(std::move(callback));

[mmenke, 2017/07/12 18:29:03]

I don't think these can be nullptr?

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Ah, good point.  Done.

 }
 
-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();
+  ConditionalCookieCallback(std::move(callback));

[mmenke, 2017/07/12 18:29:04]

ditto

[Randy Smith (Not in Mondays), 2017/07/12 19:47:35]

Done.

 }
 
 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