[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_scheduler.cc

Index: components/ntp_snippets/remote/remote_suggestions_scheduler.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_scheduler.cc b/components/ntp_snippets/remote/remote_suggestions_scheduler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7d54f537722cdab84e3aeffc1f74fb6cb9200a14
--- /dev/null
+++ b/components/ntp_snippets/remote/remote_suggestions_scheduler.cc
@@ -0,0 +1,189 @@
+// 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/remote_suggestions_scheduler.h"
+
+#include <string>
+
+#include "components/ntp_snippets/features.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h"
+#include "components/ntp_snippets/user_classifier.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "components/variations/variations_associated_data.h"
+
+namespace ntp_snippets {
+
+namespace {
+
+// Default values for fetching intervals, fallback and wifi.
+const double kDefaultFetchingIntervalRareNtpUser[] = {48.0, 24.0};
+const double kDefaultFetchingIntervalActiveNtpUser[] = {24.0, 6.0};
+const double kDefaultFetchingIntervalActiveSuggestionsConsumer[] = {24.0, 6.0};
+
+// Variation parameters than can override the default fetching intervals.
+const char* kFetchingIntervalParamNameRareNtpUser[] = {
+    "fetching_interval_hours-fallback-rare_ntp_user",
+    "fetching_interval_hours-wifi-rare_ntp_user"};
+const char* kFetchingIntervalParamNameActiveNtpUser[] = {
+    "fetching_interval_hours-fallback-active_ntp_user",
+    "fetching_interval_hours-wifi-active_ntp_user"};
+const char* kFetchingIntervalParamNameActiveSuggestionsConsumer[] = {
+    "fetching_interval_hours-fallback-active_suggestions_consumer",
+    "fetching_interval_hours-wifi-active_suggestions_consumer"};
+
+base::TimeDelta GetCurrentUpdateInterval(
+    bool is_wifi,
+    UserClassifier::UserClass user_class) {
+  double default_value_hours = 0.0;
+
+  const int index = is_wifi ? 1 : 0;
+  const char* param_name = "";

[Marc Treib, 2016/12/09 12:25:27]

nullptr? The "empty" case can't happen anyway

[jkrcal, 2016/12/14 09:42:10]

Done.

+  switch (user_class) {
+    case UserClassifier::UserClass::RARE_NTP_USER:
+      default_value_hours = kDefaultFetchingIntervalRareNtpUser[index];
+      param_name = kFetchingIntervalParamNameRareNtpUser[index];
+      break;
+    case UserClassifier::UserClass::ACTIVE_NTP_USER:
+      default_value_hours = kDefaultFetchingIntervalActiveNtpUser[index];
+      param_name = kFetchingIntervalParamNameActiveNtpUser[index];
+      break;
+    case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
+      default_value_hours =
+          kDefaultFetchingIntervalActiveSuggestionsConsumer[index];
+      param_name = kFetchingIntervalParamNameActiveSuggestionsConsumer[index];
+      break;
+  }
+
+  double value_hours = variations::GetVariationParamByFeatureAsDouble(
+      ntp_snippets::kArticleSuggestionsFeature, param_name,
+      default_value_hours);
+
+  return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
+}
+
+}  // namespace
+
+RemoteSuggestionsScheduler::RemoteSuggestionsScheduler(
+    RemoteSuggestionsHardScheduler* hard_scheduler,
+    const UserClassifier* user_classifier,
+    PrefService* pref_service)
+    : hard_scheduler_(hard_scheduler),
+      updater_(nullptr),
+      user_classifier_(user_classifier),
+      pref_service_(pref_service) {
+  DCHECK(user_classifier) << "non-null UserClassifier must be provided";
+  DCHECK(pref_service) << "non-null pref service is needed";
+}
+
+RemoteSuggestionsScheduler::~RemoteSuggestionsScheduler() = default;
+
+void RemoteSuggestionsScheduler::SetUpdater(Updater* updater) {
+  DCHECK(updater) << "non-null Updater must be provided";

[Marc Treib, 2016/12/09 12:25:27]

Since we have to handle a null updater_ anyway, maybe this isn't needed?

[jkrcal, 2016/12/14 09:42:10]

Done.

+  updater_ = updater;
+}
+
+// static
+void RemoteSuggestionsScheduler::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalWifi, 0);
+  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalFallback,
+                              0);
+}
+
+void RemoteSuggestionsScheduler::PerformHardUpdate() {

[tschumann, 2016/12/09 17:27:28]

i'm a bit confused. Does this now perform an update or simply schedule one?

[jkrcal, 2016/12/14 09:42:09]

It performs it. Is is clearer now in the new design? (OnFetchDue())

+  if (!updater_) {
+    DLOG(WARNING) << "An update is to be performed without having the updater.";
+    return;
+  }
+
+  updater_->UpdateRemoteSuggestionsBySchedule();

[Marc Treib, 2016/12/09 12:25:27]

Hm. So in practice, the OS-scheduled update gets here via
ContentSuggestionsService->remote_suggestions_provider()->scheduler(), and this
effectively calls back into the RemoteSuggestionsProvider.
Should we get rid of this roundtrip through here?

[markusheintz_, 2016/12/12 14:36:16]

+1 this hides a simple call and somehow implies that there needs some special
work to be done for the hard updates. Which does not seem to be the case. 

[jkrcal, 2016/12/14 09:42:10]

Done.

+}
+
+void RemoteSuggestionsScheduler::Schedule() {
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (!hard_scheduler_) {
+    return;
+  }
+
+  UpdateSchedule last_schedule = GetLastUpdateSchedule();
+  UpdateSchedule schedule = GetCurrentUpdateSchedule();
+
+  // Reset the schedule only if the parameters have changed.
+  if (last_schedule.interval_wifi != schedule.interval_wifi ||
+      last_schedule.interval_fallback != schedule.interval_fallback) {

[Marc Treib, 2016/12/09 12:25:26]

optional: Could put operator==/!= on the struct? IMO that'd improve changes that
this will be updated when a member is added.

[jkrcal, 2016/12/14 09:42:10]

Done.

+    ResetUpdateScheduleFromNow(schedule);
+  }
+}
+
+void RemoteSuggestionsScheduler::Unschedule() {
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (!hard_scheduler_) {
+    return;
+  }
+
+  // Do not unschedule if already switched off
+  UpdateSchedule last_schedule = GetLastUpdateSchedule();
+  if (last_schedule.interval_fallback.is_zero() &&
+      last_schedule.interval_fallback.is_zero()) {

[Marc Treib, 2016/12/09 12:25:27]

Similar here, could put an accessor on the struct.
Also, you probably don't want to check interval_fallback twice :)  (Should we
have a test to catch this?)

[jkrcal, 2016/12/14 09:42:10]

Done.

+    return;
+  }
+
+  hard_scheduler_->Unschedule();
+
+  StoreLastUpdateSchedule(UpdateSchedule{base::TimeDelta::FromHours(0),
+                                         base::TimeDelta::FromHours(0)});

