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

net/cookies/evicted_domain_cookie_counter.cc

+// 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.
+
+#include "net/cookies/evicted_domain_cookie_counter.h"
+
+#include <limits>
+#include <queue>
+#include <vector>
+
+#include "base/metrics/histogram.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/cookies/canonical_cookie.h"
+
+namespace net {
+
+using base::Time;
+using base::TimeDelta;
+
+namespace {
+
+const size_t kMaxEvictedDomainCookies = 500;
+const size_t kPurgeEvictedDomainCookies = 100;
+
+class DelegateImpl : public EvictedDomainCookieCounter::Delegate {
+ public:
+  DelegateImpl();
+
+  // EvictedDomainCookieCounter::Delegate implementation.
+  virtual void Report(const EvictedDomainCookieCounter::EvictedCookie& ec,
+                      const Time& reinstatement_time) OVERRIDE;
+  virtual Time CurrentTime() OVERRIDE;
+};
+
+DelegateImpl::DelegateImpl() {}
+
+void DelegateImpl::Report(const EvictedDomainCookieCounter::EvictedCookie& ec,
+                          const Time& reinstatement_time) {
+  static TimeDelta time_min(TimeDelta::FromSeconds(1));
+  static TimeDelta time_max(TimeDelta::FromDays(7));
+  TimeDelta reinstatement_delay(reinstatement_time - ec.eviction_time_);
+  if (ec.is_google_) {
+    HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogleSeconds",

[erikwright (departed), 2013/03/22 15:53:26]

I believe that this macro already defines a static object, so there's no need
for you to use statics for the two constants. You should be able to inline them.

Please grep for other invocations and do what they do (I suspect it's inline).

[erikwright (departed), 2013/03/22 15:53:26]

Please check for other uses of HISTOGRAM_CUSTOM_TIMES. Is it appropriate to
include the unit in the name?

[huangs, 2013/03/22 21:38:07]

Yeah it can be inlined.

[huangs, 2013/03/22 21:38:07]

I was probably looking at the wrong things.  Removed.

+                           reinstatement_delay, time_min, time_max, 50);
+  } else {
+    HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesOtherSeconds",
+                           reinstatement_delay, time_min, time_max, 50);
+  }
+}
+
+Time DelegateImpl::CurrentTime() {
+  return Time::Now();
+}
+
+}  // namespace
+
+EvictedDomainCookieCounter::EvictedDomainCookieCounter(
+    scoped_refptr<CookieMonster::Delegate> next_delegate)
+    : next_delegate_(next_delegate),
+      reporter_(new DelegateImpl),
+      max_size_(kMaxEvictedDomainCookies),
+      purge_count_(kPurgeEvictedDomainCookies) {
+  DCHECK(reporter_);

[erikwright (departed), 2013/03/22 15:53:26]

Not required here, since you just assigned it three lines up.

[huangs, 2013/03/22 21:38:07]

Done.

+}
+
+EvictedDomainCookieCounter::EvictedDomainCookieCounter(
+    scoped_refptr<CookieMonster::Delegate> next_delegate,
+    scoped_ptr<Delegate> reporter,
+    size_t max_size,
+    size_t purge_count)
+    : next_delegate_(next_delegate),
+      reporter_(reporter.Pass()),
+      max_size_(max_size),
+      purge_count_(purge_count) {
+  DCHECK(reporter_);
+  DCHECK(purge_count < max_size_);

[erikwright (departed), 2013/03/22 15:53:26]

Unless something is going to crash later because of this, I wouldn't worry about
it.

[huangs, 2013/03/22 21:38:07]

These are both size_t's, and a difference will be taken. So negative => large
positive. Better to be safe, I think.

+}
+
+EvictedDomainCookieCounter::~EvictedDomainCookieCounter() {
+  STLDeleteContainerPairSecondPointers(evicted_cookies_.begin(),
+                                       evicted_cookies_.end());
+}
+
+// static
+// Simplified from google_util.cc: IsGoogleHostname().
+bool EvictedDomainCookieCounter::UnsafeIsGoogleCookie(
+    const CanonicalCookie& cc) {
+  const std::string& host(cc.Domain());
+  size_t tld_length =
+      RegistryControlledDomainService::GetRegistryLength(host, false);
+  if ((tld_length == 0) || (tld_length == std::string::npos))
+    return false;
+
+  std::string host_minus_tld(host, 0, host.length() - tld_length);
+  return LowerCaseEqualsASCII(host_minus_tld, "google.") ||
+      EndsWith(host_minus_tld, ".google.", false);
+}
+
+size_t EvictedDomainCookieCounter::GetStorageSize() const {
+  return evicted_cookies_.size();
+}
+
+void EvictedDomainCookieCounter::OnCookieChanged(const CanonicalCookie& cc,
+                                                 bool removed,
+                                                 ChangeCause cause) {
+  EvictedDomainCookieCounter::EvictedCookieKey key(GetKey(cc));
+  Time current(reporter_->CurrentTime());
+  if (removed) {
+    if (cause == CookieMonster::Delegate::CHANGE_COOKIE_EVICTED)
+      StoreEvictedCookie(key, cc, current);
+    else  // Explicit deletion, so stop tracking.
+      FreeEvictedCookie(key);
+  } else {  // Including adds or updates.
+    if (ProcessCookieReinstatement(key, cc, current))

[erikwright (departed), 2013/03/22 15:53:26]

I think it would be clearer if ProcessCookieReinstatement were renamed to
ProcessNewCookie and handled both the potential reporting and eviction.

[huangs, 2013/03/22 21:38:07]

Renamed, and moved deletion to the routine.

+      FreeEvictedCookie(key);
+  }
+
+  if (next_delegate_)
+    next_delegate_->OnCookieChanged(cc, removed, cause);
+}
+
+// static
+EvictedDomainCookieCounter::EvictedCookieKey
+    EvictedDomainCookieCounter::GetKey(const CanonicalCookie& cc) {
+  return cc.Domain() + ";" + cc.Path() + ";" + cc.Name();
+}
+
+void EvictedDomainCookieCounter::FreeEvictedCookie(
+    const EvictedCookieKey& key) {
+  EvictedCookieMap::iterator it = evicted_cookies_.find(key);
+  if (it != evicted_cookies_.end()) {
+    delete it->second;
+    evicted_cookies_.erase(it);
+  }
+}
+
+// |current| is passed by value, in case it came from a to-be-deleted element.
+void EvictedDomainCookieCounter::FreeExpiredEvictedCookies(Time current) {
+  EvictedCookieMap::iterator it = evicted_cookies_.begin();
+  while (it != evicted_cookies_.end()) {
+    if (it->second->is_expired(current)) {
+      delete it->second;
+      evicted_cookies_.erase(it++);  // Post-increment idiom.
+    } else {
+      ++it;
+    }
+  }
+}
+
+void EvictedDomainCookieCounter::GarbageCollect(const Time& current) {
+  if (evicted_cookies_.size() <= max_size_)
+    return;
+
+  // From |evicted_cookies_|, removed all expired cookies, and remove cookies
+  // with the oldest |eviction_time_| until |size_goal| is attained.
+  size_t size_goal = max_size_ - purge_count_;
+  // Bound on number of non-expired cookies to remove.
+  size_t remove_quota = evicted_cookies_.size() - size_goal;
+  DCHECK(remove_quota > 0);
+
+  // Among evicted cookies with the same eviction time, bias may arise owing to

[erikwright (departed), 2013/03/22 15:53:26]

LOL. You seem to take this personally. This does not require a comment.

[huangs, 2013/03/22 21:38:07]

Removed.  :(

+  // traversal order in EvictedCookieMap. However, we deem this effect minor,
+  // so we won't resolve ties.
+  struct EvictedCookiePriorityQueueEntry {

[erikwright (departed), 2013/03/22 15:53:26]

IIUC, you don't need a custom type here anymore, just a custom comparator
(second type parameter to std::priority_queue)

[huangs, 2013/03/22 21:38:07]

The second type parameter to std::priority_queue is the container. Third is the
comparator, BUT it looks like we need a class with operator(), rather than just
a function (as with the case of std::sort()).

+    EvictedCookie* cookie_ptr;
+
+    EvictedCookiePriorityQueueEntry(EvictedCookie* cookie_ptr)
+      : cookie_ptr(cookie_ptr) {}
+
+    bool operator < (const EvictedCookiePriorityQueueEntry& other) const {
+      return cookie_ptr->eviction_time_ < other.cookie_ptr->eviction_time_;
+    }
+  };
+
+  // Use priority queue to store oldest cookies, resize as necessary.
+  std::priority_queue<EvictedCookiePriorityQueueEntry> oldest_set;
+  for (EvictedCookieMap::iterator it = evicted_cookies_.begin();
+      it != evicted_cookies_.end(); ++it) {
+    if (it->second->is_expired(current))  // Will be removed, so decrease quota.
+      remove_quota = remove_quota ? remove_quota - 1 : 0;  // Cap to 0 (size_t).
+    else
+      oldest_set.push(EvictedCookiePriorityQueueEntry(it->second));
+
+    if (oldest_set.size() > remove_quota)
+      oldest_set.pop();  // Spare the youngest element from deletion.
+  }
+  DCHECK(oldest_set.size() <= remove_quota);
+
+  // Mark oldest non-expired cookies as expired, for removal.
+  while (!oldest_set.empty()) {
+    oldest_set.top().cookie_ptr->set_expired();
+    oldest_set.pop();
+  }
+
+  FreeExpiredEvictedCookies(current);
+  // Apply stricter check if non-expired cookies were deleted.
+  DCHECK(remove_quota ? evicted_cookies_.size() == size_goal :
+         evicted_cookies_.size() <= size_goal);
+}
+
+void EvictedDomainCookieCounter::StoreEvictedCookie(const EvictedCookieKey& key,
+                                                    const CanonicalCookie& cc,
+                                                    const Time& current) {
+  if (!cc.IsExpired(current)) {
+    FreeEvictedCookie(key);  // Deallocate explicitly.
+
+    EvictedCookie* ec =
+        new EvictedCookie(current, cc.ExpiryDate(), UnsafeIsGoogleCookie(cc));
+    evicted_cookies_.insert(EvictedCookieMap::value_type(key, ec));
+
+    GarbageCollect(current);
+  }
+}
+
+bool EvictedDomainCookieCounter::ProcessCookieReinstatement(
+    const EvictedCookieKey& key,
+    const CanonicalCookie& cc,
+    const Time& current) {
+  bool ret = false;
+  EvictedCookieMap::iterator it = evicted_cookies_.find(key);

[huangs, 2013/03/22 21:38:07]

If cc is expired, we would count it as being reinstated, which makes no sense. 
Fixing this.

+  if (it != evicted_cookies_.end()) {
+    if (!it->second->is_expired(current))
+      reporter_->Report(*it->second, current);
+    ret = true;
+  }
+  return ret;
+}
+
+}  // namespace net