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

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
index d4ec38e483049b7edb8a35507ff5fd1a9ddb778f..08a14db3ac4845887c8bec04803c98b48b472d18 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
@@ -95,6 +95,8 @@ const char* kTriggerTypeNames[] = {"persistent_scheduler_wake_up", "ntp_opened",
 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(
@@ -192,11 +194,7 @@ SchedulingRemoteSuggestionsProvider::SchedulingRemoteSuggestionsProvider(
 
   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() =
@@ -215,6 +213,31 @@ void SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
                               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();
@@ -253,9 +276,9 @@ void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
   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(
@@ -344,19 +367,6 @@ void SchedulingRemoteSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
   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();
 
@@ -469,7 +479,10 @@ bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow(
       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(
@@ -519,6 +532,10 @@ void SchedulingRemoteSuggestionsProvider::OnFetchCompleted(
   ApplyPersistentFetchingSchedule();
 }
 
+void SchedulingRemoteSuggestionsProvider::ClearLastFetchAttemptTime() {
+  pref_service_->ClearPref(prefs::kSnippetLastFetchAttempt);
+}
+
 std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
 SchedulingRemoteSuggestionsProvider::GetEnabledTriggerTypes() {
   static_assert(static_cast<unsigned int>(TriggerType::COUNT) ==