Add a SetCanonicalCookie method for CookieMonster.

net/cookies/cookie_monster.cc

Index: net/cookies/cookie_monster.cc
diff --git a/net/cookies/cookie_monster.cc b/net/cookies/cookie_monster.cc
index 1e5430a5e83d946093ead3bce927ccb6d27dff10..449ee0c7b34fc9a4d9210e598c48f45721b4e62b 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -62,6 +62,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"

[mmenke, 2017/06/16 15:56:28]

nit:  Not needed, already included in header.

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

Done.

 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_util.h"
@@ -694,6 +695,42 @@ int 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,
+                         const SetCookiesCallback& callback)
+      : CookieMonsterTask(cookie_monster),
+        cookie_(std::move(cookie)),
+        secure_source_(secure_source),
+        modify_http_only_(modify_http_only),
+        callback_(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())
+    callback_.Run(result);
+}
+
 // Task class for SetCookieWithOptions call.
 class CookieMonster::SetCookieWithOptionsTask : public CookieMonsterTask {
  public:
@@ -902,6 +939,23 @@ void CookieMonster::SetAllCookiesAsync(const CookieList& list,
   DoCookieTask(task);
 }
 
+void CookieMonster::SetCanonicalCookieAsync(
+    const CanonicalCookie& cookie,
+    bool secure_source,
+    bool modify_http_only,
+    const SetCookiesCallback& callback) {
+  DCHECK(cookie.IsCanonical());
+  scoped_refptr<SetCanonicalCookieTask> task = new SetCanonicalCookieTask(
+      this, base::MakeUnique<CanonicalCookie>(cookie), secure_source,

[mmenke, 2017/06/16 15:56:28]

Should this take a unique_ptr<CanonicalCookie> instead?  Does anyone need to
have their cookie and eat it too (Or at least feed it to the monster, too)?

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

I think I'm going to say "Yeah, that makes sense", though it pushes a problem
off until the next CL.  This interface is being created for use by the Mojo
interface, and I don't think I can get mojo to give me a std::unique_ptr<>, so
I'm just going to have to copy it at that level.  But maybe I can find a way
around that when I'm focussing on the mojo CL, and I don't think there's any
gain even if I can't in doing the copy here instead of there.  Done.

+      modify_http_only, callback);
+
+  // TODO(rdsmith): Switch to DoCookieTaskForURL (or the equivalent).
+  // This is tricky because we don't have the scheme in this routine
+  // and DoCookieTaskForURL uses cookie_util::GetEffectiveDomain(scheme, host)
+  // to generate the database key to block behind.
+  DoCookieTask(task);
+}
+
 void CookieMonster::SetCookieWithOptionsAsync(
     const GURL& url,
     const std::string& cookie_line,
@@ -1068,16 +1122,6 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
   if (!HasCookieableScheme(url))
     return false;
 
-  // 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 actual_creation_time = creation_time;
-  if (creation_time.is_null()) {
-    actual_creation_time = CurrentTime();
-    last_time_seen_ = actual_creation_time;
-  }
-
   // Validate consistency of passed arguments.
   if (ParsedCookie::ParseTokenString(name) != name ||
       ParsedCookie::ParseValueString(value) != value ||
@@ -1104,9 +1148,8 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
                             canon_path_component.len);
 
   std::unique_ptr<CanonicalCookie> cc(base::MakeUnique<CanonicalCookie>(
-      name, value, cookie_domain, cookie_path, actual_creation_time,
-      expiration_time, last_access_time, secure, http_only, same_site,
-      priority));
+      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);
 }
@@ -1766,9 +1809,19 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   if (cc->IsSecure() && !secure_source)
     return false;
 
-  Time creation_time = cc->CreationDate();
   const std::string key(GetKey(cc->Domain()));
-  bool already_expired = cc->IsExpired(creation_time);
+
+  // 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)) {
@@ -1790,7 +1843,7 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
-          (cc->ExpiryDate() - creation_time).InMinutes());
+          (cc->ExpiryDate() - creation_date).InMinutes());
     }
 
     // Histogram the type of scheme used on URLs that set cookies. This
@@ -1818,7 +1871,7 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   // 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_time, key);
+  GarbageCollect(creation_date, key);

[mmenke, 2017/06/16 15:56:28]

So we just accept a creation date from the consumer, and we garbage collect,
just assuming it's the current time?  That seems not great.

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

Good point, but I think it's currently existing behavior: SetCookieWithDetails
-> SetCanonicalCookie -> GarbageCollect()?

 
   return true;
 }