[Marc Treib, 2016/12/09 12:25:26]

Maybe make an UpdateSchedule::Empty() ?

[jkrcal, 2016/12/14 09:42:10]

Done.

+}
+
+void RemoteSuggestionsScheduler::OnSuccessfulUpdate() {
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (!hard_scheduler_) {
+    return;
+  }
+
+  ResetUpdateScheduleFromNow(GetLastUpdateSchedule());
+}
+
+void RemoteSuggestionsScheduler::ResetUpdateScheduleFromNow(

[tschumann, 2016/12/09 17:27:28]

the 'fromNow' suffix seems superfluous.
Actually, I find it super hard to figure out what this function does from just
looking at it's code...

[jkrcal, 2016/12/14 09:42:10]

Renamed to ApplyFetchingSchedule(). WDYT?

+    UpdateSchedule schedule) {
+  hard_scheduler_->Schedule(schedule.interval_wifi, schedule.interval_fallback);
+
+  StoreLastUpdateSchedule(schedule);
+}
+
+RemoteSuggestionsScheduler::UpdateSchedule
+RemoteSuggestionsScheduler::GetCurrentUpdateSchedule() {

[Marc Treib, 2016/12/09 12:25:27]

Hm, IMO this name is a bit misleading. I'd interpret it as "give me the values
that are currently scheduled".

[jkrcal, 2016/12/14 09:42:10]

Renamed to GetDesiredUpdateSchedule(). Better?

[Marc Treib, 2016/12/14 10:47:01]

Yup, thanks!

+  UserClassifier::UserClass user_class = user_classifier_->GetUserClass();
+
+  UpdateSchedule schedule;
+  schedule.interval_wifi =
+      GetCurrentUpdateInterval(/*is_wifi=*/true, user_class);
+  schedule.interval_fallback =
+      GetCurrentUpdateInterval(/*is_wifi=*/false, user_class);
+  return schedule;
+}
+
+RemoteSuggestionsScheduler::UpdateSchedule
+RemoteSuggestionsScheduler::GetLastUpdateSchedule() {
+  UpdateSchedule schedule;
+  schedule.interval_wifi = base::TimeDelta::FromInternalValue(
+      pref_service_->GetInt64(prefs::kSnippetBackgroundFetchingIntervalWifi));
+  schedule.interval_fallback =
+      base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
+          prefs::kSnippetBackgroundFetchingIntervalFallback));
+  return schedule;
+}
+
+void RemoteSuggestionsScheduler::StoreLastUpdateSchedule(
+    RemoteSuggestionsScheduler::UpdateSchedule schedule) {
+  pref_service_->SetInt64(
+      prefs::kSnippetBackgroundFetchingIntervalWifi,
+      schedule.interval_wifi.ToInternalValue());
+  pref_service_->SetInt64(
+      prefs::kSnippetBackgroundFetchingIntervalFallback,
+      schedule.interval_fallback.ToInternalValue());
+}
+
+}  // namespace ntp_snippets