[Remote suggestions] Move all decisions to fetch to the scheduler

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

 // 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/remote/scheduling_remote_suggestions_provider.h"
 
 #include <random>
 #include <string>
 #include <utility>
 
@@ skipping 77 matching lines @@
             arraysize(kFetchingIntervalParamNameActiveSuggestionsConsumer),
     "Fill in all the info for fetching intervals.");
 
 const char* kTriggerTypeNames[] = {"persistent_scheduler_wake_up", "ntp_opened",
                                    "browser_foregrounded",
                                    "browser_cold_start"};
 
 const char* kTriggerTypesParamName = "scheduler_trigger_types";
 const char* kTriggerTypesParamValueForEmptyList = "-";
 
+const int kBlockBackgroundFetchesMinutesAfterClearingHistory = 30;
+
 // Returns the time interval to use for scheduling remote suggestion fetches for
 // the given interval and user_class.
 base::TimeDelta GetDesiredFetchingInterval(
     FetchingInterval interval,
     UserClassifier::UserClass user_class) {
   DCHECK(interval != FetchingInterval::COUNT);
   const unsigned int index = static_cast<unsigned int>(interval);
   DCHECK(index < arraysize(kDefaultFetchingIntervalHoursRareNtpUser));
 
   double default_value_hours = 0.0;
@@ skipping 77 matching lines @@
       background_fetch_in_progress_(false),
       user_classifier_(user_classifier),
       pref_service_(pref_service),
       clock_(std::move(clock)),
       enabled_triggers_(GetEnabledTriggerTypes()) {
   DCHECK(user_classifier);
   DCHECK(pref_service);
 
   LoadLastFetchingSchedule();
 
-  provider_->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::BindRepeating(
-              &SchedulingRemoteSuggestionsProvider::OnProviderStatusChanged,
-              base::Unretained(this))));
+  provider_->SetRemoteSuggestionsScheduler(this);

[Marc Treib, 2017/02/23 13:52:24]

This is a bit weird: We now have two-way linking between the SchedulingProvider
and the Impl. And this method is defined on the common interface, but it should
never be called on this class.

Also, do we need to set it back to nullptr at some point? What if the provider
tries to call the scheduler during destruction, something like that?

[jkrcal, 2017/02/24 09:21:43]

Added a bug and TODOs (after an offline discussion). 

I do not think we need to null it because this class has a unique_ptr to the
provider_  so it is destructed first.

 }
 
 SchedulingRemoteSuggestionsProvider::~SchedulingRemoteSuggestionsProvider() =
     default;
 
 // static
 void SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
   registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalWifi, 0);
   registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalFallback,
                               0);
   registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
                               0);
   registry->RegisterInt64Pref(prefs::kSnippetLastFetchAttempt, 0);
   registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalOnNtpOpened,
                               0);
 }
 
+void SchedulingRemoteSuggestionsProvider::OnProviderActivated() {
+  StartScheduling();
+}
+
+void SchedulingRemoteSuggestionsProvider::OnProviderInactivated() {
+  StopScheduling();
+}
+
+void SchedulingRemoteSuggestionsProvider::OnSuggestionsCleared() {
+  // There are no suggestions, be as eager to fetch as possible.
+  ClearLastFetchAttemptTime();
+}
+
+void SchedulingRemoteSuggestionsProvider::OnHistoryCleared() {
+  // Due to privacy, we should not fetch for a while (unless the user explicitly
+  // asks for new suggestions) do give sync the time to propagate the changes in

[Marc Treib, 2017/02/23 13:52:24]

s/do/to/

[jkrcal, 2017/02/24 09:21:43]

Done.

+  // history to the server.
+  background_fetches_allowed_after_ =
+      clock_->Now() +
+      base::TimeDelta::FromMinutes(
+          kBlockBackgroundFetchesMinutesAfterClearingHistory);
+  // After that time elapses, we should fetch as soon as possible.
+  ClearLastFetchAttemptTime();
+}
+
 void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {
   // Force the reschedule by stopping and starting it again.
   StopScheduling();
   StartScheduling();
 }
 
 void SchedulingRemoteSuggestionsProvider::OnPersistentSchedulerWakeUp() {
   RefetchInTheBackgroundIfEnabled(TriggerType::PERSISTENT_SCHEDULER_WAKE_UP);
 }
 
@@ skipping 18 matching lines @@
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
   if (!ShouldRefetchInTheBackgroundNow(TriggerType::NTP_OPENED)) {
     return;
   }
 
   RefetchInTheBackgroundIfEnabled(TriggerType::NTP_OPENED);
 }
 
