Extend CookieOptions to include updating last accessed time.

We have too many methods of accessing cookies hanging off of CookieStore
(and CookieMonster). This patch untangles some of the behavioral
differences between the methods by moving the decision about whether or
not a "get" operation updates the last-accessed time to CookieOptions.
This allows us to more cleanly implement the backend of the 'GetAll*'
methods, with the goal of getting rid of them entirely.

BUG=588081
R=rdsmith@chromium.org,mmenke@chromium.org

Review URL: https://codereview.chromium.org/1713013002

Cr-Commit-Position: refs/heads/master@{#376780}

net/cookies/cookie_monster.cc

@@ -522,11 +522,11 @@ void CookieMonster::GetAllCookiesTask::Run() {
   }
 }
 
-// Task class for GetAllCookiesForURLWithOptions call.
-class CookieMonster::GetAllCookiesForURLWithOptionsTask
+// Task class for GetCookieListForURLWithOptionsAsync call.
+class CookieMonster::GetCookieListForURLWithOptionsTask
     : public CookieMonsterTask {
  public:
-  GetAllCookiesForURLWithOptionsTask(CookieMonster* cookie_monster,
+  GetCookieListForURLWithOptionsTask(CookieMonster* cookie_monster,
                                      const GURL& url,
                                      const CookieOptions& options,
                                      const GetCookieListCallback& callback)
@@ -539,20 +539,20 @@ class CookieMonster::GetAllCookiesForURLWithOptionsTask
   void Run() override;
 
  protected:
-  ~GetAllCookiesForURLWithOptionsTask() override {}
+  ~GetCookieListForURLWithOptionsTask() override {}
 
  private:
   GURL url_;
   CookieOptions options_;
   GetCookieListCallback callback_;
 
-  DISALLOW_COPY_AND_ASSIGN(GetAllCookiesForURLWithOptionsTask);
+  DISALLOW_COPY_AND_ASSIGN(GetCookieListForURLWithOptionsTask);
 };
 
-void CookieMonster::GetAllCookiesForURLWithOptionsTask::Run() {
+void CookieMonster::GetCookieListForURLWithOptionsTask::Run() {
   if (!callback_.is_null()) {
     CookieList cookies =
-        this->cookie_monster()->GetAllCookiesForURLWithOptions(url_, options_);
+        this->cookie_monster()->GetCookieListForURLWithOptions(url_, options_);
     this->InvokeCallback(base::Bind(&GetCookieListCallback::Run,
                                     base::Unretained(&callback_), cookies));
   }
@@ -905,12 +905,12 @@ void CookieMonster::SetCookieWithDetailsAsync(
   DoCookieTaskForURL(task, url);
 }
 
-void CookieMonster::GetAllCookiesForURLWithOptionsAsync(
+void CookieMonster::GetCookieListForURLWithOptionsAsync(
     const GURL& url,
     const CookieOptions& options,
     const GetCookieListCallback& callback) {
-  scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
-      new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
+  scoped_refptr<GetCookieListForURLWithOptionsTask> task =
+      new GetCookieListForURLWithOptionsTask(this, url, options, callback);
 
   DoCookieTaskForURL(task, url);
 }
@@ -962,8 +962,9 @@ void CookieMonster::GetAllCookiesForURLAsync(
   CookieOptions options;
   options.set_include_httponly();
   options.set_include_same_site();
-  scoped_refptr<GetAllCookiesForURLWithOptionsTask> task =
-      new GetAllCookiesForURLWithOptionsTask(this, url, options, callback);
+  options.set_do_not_update_access_time();
+  scoped_refptr<GetCookieListForURLWithOptionsTask> task =
+      new GetCookieListForURLWithOptionsTask(this, url, options, callback);
 
   DoCookieTaskForURL(task, url);
 }
@@ -1159,13 +1160,13 @@ CookieList CookieMonster::GetAllCookies() {
   return cookie_list;
 }
 
-CookieList CookieMonster::GetAllCookiesForURLWithOptions(
+CookieList CookieMonster::GetCookieListForURLWithOptions(
     const GURL& url,
     const CookieOptions& options) {
   base::AutoLock autolock(lock_);
 
   std::vector<CanonicalCookie*> cookie_ptrs;
-  FindCookiesForHostAndDomain(url, options, false, &cookie_ptrs);
+  FindCookiesForHostAndDomain(url, options, &cookie_ptrs);
   std::sort(cookie_ptrs.begin(), cookie_ptrs.end(), CookieSorter);
 
   CookieList cookies;
@@ -1254,7 +1255,7 @@ std::string CookieMonster::GetCookiesWithOptions(const GURL& url,
     return std::string();
 
   std::vector<CanonicalCookie*> cookies;
-  FindCookiesForHostAndDomain(url, options, true, &cookies);
+  FindCookiesForHostAndDomain(url, options, &cookies);
   std::sort(cookies.begin(), cookies.end(), CookieSorter);
 
   std::string cookie_line = BuildCookieLine(cookies);
@@ -1276,7 +1277,7 @@ void CookieMonster::DeleteCookie(const GURL& url,
   options.set_include_same_site();
   // Get the cookies for this host and its domain(s).
   std::vector<CanonicalCookie*> cookies;
-  FindCookiesForHostAndDomain(url, options, true, &cookies);
+  FindCookiesForHostAndDomain(url, options, &cookies);
   std::set<CanonicalCookie*> matching_cookies;
 
   for (std::vector<CanonicalCookie*>::const_iterator it = cookies.begin();
@@ -1607,7 +1608,6 @@ void CookieMonster::TrimDuplicateCookiesForKey(const std::string& key,
 void CookieMonster::FindCookiesForHostAndDomain(
     const GURL& url,
     const CookieOptions& options,
-    bool update_access_time,
     std::vector<CanonicalCookie*>* cookies) {
   lock_.AssertAcquired();
 
@@ -1620,15 +1620,13 @@ void CookieMonster::FindCookiesForHostAndDomain(
 
   // Can just dispatch to FindCookiesForKey
   const std::string key(GetKey(url.host()));
-  FindCookiesForKey(key, url, options, current_time, update_access_time,
-                    cookies);
+  FindCookiesForKey(key, url, options, current_time, cookies);
 }
 
 void CookieMonster::FindCookiesForKey(const std::string& key,
                                       const GURL& url,
                                       const CookieOptions& options,
                                       const Time& current,
-                                      bool update_access_time,
                                       std::vector<CanonicalCookie*>* cookies) {
   lock_.AssertAcquired();
 
@@ -1652,7 +1650,7 @@ void CookieMonster::FindCookiesForKey(const std::string& key,
 
     // Add this cookie to the set of matching cookies. Update the access
     // time if we've been requested to do so.
-    if (update_access_time) {
+    if (options.update_access_time()) {
       InternalUpdateCookieAccessTime(cc, current);
     }
     cookies->push_back(cc);

net/cookies/cookie_monster.h

@@ -153,7 +153,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   // mark the cookies as having been accessed.
   // The returned cookies are ordered by longest path, then earliest
   // creation date.
-  void GetAllCookiesForURLWithOptionsAsync(
+  void GetCookieListForURLWithOptionsAsync(
       const GURL& url,
       const CookieOptions& options,
       const GetCookieListCallback& callback);
@@ -238,7 +238,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   class DeleteAllCreatedBetweenForHostTask;
   class DeleteCookieTask;
   class DeleteCanonicalCookieTask;
-  class GetAllCookiesForURLWithOptionsTask;
+  class GetCookieListForURLWithOptionsTask;
   class GetAllCookiesTask;
   class GetCookiesWithOptionsTask;
   class SetAllCookiesTask;
@@ -418,7 +418,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
 
   CookieList GetAllCookies();
 
-  CookieList GetAllCookiesForURLWithOptions(const GURL& url,
+  CookieList GetCookieListForURLWithOptions(const GURL& url,
                                             const CookieOptions& options);
 
   int DeleteAllCreatedBetween(const base::Time& delete_begin,
@@ -496,14 +496,12 @@ class NET_EXPORT CookieMonster : public CookieStore {
 
   void FindCookiesForHostAndDomain(const GURL& url,
                                    const CookieOptions& options,
-                                   bool update_access_time,
                                    std::vector<CanonicalCookie*>* cookies);
 
   void FindCookiesForKey(const std::string& key,
                          const GURL& url,
                          const CookieOptions& options,
                          const base::Time& current,
-                         bool update_access_time,
                          std::vector<CanonicalCookie*>* cookies);
 
   // Delete any cookies that are equivalent to |ecc| (same path, domain, etc).

net/cookies/cookie_monster_unittest.cc

@@ -126,7 +126,7 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
                                             const CookieOptions& options) {
     DCHECK(cm);
     GetCookieListCallback callback;
-    cm->GetAllCookiesForURLWithOptionsAsync(
+    cm->GetCookieListForURLWithOptionsAsync(
         url, options,
         base::Bind(&GetCookieListCallback::Run, base::Unretained(&callback)));
     callback.WaitUntilDone();
@@ -826,8 +826,8 @@ ACTION_P2(DeleteAllAction, cookie_monster, callback) {
   cookie_monster->DeleteAllAsync(callback->AsCallback());
 }
 
-ACTION_P3(GetAllCookiesForUrlWithOptionsAction, cookie_monster, url, callback) {
-  cookie_monster->GetAllCookiesForURLWithOptionsAsync(url, CookieOptions(),
+ACTION_P3(GetCookieListForUrlWithOptionsAction, cookie_monster, url, callback) {
+  cookie_monster->GetCookieListForURLWithOptionsAsync(url, CookieOptions(),
                                                       callback->AsCallback());
 }
 
@@ -1137,14 +1137,14 @@ TEST_F(DeferredCookieTaskTest, DeferredGetAllForUrlWithOptionsCookies) {
   MockGetCookieListCallback get_cookie_list_callback;
 
   BeginWithForDomainKey(http_www_google_.domain(),
-                        GetAllCookiesForUrlWithOptionsAction(
+                        GetCookieListForUrlWithOptionsAction(
                             &cookie_monster(), http_www_google_.url(),
                             &get_cookie_list_callback));
 
   WaitForLoadCall();
 
   EXPECT_CALL(get_cookie_list_callback, Invoke(testing::_))
-      .WillOnce(GetAllCookiesForUrlWithOptionsAction(
+      .WillOnce(GetCookieListForUrlWithOptionsAction(
           &cookie_monster(), http_www_google_.url(),
           &get_cookie_list_callback));
   base::RunLoop loop;
@@ -1372,12 +1372,33 @@ TEST_F(CookieMonsterTest, TestLastAccess) {
   // Reading the cookie again immediately shouldn't update the access date,
   // since we're inside the threshold.
   EXPECT_EQ("A=B", GetCookies(cm.get(), http_www_google_.url()));
-  EXPECT_TRUE(last_access_date == GetFirstCookieAccessDate(cm.get()));
+  EXPECT_EQ(last_access_date, GetFirstCookieAccessDate(cm.get()));
 
-  // Reading after a short wait should update the access date.
+  // Reading after a short wait will update the access date, if the cookie
+  // is requested with options that would update the access date. First, test
+  // that the flag's behavior is respected.
   base::PlatformThread::Sleep(
       base::TimeDelta::FromMilliseconds(kAccessDelayMs));
-  EXPECT_EQ("A=B", GetCookies(cm.get(), http_www_google_.url()));
+  CookieOptions options;
+  options.set_do_not_update_access_time();
+  EXPECT_EQ("A=B",
+            GetCookiesWithOptions(cm.get(), http_www_google_.url(), options));
+  EXPECT_EQ(last_access_date, GetFirstCookieAccessDate(cm.get()));
+
+  // Getting all cookies for a URL doesn't update the accessed time either.
+  CookieList cookies = GetAllCookiesForURL(cm.get(), http_www_google_.url());
+  CookieList::iterator it = cookies.begin();
+  ASSERT_TRUE(it != cookies.end());
+  EXPECT_EQ(http_www_google_.host(), it->Domain());
+  EXPECT_EQ("A", it->Name());
+  EXPECT_EQ("B", it->Value());
+  EXPECT_EQ(last_access_date, GetFirstCookieAccessDate(cm.get()));
+  EXPECT_TRUE(++it == cookies.end());
+
+  // If the flag isn't set, the last accessed time should be updated.
+  options = CookieOptions();
+  EXPECT_EQ("A=B",
+            GetCookiesWithOptions(cm.get(), http_www_google_.url(), options));
   EXPECT_FALSE(last_access_date == GetFirstCookieAccessDate(cm.get()));
 }
 
@@ -1481,7 +1502,7 @@ TEST_F(CookieMonsterTest, GetAllCookiesForURL) {
   ASSERT_TRUE(++it == cookies.end());
 
   // Reading after a short wait should not update the access date.
-  EXPECT_TRUE(last_access_date == GetFirstCookieAccessDate(cm.get()));
+  EXPECT_EQ(last_access_date, GetFirstCookieAccessDate(cm.get()));
 }
 
 TEST_F(CookieMonsterTest, GetAllCookiesForURLPathMatching) {
@@ -2471,7 +2492,7 @@ class MultiThreadedCookieMonsterTest : public CookieMonsterTest {
                                           const GURL& url,
                                           const CookieOptions& options,
                                           GetCookieListCallback* callback) {
-    cm->GetAllCookiesForURLWithOptionsAsync(
+    cm->GetCookieListForURLWithOptionsAsync(
         url, options,
         base::Bind(&GetCookieListCallback::Run, base::Unretained(callback)));
   }

net/cookies/cookie_options.cc

@@ -12,6 +12,7 @@ CookieOptions::CookieOptions()
     : exclude_httponly_(true),
       include_same_site_(false),
       enforce_strict_secure_(false),
+      update_access_time_(true),
       server_time_() {}
 
 }  // namespace net

net/cookies/cookie_options.h

@@ -20,12 +20,14 @@ class NET_EXPORT CookieOptions {
   // * Excludes HttpOnly cookies
   // * Excludes SameSite cookies
   // * Does not enforce prefix restrictions (e.g. "$Secure-*")
+  // * Updates last-accessed time.
   //
   // These settings can be altered by calling:
   //
   // * |set_{include,exclude}_httponly()|
   // * |set_include_same_site()|
   // * |set_enforce_prefixes()|
+  // * |set_do_not_update_access_time()|
   CookieOptions();
 
   void set_exclude_httponly() { exclude_httponly_ = true; }
@@ -50,10 +52,14 @@ class NET_EXPORT CookieOptions {
   bool has_server_time() const { return !server_time_.is_null(); }
   base::Time server_time() const { return server_time_; }
 
+  void set_do_not_update_access_time() { update_access_time_ = false; }
+  bool update_access_time() const { return update_access_time_; }
+
  private:
   bool exclude_httponly_;
   bool include_same_site_;
   bool enforce_strict_secure_;
+  bool update_access_time_;
   base::Time server_time_;
 };
 

net/cookies/cookie_store.h

@@ -98,8 +98,9 @@ class NET_EXPORT CookieStore : public base::RefCountedThreadSafe<CookieStore> {
       const CookieOptions& options,
       const GetCookiesCallback& callback) = 0;
 
-  // Invokes GetAllCookiesForURLWithOptions with options set to include HTTP
-  // only cookies.
+  // Returns all cookies associated with |url|, including http-only, and
+  // same-site cookies. The returned cookies are ordered by longest path, then
+  // by earliest creation date, and are not marked as having been accessed.
   virtual void GetAllCookiesForURLAsync(
       const GURL& url,
       const GetCookieListCallback& callback) = 0;