Implementing EvictedDomainCookieCounter to keep user metrics on cookie eviction and reinstatement.

chrome/browser/net/evicted_domain_cookie_counter.h

+// Copyright 2013 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.
+
+#ifndef CHROME_BROWSER_NET_EVICTED_DOMAIN_COOKIE_COUNTER_H_
+#define CHROME_BROWSER_NET_EVICTED_DOMAIN_COOKIE_COUNTER_H_
+
+#include <map>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "net/cookies/cookie_monster.h"
+
+namespace net {
+class CanonicalCookie;
+}  // namespace net
+
+namespace chrome_browser_net {
+
+// The Evicted Domain Cookie Counter generates statistics on "wrongly evicted"
+// cookies, i.e., cookies that were "evicted" (on reaching domain cookie limit)
+// but are then "reinstated" later by a website because they were important.
+// Here is a specific scenario: a long-lived login session cookie gets evicted
+// due to its age, thereby forcing the user to lose session, and is reinstated
+// when the annoyed user reauthenticates.
+//
+// A solution to the above problem is the Cookie Priority Field, which gives
+// server a way to protect important cookies, thereby decreasing the chance
+// that these cookies are wrongly evicted. To measure the effectiveness of this
+// solution, we record eviction statistics before vs. after the fix.
+//
+// Specifically, we wish to record statistics on "reinstatement delay", i.e.,
+// the duration between eviction and reinstatement of cookie. We expect that
+// after the fix, average reinstantement delay will increase, since low priority
+// cookies are less likely to be reinstated after eviction.
+//
+// Statistics for Google domains are tracked separately.
+//
+class EvictedDomainCookieCounter : public net::CookieMonster::Delegate {
+ public:
+  // Structure to store sanitized data from CanonicalCookie.
+  struct EvictedCookie {
+    base::Time eviction_time_;
+    base::Time expiry_time_;
+    bool is_google_;

[mmenke, 2013/03/29 15:22:16]

Per Google style guide, members should go at end of struct.

[mmenke, 2013/03/29 15:22:16]

Per Google style guide,, struct members should not end in semi-colons.

[mmenke, 2013/03/29 15:23:04]

Underscores, rather

[huangs, 2013/04/02 19:21:52]

Done.

[huangs, 2013/04/02 19:21:52]

Done.

+
+    EvictedCookie(base::Time eviction_time,
+                  base::Time expiry_time,
+                  bool is_google)
+        : eviction_time_(eviction_time),
+          expiry_time_(expiry_time),
+          is_google_(is_google) {}
+
+    bool is_expired(const base::Time& current) {

[mmenke, 2013/03/29 15:22:16]

nit:  const

[huangs, 2013/04/02 19:21:52]

Done.

+      return !expiry_time_.is_null() && current >= expiry_time_;
+    }
+  };
+
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called when a stored evicted cookie is reinstated.
+    virtual void Report(const EvictedCookie& dc,
+                        const base::Time& reinstatement_time) = 0;
+
+    // Getter of time is placed here to enable mocks.
+    virtual base::Time CurrentTime() = 0;

[mmenke, 2013/03/29 15:22:16]

const?

[huangs, 2013/04/02 19:21:52]

Okay. Done.

+  };
+
+  // |next_delegate| can be NULL.
+  explicit EvictedDomainCookieCounter(
+    scoped_refptr<net::CookieMonster::Delegate> next_delegate);

[mmenke, 2013/03/29 15:22:16]

nit:  Fix indent.

[huangs, 2013/04/02 19:21:52]

Done.

+
+  EvictedDomainCookieCounter(

[mmenke, 2013/03/29 15:22:16]

Think it's worth commenting that this constructor is only exposed for testing
(And the same for the public class and struct).

Could also consider making them non-public, though that would require you to
muck with things a bit to get the test access to them.

[huangs, 2013/04/02 19:21:52]

Added comment. We opted to expose extra constructor, as "friend" was judged to
be a greater scourge.

+      scoped_refptr<net::CookieMonster::Delegate> next_cookie_monster_delegate,
+      scoped_ptr<Delegate> delegate,

[mmenke, 2013/03/29 15:22:16]

I think having two different classes named simple "delegate" is confusing.  I'd
suggest renaming "delegate" cookie_counter_delegate.  May be worth renaming the
class, too.

[huangs, 2013/04/02 19:21:52]

Renamed by the variable, but not the class. The context of subclass Delegate
should be clear by the context (A::Delegate for class A; B::Delegate for class
B, simply Delegate for current class).

+      size_t max_size,
+      size_t purge_count);
+
+  // Returns the number of evicted cookies stored.
+  size_t GetStorageSize() const;
+
+  // CookieMonster::Delegate implementation.
+  virtual void OnCookieChanged(const net::CanonicalCookie& cc,
+                               bool removed,
+                               ChangeCause cause) OVERRIDE;
+
+ private:
+  // Identifier of an evicted cookie.
+  typedef std::string EvictedCookieKey;
+
+  // Storage class of evicted cookie.
+  typedef std::map<EvictedCookieKey, EvictedCookie*> EvictedCookieMap;
+
+  virtual ~EvictedDomainCookieCounter();
+
+  // Computes a key for |cc| compatible with CanonicalCookie::IsEquivalent(),
+  // i.e., IsEquivalent(a, b) ==> GetKey(a) == GetKey(b).
+  static EvictedCookieKey GetKey(const net::CanonicalCookie& cc);
+
+  // If too many evicted cookies are stored, delete the expired ones, then
+  // delete cookies that were evicted the longest, until size limit reached.
+  void GarbageCollect(const base::Time& current);
+
+  // Called when a cookie is evicted. Adds the evicted cookie to storage,
+  // possibly replacing an existing equivalent cookie.
+  void StoreEvictedCookie(const EvictedCookieKey& key,
+                          const net::CanonicalCookie& cc,

[mmenke, 2013/03/29 15:22:16]

Google style discourages non-standard abbreviations like.  Suggest just
canonical_cookie.

[huangs, 2013/04/02 19:21:52]

cc was used in CookieMonster. Compromising by renaming to "cookie".

+                          const base::Time& current);

[mmenke, 2013/03/29 15:22:16]

"current_time" is a much clearer variable name than "current"

[huangs, 2013/04/02 19:21:52]

Done.

+
+  // Called when a new cookie is added. If reinstatement occurs, then notifies
+  // |delegate_| and then removes the evicted cookie.
+  void ProcessNewCookie(const EvictedCookieKey& key,
+                        const net::CanonicalCookie& cc,

[mmenke, 2013/03/29 15:22:16]

canonical_cookie

[huangs, 2013/04/02 19:21:52]

Done.

+                        const base::Time& current);

[mmenke, 2013/03/29 15:22:16]

"current_time"

[huangs, 2013/04/02 19:21:52]

Done.

+
+  // Another delegate to forward events to.
+  scoped_refptr<net::CookieMonster::Delegate> next_cookie_monster_delegate_;
+
+  scoped_ptr<Delegate> delegate_;

[mmenke, 2013/03/29 15:22:16]

suggest "cookie_counter_delegate" per earlier comment.

[huangs, 2013/04/02 19:21:52]

Done.

+
+  EvictedCookieMap evicted_cookies_;
+
+  // Capacity of the evicted cookie storage, before garbage collection occurs.
+  const size_t max_size_;
+
+  // After garbage collection, size reduces to <= |max_size_| - |purge_count_|.
+  const size_t purge_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(EvictedDomainCookieCounter);
+};
+
+}  // namespace chrome_browser_net
+
+#endif  // CHROME_BROWSER_NET_EVICTED_DOMAIN_COOKIE_COUNTER_H_