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

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 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/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 176 matching lines @@
   database_->LoadSnippets(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
                                      base::Unretained(this)));
 }
 
 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);
 }
 
 void NTPSnippetsService::FetchSnippets(bool interactive_request) {
   if (ready())
     FetchSnippetsFromHosts(GetSuggestionsHosts(), interactive_request);
   else
     fetch_when_ready_ = true;
 }
@@ skipping 16 matching lines @@
   for (const auto& item : categories_) {
     const CategoryContent& content = item.second;
     for (const auto& snippet : content.dismissed)
       excluded_ids.insert(snippet->id());
   }
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
                                             excluded_ids, kMaxSnippetCount,
                                             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);
+    }
   }
 }
 
 CategoryStatus NTPSnippetsService::GetCategoryStatus(Category category) {
   DCHECK(categories_.find(category) != categories_.end());
   return categories_[category].status;
 }
 
 CategoryInfo NTPSnippetsService::GetCategoryInfo(Category category) {
   DCHECK(categories_.find(category) != categories_.end());
@@ skipping 296 matching lines @@
   }
 
   // TODO(sfiera): notify only when a category changed above.
   NotifyNewSuggestions();
 
   // Reschedule after a successful fetch. This resets all currently scheduled
   // fetches, to make sure the fallback interval triggers only if no wifi fetch
   // 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,
                                        NTPSnippet::PtrVector new_snippets) {
   DCHECK(ready());
   CategoryContent* content = &categories_[category];
 
   // Remove new snippets that we already have, or that have been dismissed.
   std::set<std::string> old_snippet_ids;
   InsertAllIDs(content->dismissed, &old_snippet_ids);
@@ skipping 371 matching lines @@
       break;
 
     case State::ERROR_OCCURRED:
       DVLOG(1) << "Entering state: ERROR_OCCURRED";
       state_ = State::ERROR_OCCURRED;
       EnterStateError();
       break;
   }
 
   // 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() {
   for (const auto& item : categories_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
 
     std::vector<ContentSuggestion> result;
     for (const std::unique_ptr<NTPSnippet>& snippet : content.snippets) {
       // TODO(sfiera): if a snippet is not going to be displayed, move it
@@ skipping 41 matching lines @@
 }
 
 NTPSnippetsService::CategoryContent::CategoryContent() = default;
 NTPSnippetsService::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 NTPSnippetsService::CategoryContent::~CategoryContent() = default;
 NTPSnippetsService::CategoryContent& NTPSnippetsService::CategoryContent::
 operator=(CategoryContent&&) = default;
 
 }  // namespace ntp_snippets