Connecting UserClassifier to NtpSnippetsFetcher

components/ntp_snippets/remote/request_throttler.cc

Index: components/ntp_snippets/remote/request_throttler.cc
diff --git a/components/ntp_snippets/remote/request_throttler.cc b/components/ntp_snippets/remote/request_throttler.cc
index 4777f38156a0108c3866c439c373feed27d99df0..0f5c1b56f00e1dcb2bf2a2c8f3e63900eec75939 100644
--- a/components/ntp_snippets/remote/request_throttler.cc
+++ b/components/ntp_snippets/remote/request_throttler.cc
@@ -5,6 +5,7 @@
 #include "components/ntp_snippets/remote/request_throttler.h"
 
 #include <climits>
+#include <set>
 #include <vector>
 
 #include "base/metrics/histogram.h"
@@ -50,11 +51,22 @@ struct RequestThrottler::RequestTypeInfo {
 // When adding a new type here, extend also the "RequestThrottlerTypes"
 // <histogram_suffixes> in histograms.xml with the |name| string.
 const RequestThrottler::RequestTypeInfo RequestThrottler::kRequestTypeInfo[] = {
-    // RequestCounter::RequestType::CONTENT_SUGGESTION_FETCHER,
-    {"SuggestionFetcher", prefs::kSnippetFetcherRequestCount,
+    // The following three types share the same prefs. They differ in quota
+    // values (and UMA histograms).
+    // RequestCounter::RequestType::CONTENT_SUGGESTION_FETCHER_RARE_NTP_USER,
+    {"SuggestionFetcherRareNTPUser", prefs::kSnippetFetcherRequestCount,
      prefs::kSnippetFetcherInteractiveRequestCount,
-     prefs::kSnippetFetcherRequestsDay, 50, kUnlimitedQuota},
-     // RequestCounter::RequestType::CONTENT_SUGGESTION_THUMBNAIL,
+     prefs::kSnippetFetcherRequestsDay, 5, kUnlimitedQuota},
+    // RequestCounter::RequestType::CONTENT_SUGGESTION_FETCHER_ACTIVE_NTP_USER,
+    {"SuggestionFetcherActiveNTPUser", prefs::kSnippetFetcherRequestCount,
+     prefs::kSnippetFetcherInteractiveRequestCount,
+     prefs::kSnippetFetcherRequestsDay, 20, kUnlimitedQuota},
+    // RequestCounter::RequestType::CONTENT_SUGGESTION_FETCHER_ACTIVE_SUGGESTIONS_CONSUMER,
+    {"SuggestionFetcherActiveSuggestionsConsumer",
+     prefs::kSnippetFetcherRequestCount,
+     prefs::kSnippetFetcherInteractiveRequestCount,
+     prefs::kSnippetFetcherRequestsDay, 20, kUnlimitedQuota},
+    // RequestCounter::RequestType::CONTENT_SUGGESTION_THUMBNAIL,
     {"SuggestionThumbnailFetcher", prefs::kSnippetThumbnailsRequestCount,
      prefs::kSnippetThumbnailsInteractiveRequestCount,
      prefs::kSnippetThumbnailsRequestsDay, kUnlimitedQuota, kUnlimitedQuota}};
@@ -107,10 +119,18 @@ RequestThrottler::RequestThrottler(PrefService* pref_service, RequestType type)
 
 // static
 void RequestThrottler::RegisterProfilePrefs(PrefRegistrySimple* registry) {
+  std::set<std::string> registered;
   for (const RequestTypeInfo& info : kRequestTypeInfo) {
+    // Make sure we do not register the same prefs twice. Assuming that if any
+    // types share some prefs, they share all the prefs. Thus, checking, e.g.,
+    // day_pref is enough.

[Marc Treib, 2016/10/06 14:54:37]

Any particular reason for not just checking each pref individually?

[jkrcal, 2016/10/06 15:18:41]

The code is slightly simpler / more efficient. It makes absolutely no sense for
types to share something and not to share everything. Again, if you insist, I
can change it.

[Marc Treib, 2016/10/06 15:27:26]

I think I'd find it slightly simpler/easier to understand if the code would just
check all prefs :)  At least, it'd make this 3-line comment unnecessary.
Anyway, this is not important enough to fight over :)

[jkrcal, 2016/10/06 18:25:38]

I 've rewritten the function in a way I like (and you hopefully as well) :)

+    if (registered.count(info.day_pref) > 0)
+      continue;
+
+    registered.insert(info.day_pref);
+    registry->RegisterIntegerPref(info.day_pref, 0);
     registry->RegisterIntegerPref(info.count_pref, 0);
     registry->RegisterIntegerPref(info.interactive_count_pref, 0);
-    registry->RegisterIntegerPref(info.day_pref, 0);
   }
 }