Access-time threshold unit failure in CookieMonster.

net/cookies/cookie_monster.h

 // 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.
 
 // Brought to you by the letter D and the number 2.
 
 #ifndef NET_COOKIES_COOKIE_MONSTER_H_
 #define NET_COOKIES_COOKIE_MONSTER_H_
 
 #include <stddef.h>
@@ skipping 18 matching lines @@
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_constants.h"
 #include "net/cookies/cookie_store.h"
 #include "url/gurl.h"
 
 namespace base {
 class Histogram;
 class HistogramBase;
 class TimeTicks;

[Scott Hess - ex-Googler, 2016/05/18 23:14:29]

In verifying that a straight base::TimeDelta should be fine, I noticed that this
extra forward declaration is redundant and also not necessary.

And then I noticed that Histogram isn't necessary, either.

 }  // namespace base
 
 namespace net {
 
 class CookieMonsterDelegate;
 class ParsedCookie;

[Scott Hess - ex-Googler, 2016/05/18 23:14:29]

Once looking, you notice other things which aren't used, either.  I might stop
asking for trouble, now.

 
 // The cookie monster is the system for storing and retrieving cookies. It has
 // an in-memory list of all cookies, and synchronizes non-session cookies to an
 // optional permanent storage that implements the PersistentCookieStore
 // interface.
 //
 // Tasks may be deferred if all affected cookies are not yet loaded from the
 // backing store. Otherwise, callbacks may be invoked immediately.
 //
 // A cookie task is either pending loading of the entire cookie store, or
@@ skipping 77 matching lines @@
   // class will take care of initializing it. The backing store is NOT owned by
   // this class, but it must remain valid for the duration of the cookie
   // monster's existence. If |store| is NULL, then no backing store will be
   // updated. If |delegate| is non-NULL, it will be notified on
   // creation/deletion of cookies.
   CookieMonster(PersistentCookieStore* store, CookieMonsterDelegate* delegate);
 
   // Only used during unit testing.
   CookieMonster(PersistentCookieStore* store,
                 CookieMonsterDelegate* delegate,
-                int last_access_threshold_milliseconds);
+                const base::TimeDelta& last_access_threshold);

[Ryan Hamilton, 2016/05/18 22:45:20]

nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by value
instead of by const-ref seems better.  

[Ryan Sleevi, 2016/05/18 23:07:07]

The fact that it had to be clarified "(under the covers)" gives a hint about why
I might disagree. In any event, either approach is consistent with this file,
which is itself inconsistent (although the const-ref is far, far more consistent
in the wider codebase)

[Scott Hess - ex-Googler, 2016/05/18 23:14:29]

Done.

[Scott Hess - ex-Googler, 2016/05/18 23:14:29]

My of const& was _mostly_ because I assumed that the class was forward-declared,
and thus you wouldn't be able to pass by value.  I'll leave if const-value.

[Ryan Hamilton, 2016/05/18 23:46:16]

Hm. I'm surprised to hear you say this, because in the past you proposed using a
vector<string> and doing a binary search instead of (what?) a hash_set<string>
because of short string optimization (which requires knowledge of the under-the
hood representation of string). How would you characterize the difference
between these two situations? There's probably a guiding principle here that
I've not thought about.

That being said, +1 for consistency. I only looked at this CL 'cause shess G+'d
about it and the const& just caught my eye, but I didn't actually look at the
surrounding code :>

 
   ~CookieMonster() override;
 
   // Replaces all the cookies by |list|. This method does not flush the backend.
   void SetAllCookiesAsync(const CookieList& list,
                           const SetCookiesCallback& callback);
 
   // CookieStore implementation.
   void SetCookieWithOptionsAsync(const GURL& url,
                                  const std::string& cookie_line,
@@ skipping 648 matching lines @@
   virtual ~PersistentCookieStore() {}
 
  private:
   friend class base::RefCountedThreadSafe<PersistentCookieStore>;
   DISALLOW_COPY_AND_ASSIGN(PersistentCookieStore);
 };
 
 }  // namespace net
 
 #endif  // NET_COOKIES_COOKIE_MONSTER_H_