Introduce a request throttler for limiting requests in mobile NTP.

components/ntp_snippets/request_counter.h

+// 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.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_REQUEST_COUNTER_H_
+#define COMPONENTS_NTP_SNIPPETS_REQUEST_COUNTER_H_
+
+#include <string>
+
+#include "base/macros.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace base {
+class HistogramBase;
+}  // namespace base
+
+namespace ntp_snippets {
+
+struct RequestTypeInfo;
+
+// Counts requests to external services, compares them to a daily quota, reports
+// them to UMA. In the application code, create one local instance for each type
+// of requests, identified by the RequestType. The request counter is based on:
+//  - daily quota from a variation param "quota_|type|" in the NTPSnippets trial
+//  - pref "ntp.requests_quota.|type|.count" to store the current counter value,
+//  - pref "ntp.requests_quota.|type|.day" to store current day to which the
+//    current counter value applies.
+// Furthermore the counter reports to histograms:
+//  - "NewTabPage.RequestCounter.RequestStatus_|type|" - status of each request;
+//  - "NewTabPage.RequestCounter.PerDay_|type|" - the daily number of requests.
+//
+// Implementation notes: When extending this class for a new RequestType, please
+//  1) define in request_counter.cc in kRequestTypeInfo
+//     a) the string value for your |type| and
+//     b) constants for day/count prefs;
+//  2) define a new RequestCounterTypes histogram suffix in histogram.xml (with
+//     the same string value as in 1a)).
+class RequestCounter {
+ public:
+  // Enumeration listing all current applications of the request counter.
+  enum class RequestType {
+    CONTENT_SUGGESTION_FETCHER

[tschumann, 2016/07/21 18:27:45]

are there plans for further types? if not, we might be better off hard-coding
this (YAGNI)

[jkrcal, 2016/07/21 19:17:38]

Sure there are: 
 - Fetching thumbnails,
 - Fetching favicons for articles,
 - Fetching favicons for NTP tiles,
and maybe more.

[tschumann, 2016/07/22 07:49:08]

ok. Let's have a quick in-person chat about those and how paranoid we plan to
be. Do we plan any pre-fetching of those? (if not, I'm not sure if we had any
non-forced loading of such resources).

[jkrcal, 2016/07/22 09:10:03]

Acknowledged.

+  };
+
+  // Enumeration listing all possible outcomes for fetch attempts. Used for UMA
+  // histogram, so do not change existing values. Insert new values at the end,
+  // and update the histogram definition.
+  enum class RequestStatus {

[tschumann, 2016/07/21 18:27:45]

why is this exposed as part of the public interface?
make it private or -- even better -- move into the .cc file?

[jkrcal, 2016/07/21 19:17:38]

Oh, thanks for a pair of fresh eyes. I could not spot such a mistake after so
many rounds of review :)

+    FORCED,
+    QUOTA_GRANTED,
+    QUOTA_EXCEEDED,
+    REQUEST_STATUS_COUNT
+  };
+
+  RequestCounter(PrefService* pref_service,
+                 RequestType type,
+                 int default_quota);
+
+  // Registers profile prefs for all RequestTypes.
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
+  // Returns whether quota is available for another request and reports this
+  // information to UMA. Forced requests do not count against the quota and

[tschumann, 2016/07/21 18:27:45]

i consider the counting aspect an implementation detail. IMO, such a comment
would be better in the .cc file and not on the header. For users of this class
those internals shouldn't matter. What you should say is that ForcedRequests are
guaranteed to get granted.
If you want to you can add that they should be directly initiated by the user.

[jkrcal, 2016/07/21 19:17:38]

I do not think this is an implementation detail. If I want to use the class, I
need to understand how to pick my quota so that it never hits the user under
normal conditions. 

Do not want to oppose you too strongly, though. I am just trying to understand
your arguments for future coding.

[tschumann, 2016/07/22 07:49:08]

My point would be that places which use this class only have a local view of
things -- that's also not where the quota is being defined. Those places only
care about having their requests properly throttled, and if somebody is
interested in the details of throttling, they can read the implementation of the
the throttler.
IMO, this class has one task -- to throttle requests. How that's done is this
classes implementation. All other parts of the system just need to make sure
they apply some throttling.

This might get a bit esotheric, but my main point is the following: There's a
steady risk of comments getting out of sync, especially if they are not right at
the implementation. If somebody really wants to understand the details of
throttling, he needs to read that code.

That said, I'm not feeling too strongly, as it's just a little detail. But we
need to make sure the comment stays up-to-date.

[jkrcal, 2016/07/22 09:10:03]

Thanks for explaining. Now I tend to agree (not too strongly :). Done.

+  // return always true -- useful for requests initiated by the user.
+  bool DemandQuotaForRequest(bool force_request);
+
+ private:
+  // Also emits the PerDay histogram if the day changed.
+  void ResetCounterIfDayChanged();
+
+  const char* GetRequestTypeAsString() const;
+
+  int GetCount() const;
+  void SetCount(int count);
+  int GetDay() const;
+  void SetDay(int day);
+  bool HasDay() const;
+
+  PrefService* pref_service_;
+  const RequestTypeInfo& type_info_;
+
+  // It comes from a variation parameter or |default_quota| as a fallback.
+  int quota_;
+
+  // The histograms for reporting the requests of the given |type_|.
+  base::HistogramBase* histogram_request_status_;
+  base::HistogramBase* histogram_per_day_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestCounter);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_REQUEST_COUNTER_H_