Introduce a request throttler for limiting requests in mobile NTP.

components/ntp_snippets/request_counter.cc

Index: components/ntp_snippets/request_counter.cc
diff --git a/components/ntp_snippets/request_counter.cc b/components/ntp_snippets/request_counter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4f83bbb9bfbc321d3d899053f65b7c2209b0f148
--- /dev/null
+++ b/components/ntp_snippets/request_counter.cc
@@ -0,0 +1,133 @@
+// Copyright 2016 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 "components/ntp_snippets/request_counter.h"
+
+#include <vector>
+
+#include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/ntp_snippets_constants.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "components/variations/variations_associated_data.h"
+
+namespace {
+
+const char kQuotaParamPrefix[] = "quota_";
+const char kUMAHistogramTotalFormat[] = "NewTabPage.RequestCounter.Total_%s";
+const char kUMAHistogramPerDayFormat[] = "NewTabPage.RequestCounter.PerDay_%s";
+
+}  // namespace
+
+namespace ntp_snippets {
+
+RequestCounter::RequestCounter(PrefService* pref_service,
+                               RequestCounter::RequestType type,

[Marc Treib, 2016/07/20 10:19:05]

nit: "RequestCounter::" not needed

[jkrcal, 2016/07/20 12:09:27]

Done.

+                               int default_quota)
+    : pref_service_(pref_service), type_(type) {
+  DCHECK(pref_service);
+
+  std::string quota = variations::GetVariationParamValue(

[Marc Treib, 2016/07/20 10:19:05]

This should really be GetVariationParamValueByFeature, but I guess that's not
possible yet because the feature is currently defined somewhere in
c/b/android/... oh well.

[jkrcal, 2016/07/20 12:09:27]

This is exactly the reason. I do not think it is worth passing the feature to
the service / fetcher / ...

[Marc Treib, 2016/07/20 12:58:27]

No, we should just move the feature definition into the component. (That's for
another CL though)

[jkrcal, 2016/07/20 14:20:17]

Okay, this would make sense.

+      ntp_snippets::kStudyName, kQuotaParamPrefix + GetRequestTypeAsString());
+  if (!base::StringToInt(quota, &quota_)) {
+    DCHECK(quota.empty()) << "Invalid variation parameter for quota for "
+                          << GetRequestTypeAsString();
+    quota_ = default_quota;
+  }
+
+  counter_ = pref_service_->GetInteger(GetCountPref());
+
+  int count = static_cast<int>(RequestStatus::REQUEST_STATUS_MAX);
+  histogram_total_ = base::LinearHistogram::FactoryGet(
+      base::StringPrintf(kUMAHistogramTotalFormat,
+                         GetRequestTypeAsString().c_str()),
+      1, count, count + 1, base::HistogramBase::kUmaTargetedHistogramFlag);
+}
+
+bool RequestCounter::GetQuota() {
+  ResetCounterIfDayChanged();
+
+  counter_++;
+  pref_service_->SetInteger(GetCountPref(), counter_);
+
+  bool available = (counter_ <= quota_);
+  histogram_total_->Add(
+      static_cast<int>(available ? RequestStatus::QUOTA_GRANTED
+                                 : RequestStatus::QUOTA_EXCEEDED));
+  return available;
+}
+
+void RequestCounter::ReportForcedRequest() const {
+  histogram_total_->Add(static_cast<int>(RequestStatus::FORCED));
+}
+
+void RequestCounter::ResetCounterIfDayChanged() {
+  // The count of days since a fixed reference (the Unix epoch).
+  int now_day = (base::Time::Now() - base::Time::UnixEpoch()).InDays();
+  DLOG(WARNING) << "Counter " << now_day;

[Marc Treib, 2016/07/20 10:19:05]

This is too spammy - DVLOG?

[jkrcal, 2016/07/20 12:09:27]

Oh, I am sorry, a debugging line leaked into the CL :)

+  std::string day_pref_key = GetDaysPref();
+
+  if (!pref_service_->HasPrefPath(day_pref_key)) {
+    // The counter starts from 0 for the first time in this profile.
+    pref_service_->SetInteger(day_pref_key, now_day);
+  } else if (now_day != pref_service_->GetInteger(day_pref_key)) {
+    // Day has changed - report the no of requests from the previous day.

[Marc Treib, 2016/07/20 10:19:05]

s/no/number

[jkrcal, 2016/07/20 12:09:27]

Done.

+    base::HistogramBase* histogram_per_day_ = base::Histogram::FactoryGet(
+        base::StringPrintf(kUMAHistogramPerDayFormat,
+                           GetRequestTypeAsString().c_str()),
+        1, 100, 50,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
+    histogram_per_day_->Add(counter_);
+
+    pref_service_->SetInteger(day_pref_key, now_day);
+    counter_ = 0;

[Marc Treib, 2016/07/20 10:19:05]

Another argument for just having the pref as the single source-of-truth: You're
just setting the variable here, but leaving the pref at its old value.

[jkrcal, 2016/07/20 12:09:27]

Not a bug, the value is set right after in GetQuota. 
Place for potential future bug, though...

Okay, I removed |counter_|.

+  }
+}
+
+std::string RequestCounter::GetRequestTypeAsString() const {
+  // When adding a new type here, extend also the "RequestCounterTypes"
+  // <histogram_suffixes> in histograms.xml.
+  switch (type_) {

[Marc Treib, 2016/07/20 10:19:05]

Hm. Could we just have a global array mapping from RequestType to string, pref
name, and whatever else is needed? Then there'd be a single place where all
these things need to be added.

[jkrcal, 2016/07/20 12:09:27]

Done.

+    case RequestCounter::RequestType::ARTICLE_CONTENT_FETCHER:
+      return "Fetcher";
+    default:

[Marc Treib, 2016/07/20 10:19:05]

I would leave out the default case, then you should get a compiler error if a
new enum value is created but not added to the switch.
(Also in the other switches)

[jkrcal, 2016/07/20 12:09:27]

You are right. The code is not there, anymore.

+      NOTREACHED();
+      return "";
+  }
+}
+
+std::string RequestCounter::GetCountPref() const {
+  switch (type_) {
+    case RequestCounter::RequestType::ARTICLE_CONTENT_FETCHER:
+      return ntp_snippets::prefs::kSnippetFetcherQuotaCount;
+    default:
+      NOTREACHED();
+      return "";
+  }
+}
+
+std::string RequestCounter::GetDaysPref() const {
+  switch (type_) {
+    case RequestCounter::RequestType::ARTICLE_CONTENT_FETCHER:
+      return ntp_snippets::prefs::kSnippetFetcherQuotaDay;
+    default:
+      NOTREACHED();
+      return "";
+  }
+}
+
+// static
+void RequestCounter::RegisterProfilePrefs(PrefRegistrySimple* registry) {

[Marc Treib, 2016/07/20 10:19:05]

nit: move up, to match the order in the header

[jkrcal, 2016/07/20 12:09:27]

Done.

+  registry->RegisterIntegerPref(ntp_snippets::prefs::kSnippetFetcherQuotaDay,
+                                0);
+  registry->RegisterIntegerPref(ntp_snippets::prefs::kSnippetFetcherQuotaCount,
+                                0);
+}
+
+}  // namespace ntp_snippets