Adding a request counter to the snippets fetcher.

This CL applies the RequestCounter to NTPSnippetsFetcher.

BUG=627073

Review-Url: https://codereview.chromium.org/2162533002
Cr-Commit-Position: refs/heads/master@{#407769}

chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java

@@ -121,7 +121,8 @@ public class ChromeBackgroundService extends GcmTaskService {
 
     @VisibleForTesting
     protected void fetchSnippets() {
-        SnippetsBridge.fetchSnippets();
+        // Do not force regular background fetches.
+        SnippetsBridge.fetchSnippets(/*forceRequest=*/false);
     }
 
     private void handleRescheduleSnippets(Context context, String tag) {

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java

@@ -239,7 +239,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
 
     /** Start a request for new snippets. */
     public void reloadSnippets() {
-        SnippetsBridge.fetchSnippets();
+        SnippetsBridge.fetchSnippets(/*forceRequest=*/true);
     }
 
     private void loadSnippets(List<SnippetArticleListItem> snippets) {

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java

@@ -67,8 +67,8 @@ public class SnippetsBridge {
     /**
      * Fetches new snippets.
      */
-    public static void fetchSnippets() {
-        nativeFetchSnippets();
+    public static void fetchSnippets(boolean forceRequest) {
+        nativeFetchSnippets(forceRequest);
     }
 
     /**
@@ -148,7 +148,7 @@ public class SnippetsBridge {
 
     private native long nativeInit(Profile profile);
     private native void nativeDestroy(long nativeNTPSnippetsBridge);
-    private static native void nativeFetchSnippets();
+    private static native void nativeFetchSnippets(boolean forceRequest);
     private static native void nativeRescheduleFetching();
     private native void nativeDiscardSnippet(long nativeNTPSnippetsBridge, String snippetId);
     private native void nativeSetObserver(long nativeNTPSnippetsBridge, SnippetsBridge bridge);

chrome/browser/android/ntp/ntp_snippets_bridge.cc

@@ -56,9 +56,11 @@ static jlong Init(JNIEnv* env,
 }
 
 static void FetchSnippets(JNIEnv* env,
-                          const JavaParamRef<jclass>& caller) {
+                          const JavaParamRef<jclass>& caller,
+                          jboolean j_force_request) {
   Profile* profile = ProfileManager::GetLastUsedProfile();
-  NTPSnippetsServiceFactory::GetForProfile(profile)->FetchSnippets();
+  NTPSnippetsServiceFactory::GetForProfile(profile)->FetchSnippets(
+      j_force_request);
 }
 
 // Reschedules the fetching of snippets. Used to support different fetching

chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc

@@ -124,6 +124,7 @@ KeyedService* NTPSnippetsServiceFactory::BuildServiceInstanceFor(
           g_browser_process->GetApplicationLocale(), scheduler,
           base::MakeUnique<ntp_snippets::NTPSnippetsFetcher>(
               signin_manager, token_service, request_context,
+              profile->GetPrefs(),
               base::Bind(&safe_json::SafeJsonParser::Parse),
               chrome::GetChannel() == version_info::Channel::STABLE),
           base::MakeUnique<ImageFetcherImpl>(

chrome/browser/ui/webui/snippets_internals_message_handler.cc

@@ -235,7 +235,7 @@ void SnippetsInternalsMessageHandler::HandleDownload(
       hosts_string, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
   std::set<std::string> hosts(hosts_vector.begin(), hosts_vector.end());
 
-  ntp_snippets_service_->FetchSnippetsFromHosts(hosts);
+  ntp_snippets_service_->FetchSnippetsFromHosts(hosts, /*force_requests=*/true);
 }
 
 void SnippetsInternalsMessageHandler::HandleClearCachedSuggestions(

components/ntp_snippets/ntp_snippets_fetcher.cc

@@ -158,6 +158,7 @@ NTPSnippetsFetcher::NTPSnippetsFetcher(
     SigninManagerBase* signin_manager,
     OAuth2TokenService* token_service,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
+    PrefService* pref_service,
     const ParseJSONCallback& parse_json_callback,
     bool is_stable_channel)
     : OAuth2TokenService::Consumer("ntp_snippets"),
@@ -172,6 +173,9 @@ NTPSnippetsFetcher::NTPSnippetsFetcher(
                      : CHROME_READER_API),
       is_stable_channel_(is_stable_channel),
       tick_clock_(new base::DefaultTickClock()),
+      request_throttler_(
+          pref_service,
+          RequestThrottler::RequestType::CONTENT_SUGGESTION_FETCHER),
       weak_ptr_factory_(this) {
   // Parse the variation parameters and set the defaults if missing.
   std::string personalization = variations::GetVariationParamValue(
@@ -214,7 +218,11 @@ void NTPSnippetsFetcher::SetCallback(
 void NTPSnippetsFetcher::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     const std::string& language_code,
-    int count) {
+    int count,
+    bool force_request) {
+  if (!request_throttler_.DemandQuotaForRequest(force_request))
+    return;
+
   hosts_ = hosts;
   fetch_start_time_ = tick_clock_->NowTicks();
 

components/ntp_snippets/ntp_snippets_fetcher.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/callback.h"
@@ -16,10 +17,12 @@
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/ntp_snippet.h"
+#include "components/ntp_snippets/request_throttler.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 
+class PrefService;
 class SigninManagerBase;
 
 namespace base {
@@ -76,6 +79,7 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
       SigninManagerBase* signin_manager,
       OAuth2TokenService* oauth2_token_service,
       scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      PrefService* pref_service,
       const ParseJSONCallback& parse_json_callback,
       bool is_stable_channel);
   ~NTPSnippetsFetcher() override;
@@ -90,9 +94,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   // If an ongoing fetch exists, it will be cancelled and a new one started,
   // without triggering an additional callback (i.e. not noticeable by
   // subscriber of SetCallback()).
+  //
+  // Fetches snippets only if the daily quota not exceeded, unless
+  // |force_request| is set to true. Use force only for user-initiated fetches.
   void FetchSnippetsFromHosts(const std::set<std::string>& hosts,
                               const std::string& language_code,
-                              int count);
+                              int count,
+                              bool force_request);
 
   // Debug string representing the status/result of the last fetch attempt.
   const std::string& last_status() const { return last_status_; }
@@ -217,6 +225,9 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
   // Allow for an injectable tick clock for testing.
   std::unique_ptr<base::TickClock> tick_clock_;
 
+  // Request throttler for limiting requests.
+  RequestThrottler request_throttler_;
+
   base::WeakPtrFactory<NTPSnippetsFetcher> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsFetcher);

components/ntp_snippets/ntp_snippets_service.cc

@@ -247,19 +247,20 @@ void NTPSnippetsService::Shutdown() {
   EnterState(State::SHUT_DOWN, ContentSuggestionsCategoryStatus::NOT_PROVIDED);
 }
 
-void NTPSnippetsService::FetchSnippets() {
+void NTPSnippetsService::FetchSnippets(bool force_request) {
   if (ready())
-    FetchSnippetsFromHosts(GetSuggestionsHosts());
+    FetchSnippetsFromHosts(GetSuggestionsHosts(), force_request);
   else
     fetch_after_load_ = true;
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
-    const std::set<std::string>& hosts) {
+    const std::set<std::string>& hosts,
+    bool force_request) {
   if (!ready())
     return;
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
-                                            kMaxSnippetCount);
+                                            kMaxSnippetCount, force_request);
 }
 
 void NTPSnippetsService::RescheduleFetching() {
@@ -440,7 +441,7 @@ void NTPSnippetsService::OnSuggestionsChanged(
 
   NotifyNewSuggestions();
 
-  FetchSnippetsFromHosts(hosts);
+  FetchSnippetsFromHosts(hosts, /*force_request=*/false);
 }
 
 void NTPSnippetsService::OnFetchFinished(
@@ -655,7 +656,7 @@ void NTPSnippetsService::FetchSnippetImageFromNetwork(
 
 void NTPSnippetsService::EnterStateEnabled(bool fetch_snippets) {
   if (fetch_snippets)
-    FetchSnippets();
+    FetchSnippets(/*force_request=*/false);
 
   // If host restrictions are enabled, register for host list updates.
   // |suggestions_service_| can be null in tests.

components/ntp_snippets/ntp_snippets_service.h

@@ -101,12 +101,17 @@ class NTPSnippetsService : public KeyedService,
   bool initialized() const { return ready() || state_ == State::DISABLED; }
 
   // Fetches snippets from the server and adds them to the current ones.
-  void FetchSnippets();
+  // Requests can be marked more important by setting |force_request| to true
+  // (such request might circumvent the daily quota for requests, etc.) Useful
+  // for requests triggered by the user.
+  void FetchSnippets(bool force_request);
+
   // Fetches snippets from the server for specified hosts (overriding
   // suggestions from the suggestion service) and adds them to the current ones.
   // Only called from chrome://snippets-internals, DO NOT USE otherwise!
   // Ignored while |loaded()| is false.
-  void FetchSnippetsFromHosts(const std::set<std::string>& hosts);
+  void FetchSnippetsFromHosts(const std::set<std::string>& hosts,
+                              bool force_request);
 
   // Available snippets.
   const NTPSnippet::PtrVector& snippets() const { return snippets_; }

components/ntp_snippets/ntp_snippets_service_unittest.cc

@@ -269,6 +269,7 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
         test_url_(base::StringPrintf(kTestContentSnippetsServerFormat,
                                      google_apis::GetAPIKey().c_str())) {
     NTPSnippetsService::RegisterProfilePrefs(pref_service()->registry());
+    RequestThrottler::RegisterProfilePrefs(pref_service()->registry());
 
     // Since no SuggestionsService is injected in tests, we need to force the
     // service to fetch from all hosts.
@@ -311,7 +312,8 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher =
         base::MakeUnique<NTPSnippetsFetcher>(
             fake_signin_manager(), fake_token_service_.get(),
-            std::move(request_context_getter), base::Bind(&ParseJson),
+            std::move(request_context_getter), pref_service(),
+            base::Bind(&ParseJson),
             /*is_stable_channel=*/true);
 
     fake_signin_manager()->SignIn("foo@bar.com");
@@ -352,7 +354,7 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
 
   void LoadFromJSONString(const std::string& json) {
     SetUpFetchResponse(json);
-    service()->FetchSnippets();
+    service()->FetchSnippets(true);
     base::RunLoop().RunUntilIdle();
   }
 
@@ -876,7 +878,7 @@ TEST_F(NTPSnippetsServiceTest, HistorySyncStateChanges) {
   service()->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(NTPSnippetsService::State::DISABLED, service()->state_);
-  EXPECT_THAT(service()->snippets(), IsEmpty()); // No fetch should be made.
+  EXPECT_THAT(service()->snippets(), IsEmpty());  // No fetch should be made.
 
   // Simulate user sign in. The service should be ready again and load snippets.
   SetUpFetchResponse(GetTestJson({GetSnippet()}));

components/ntp_snippets/request_throttler.cc

@@ -19,13 +19,6 @@
 
 namespace ntp_snippets {
 
-// Used internally for working with a RequestType.
-struct RequestTypeInfo {
-  const char* name;
-  const char* count_pref;
-  const char* day_pref;
-};
-
 namespace {
 
 // Enumeration listing all possible outcomes for fetch attempts. Used for UMA
@@ -38,18 +31,23 @@ enum class RequestStatus {
   REQUEST_STATUS_COUNT
 };
 
+}  // namespace
+
+struct RequestThrottler::RequestTypeInfo {
+    const char* name;
+    const char* count_pref;
+    const char* day_pref;
+    const int default_quota;
+};
+
 // When adding a new type here, extend also the "RequestCounterTypes"
 // <histogram_suffixes> in histograms.xml with the |name| string.
-const RequestTypeInfo kRequestTypeInfo[] = {
+const RequestThrottler::RequestTypeInfo RequestThrottler::kRequestTypeInfo[] = {
     // RequestCounter::RequestType::CONTENT_SUGGESTION_FETCHER,
     {"SuggestionFetcher", prefs::kSnippetFetcherQuotaCount,
-     prefs::kSnippetFetcherQuotaDay}};
-
-}  // namespace
+     prefs::kSnippetFetcherQuotaDay, 50}};
 
-RequestThrottler::RequestThrottler(PrefService* pref_service,
-                               RequestType type,
-                               int default_quota)
+RequestThrottler::RequestThrottler(PrefService* pref_service, RequestType type)
     : pref_service_(pref_service),
       type_info_(kRequestTypeInfo[static_cast<int>(type)]) {
   DCHECK(pref_service);
@@ -61,7 +59,7 @@ RequestThrottler::RequestThrottler(PrefService* pref_service,
     LOG_IF(WARNING, !quota.empty())
         << "Invalid variation parameter for quota for "
         << GetRequestTypeAsString();
-    quota_ = default_quota;
+    quota_ = type_info_.default_quota;
   }
 
   // Since the histogram names are dynamic, we cannot use the standard macros

components/ntp_snippets/request_throttler.h

@@ -18,8 +18,6 @@ class HistogramBase;
 
 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:
@@ -45,9 +43,7 @@ class RequestThrottler {
     CONTENT_SUGGESTION_FETCHER
   };
 
-  RequestThrottler(PrefService* pref_service,
-                 RequestType type,
-                 int default_quota);
+  RequestThrottler(PrefService* pref_service, RequestType type);
 
   // Registers profile prefs for all RequestTypes. Called from browser_prefs.cc.
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
@@ -59,6 +55,13 @@ class RequestThrottler {
   bool DemandQuotaForRequest(bool force_request);
 
  private:
+  friend class RequestThrottlerTest;
+  // Used internally for working with a RequestType.
+  struct RequestTypeInfo;
+
+  // The array of info entries - one per each RequestType.
+  static const RequestTypeInfo kRequestTypeInfo[];
+
   // Also emits the PerDay histogram if the day changed.
   void ResetCounterIfDayChanged();
 

components/ntp_snippets/request_throttler_unittest.cc

@@ -24,8 +24,9 @@ class RequestThrottlerTest : public testing::Test {
     RequestThrottler::RegisterProfilePrefs(test_prefs_.registry());
     // Use any arbitrary RequestType for this unittest.
     throttler_.reset(new RequestThrottler(
-        &test_prefs_, RequestThrottler::RequestType::CONTENT_SUGGESTION_FETCHER,
-        kCounterQuota));
+        &test_prefs_,
+        RequestThrottler::RequestType::CONTENT_SUGGESTION_FETCHER));
+    throttler_->quota_ = kCounterQuota;
   }
 
  protected:

ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc

@@ -113,20 +113,21 @@ IOSChromeNTPSnippetsServiceFactory::BuildServiceInstanceFor(
               base::SequencedWorkerPool::GetSequenceToken(),
               base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
 
+  PrefService* prefs = chrome_browser_state->GetPrefs();
   // TODO(treib,markusheintz): Inject an image_fetcher::ImageDecoder once that's
   // implemented on iOS. crbug.com/609127
   return base::WrapUnique(new ntp_snippets::NTPSnippetsService(
-      false /* enabled */, chrome_browser_state->GetPrefs(),
-      suggestions_service, GetApplicationContext()->GetApplicationLocale(),
-      scheduler, base::WrapUnique(new ntp_snippets::NTPSnippetsFetcher(
-                     signin_manager, token_service, request_context,
-                     base::Bind(&ParseJson),
-                     GetChannel() == version_info::Channel::STABLE)),
+      false /* enabled */, prefs, suggestions_service,
+      GetApplicationContext()->GetApplicationLocale(), scheduler,
+      base::WrapUnique(new ntp_snippets::NTPSnippetsFetcher(
+          signin_manager, token_service, request_context, prefs,
+          base::Bind(&ParseJson),
+          GetChannel() == version_info::Channel::STABLE)),
       base::WrapUnique(new ImageFetcherImpl(request_context.get(),
                                             web::WebThread::GetBlockingPool())),
       base::MakeUnique<IOSImageDecoderImpl>(),
       base::WrapUnique(
           new ntp_snippets::NTPSnippetsDatabase(database_dir, task_runner)),
       base::WrapUnique(new ntp_snippets::NTPSnippetsStatusService(
-          signin_manager, sync_service, chrome_browser_state->GetPrefs()))));
+          signin_manager, sync_service, prefs))));
 }