[NTP Snippets] Don't reschedule background fetching on every startup

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index a4d9cc01612a103ae0df98fefdd51b62c7c66294..e51e9b79bb5c442aa1109ee13b77c23de48b944a 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -194,6 +194,9 @@ NTPSnippetsService::~NTPSnippetsService() {
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippetHosts);
+  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalWifi, 0);
+  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalFallback,
+                              0);
 
   NTPSnippetsStatusService::RegisterProfilePrefs(registry);
 }
@@ -230,21 +233,40 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
                                             interactive_request);
 }
 
-void NTPSnippetsService::RescheduleFetching() {
+void NTPSnippetsService::RescheduleFetching(bool force) {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
-  // If we're NOT_INITED, we don't know whether to schedule or un-schedule.
-  // We'll reschedule on the next state change anyway, so do nothing here.
-  if (state_ == State::NOT_INITED)
-    return;
-
   if (ready()) {
-    scheduler_->Schedule(GetFetchingIntervalWifi(),
-                         GetFetchingIntervalFallback());
+    base::TimeDelta old_interval_wifi =
+        base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
+            prefs::kSnippetBackgroundFetchingIntervalWifi));
+    base::TimeDelta old_interval_fallback =
+        base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
+            prefs::kSnippetBackgroundFetchingIntervalFallback));
+    base::TimeDelta interval_wifi = GetFetchingIntervalWifi();
+    base::TimeDelta interval_fallback = GetFetchingIntervalFallback();
+    if (force || interval_wifi != old_interval_wifi ||
+        interval_fallback != old_interval_fallback) {

[jkrcal, 2016/09/23 10:28:34]

What if:
 - the current fallback is scheduled, say, to 24h,
 - it is right after upgrade, i.e. the pref has the default value 0,
 - the new fallback interval is set to be 0?
 (and the same for the wifi interval)

To me it looks it ends up doing nothing. It should reschedule (in fact
unschedule) the fallback. Hm?

It is a bit niche, still cannot you make the default values for the prefs be -1
or MAX_INT?

[Marc Treib, 2016/09/23 11:19:13]

Prefs do survive the upgrade, so they won't be reset to their default value.
The actual scheduled tasks don't survive, so right now we might end up in a
situation where we think (via prefs) that things are scheduled, but they're
actually not. That will be addressed by the next CL when I implement
GcmTaskService.onInitializeTasks, see
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
Per above, I think having 0 (= "not scheduled") as the default makes sense.

[jkrcal, 2016/09/23 11:24:53]

I see, thanks!

+      scheduler_->Schedule(interval_wifi, interval_fallback);
+      pref_service_->SetInt64(prefs::kSnippetBackgroundFetchingIntervalWifi,
+                              interval_wifi.ToInternalValue());
+      pref_service_->SetInt64(
+          prefs::kSnippetBackgroundFetchingIntervalFallback,
+          interval_fallback.ToInternalValue());
+    }
   } else {
-    scheduler_->Unschedule();
+    // If we're NOT_INITED, we don't know whether to schedule or un-schedule.
+    // If |force| is false, all is well: We'll reschedule on the next state
+    // change anyway. If it's true, then unschedule here, to make sure that the
+    // next reschedule actually happens.
+    if (state_ != State::NOT_INITED || force) {
+      scheduler_->Unschedule();
+      pref_service_->ClearPref(prefs::kSnippetBackgroundFetchingIntervalWifi);
+      pref_service_->ClearPref(
+          prefs::kSnippetBackgroundFetchingIntervalFallback);
+    }
   }
 }
 
@@ -561,7 +583,7 @@ void NTPSnippetsService::OnFetchFinished(
   // succeeded, and also that we don't do a background fetch immediately after
   // a user-initiated one.
   if (snippets)
-    RescheduleFetching();
+    RescheduleFetching(true);
 }
 
 void NTPSnippetsService::MergeSnippets(Category category,
@@ -953,10 +975,7 @@ void NTPSnippetsService::EnterState(State state) {
   }
 
   // Schedule or un-schedule background fetching after each state change.
-  // TODO(treib): This resets all currently scheduled fetches on each Chrome
-  // start. Maybe store the currently scheduled values in prefs, and only
-  // reschedule if they have changed? crbug.com/646842
-  RescheduleFetching();
+  RescheduleFetching(false);
 }
 
 void NTPSnippetsService::NotifyNewSuggestions() {