-void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
-    std::unique_ptr<ProviderStatusCallback> callback) {
-  provider_->SetProviderStatusCallback(std::move(callback));
+void SchedulingRemoteSuggestionsProvider::SetRemoteSuggestionsScheduler(

[Marc Treib, 2017/02/23 13:52:24]

Can this method ever be called (on this particular class)?

[jkrcal, 2017/02/24 09:21:43]

Replaced by NOTREACHED()

+    RemoteSuggestionsScheduler* scheduler) {
+  provider_->SetRemoteSuggestionsScheduler(scheduler);
 }
 
 void SchedulingRemoteSuggestionsProvider::RefetchInTheBackground(
     std::unique_ptr<FetchStatusCallback> callback) {
   if (background_fetch_in_progress_) {
     if (callback) {
       callback->Run(
           Status(StatusCode::TEMPORARY_ERROR, "Background fetch in progress"));
     }
     return;
@@ skipping 68 matching lines @@
     Category category,
     const DismissedSuggestionsCallback& callback) {
   provider_->GetDismissedSuggestionsForDebugging(category, callback);
 }
 
 void SchedulingRemoteSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   provider_->ClearDismissedSuggestionsForDebugging(category);
 }
 
-void SchedulingRemoteSuggestionsProvider::OnProviderStatusChanged(
-    RemoteSuggestionsProvider::ProviderStatus status) {
-  switch (status) {
-    case RemoteSuggestionsProvider::ProviderStatus::ACTIVE:
-      StartScheduling();
-      return;
-    case RemoteSuggestionsProvider::ProviderStatus::INACTIVE:
-      StopScheduling();
-      return;
-  }
-  NOTREACHED();
-}
-
 void SchedulingRemoteSuggestionsProvider::StartScheduling() {
   FetchingSchedule new_schedule = GetDesiredFetchingSchedule();
 
   if (schedule_ == new_schedule) {
     // Do not schedule if nothing has changed;
     return;
   }
 
   schedule_ = new_schedule;
   StoreFetchingSchedule();
@@ skipping 92 matching lines @@
     case TriggerType::BROWSER_FOREGROUNDED:
     case TriggerType::BROWSER_COLD_START:
       first_allowed_fetch_time =
           last_fetch_attempt_time + schedule_.interval_soft_on_usage_event;
       break;
     case TriggerType::PERSISTENT_SCHEDULER_WAKE_UP:
     case TriggerType::COUNT:
       NOTREACHED();
       break;
   }
-  return first_allowed_fetch_time <= clock_->Now();
+  base::Time now = clock_->Now();
+
+  return background_fetches_allowed_after_ <= now &&
+         first_allowed_fetch_time <= now;
 }
 
 bool SchedulingRemoteSuggestionsProvider::BackgroundFetchesDisabled(
     SchedulingRemoteSuggestionsProvider::TriggerType trigger) const {
   if (schedule_.is_empty()) {
     return true;  // Background fetches are disabled in general.
   }
 
   if (enabled_triggers_.count(trigger) == 0) {
     return true;  // Background fetches for |trigger| are not enabled.
@@ skipping 29 matching lines @@
   // Reschedule after a fetch. The persistent schedule is applied only after a
   // successful fetch. After a failed fetch, we want to keep the previous
   // persistent schedule intact so that we eventually get a persistent
   // fallback fetch (if the wifi persistent fetches keep failing).
   if (fetch_status.code != StatusCode::SUCCESS) {
     return;
   }
   ApplyPersistentFetchingSchedule();
 }
 
+void SchedulingRemoteSuggestionsProvider::ClearLastFetchAttemptTime() {
+  pref_service_->ClearPref(prefs::kSnippetLastFetchAttempt);
+}
+
 std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
 SchedulingRemoteSuggestionsProvider::GetEnabledTriggerTypes() {
   static_assert(static_cast<unsigned int>(TriggerType::COUNT) ==
                     arraysize(kTriggerTypeNames),
                 "Fill in names for trigger types.");
 
   std::string param_value = base::GetFieldTrialParamValueByFeature(
       ntp_snippets::kArticleSuggestionsFeature, kTriggerTypesParamName);
   if (param_value == kTriggerTypesParamValueForEmptyList) {
     return std::set<TriggerType>();
@@ skipping 26 matching lines @@
   return enabled_types;
 }
 
 std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
 SchedulingRemoteSuggestionsProvider::GetDefaultEnabledTriggerTypes() {
   return {TriggerType::PERSISTENT_SCHEDULER_WAKE_UP, TriggerType::NTP_OPENED,
           TriggerType::BROWSER_FOREGROUNDED};
 }
 
 }  // namespace ntp_snippets