[Remote suggestions] Prioritize wifi for soft fetches.

components/ntp_snippets/remote/remote_suggestions_scheduler_impl.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/remote_suggestions_scheduler_impl.h"
 
 #include <random>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #include "base/time/clock.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/ntp_snippets/status.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "net/base/network_change_notifier.h"
 
 namespace ntp_snippets {
 
 namespace {
 
 // The FetchingInterval enum specifies overlapping time intervals that are used
 // for scheduling the next remote suggestion fetch. Therefore a timer is created
 // for each interval. Initially all the timers are started at the same time.
 // Fetches are
 // only performed when certain conditions associated with the intervals are
 // met. If a fetch failed, then only the corresponding timer is reset. The
 // other timers are not touched.
 // TODO(markusheintz): Describe the individual intervals.
 enum class FetchingInterval {
   PERSISTENT_FALLBACK,
   PERSISTENT_WIFI,
-  SOFT_ON_USAGE_EVENT,
-  SOFT_ON_NTP_OPENED,
+  SOFT_FALLBACK,
+  SOFT_WIFI,
   COUNT
 };
 
 // The following arrays specify default values for remote suggestions fetch
 // intervals corresponding to individual user classes. The user classes are
 // defined by the user classifier. There must be an array for each user class.
 // The values of each array specify a default time interval for the intervals
 // defined by the enum FetchingInterval. The default time intervals defined in
 // the arrays can be overridden using different variation parameters.
 const double kDefaultFetchingIntervalHoursRareNtpUser[] = {48.0, 24.0, 12.0,
                                                            6.0};
-const double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 2.0,
+const double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 4.0,

[Marc Treib, 2017/04/04 13:28:16]

Any particular reason for this change?

[jkrcal, 2017/04/05 17:32:56]

Yes, for two reasons:
 - I would like to be cautious about data we use for our users in EM (and in
general);
 - We probably need to turn down the traffic a bit. I will adapt the defaults
once again, close to BP to reflect the situation in M57 Stable.

[Marc Treib, 2017/04/06 08:29:23]

Then, arguably, all the changes to the defaults should happen in that follow-up
CL. But I guess it doesn't really matter.

[jkrcal, 2017/04/06 12:04:07]

Acknowledged.

                                                              2.0};
 const double kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[] = {
     24.0, 6.0, 2.0, 1.0};
 
 // Variation parameters than can be used to override the default fetching
 // intervals.
 const char* kFetchingIntervalParamNameRareNtpUser[] = {
     "fetching_interval_hours-fallback-rare_ntp_user",
     "fetching_interval_hours-wifi-rare_ntp_user",
-    "soft_fetching_interval_hours-active-rare_ntp_user",
-    "soft_on_ntp_opened_interval_hours-rare_ntp_user"};

[Marc Treib, 2017/04/04 13:28:15]

Removing the existing params means we'll have to update the Finch configs at the
correct time. Recent experience shows that we're bad at this. Do we have any
plans in place to make sure we don't miss it this time?

[jkrcal, 2017/04/05 17:32:56]

I'll update the current finch configs asap and create a new Beta config for M59
to reflect this change.

+    "soft_fetching_interval_hours-fallback-rare_ntp_user",
+    "soft_fetching_interval_hours-wifi-rare_ntp_user"};
 const char* kFetchingIntervalParamNameActiveNtpUser[] = {
     "fetching_interval_hours-fallback-active_ntp_user",
     "fetching_interval_hours-wifi-active_ntp_user",
-    "soft_fetching_interval_hours-active-active_ntp_user",
-    "soft_on_ntp_opened_interval_hours-active_ntp_user"};
+    "soft_fetching_interval_hours-fallback-active_ntp_user",
+    "soft_fetching_interval_hours-wifi-active_ntp_user"};
 const char* kFetchingIntervalParamNameActiveSuggestionsConsumer[] = {
     "fetching_interval_hours-fallback-active_suggestions_consumer",
     "fetching_interval_hours-wifi-active_suggestions_consumer",
-    "soft_fetching_interval_hours-active-active_suggestions_consumer",
-    "soft_on_ntp_opened_interval_hours-active_suggestions_consumer"};
+    "soft_fetching_interval_hours-fallback-active_suggestions_consumer",
+    "soft_fetching_interval_hours-wifi-active_suggestions_consumer"};
 
 static_assert(
     static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kDefaultFetchingIntervalHoursRareNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kDefaultFetchingIntervalHoursActiveNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kDefaultFetchingIntervalHoursActiveSuggestionsConsumer) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kFetchingIntervalParamNameRareNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kFetchingIntervalParamNameActiveNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             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;
 
+const char kSnippetSoftFetchingIntervalOnUsageEventDeprecated[] =
+    "ntp_snippets.soft_fetching_interval_on_usage_event";
+const char kSnippetSoftFetchingIntervalOnNtpOpenedDeprecated[] =
+    "ntp_snippets.soft_fetching_interval_on_ntp_opened";

[Marc Treib, 2017/04/04 13:28:15]

I recently learned of a good place for that kind of thing:
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?rc...

(optional; it's also fine to keep it here)

[jkrcal, 2017/04/05 17:32:56]

Ah, interesting, thanks for the pointer!
I'd keep it here for this time (so that I do not need lgtm from a browser
OWNER)...

[Marc Treib, 2017/04/06 08:29:23]

FWIW, bauerb@ and battre@ are both owners, so no timezone roundtrips required.
(But it's still fine to keep this here.)

[jkrcal, 2017/04/06 12:04:08]

Acknowledged.

+
 // 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 14 matching lines @@
       break;
   }
 
   double value_hours = base::GetFieldTrialParamByFeatureAsDouble(
       ntp_snippets::kArticleSuggestionsFeature, param_name,
       default_value_hours);
 
   return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
 }
 
+bool IsOnWifi() {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  // Return true for Wi-Fi (and any other likely unmetered connection type).

[Marc Treib, 2017/04/04 13:28:16]

Hm yeah, "wifi" is really a misnomer, also for the persistent scheduler. Hrm.

[jkrcal, 2017/04/05 17:32:56]

Yeah, I've updated the comments to explain what we mean by wifi.

+  return (connection_type ==
+              net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI ||
+          connection_type ==
+              net::NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET);
+}
+
 }  // namespace
 
 class EulaState : public web_resource::EulaAcceptedNotifier::Observer {
  public:
   EulaState(PrefService* local_state_prefs,
             RemoteSuggestionsScheduler* scheduler)
       : eula_notifier_(
             web_resource::EulaAcceptedNotifier::Create(local_state_prefs)),
         scheduler_(scheduler) {
     // EulaNotifier is not constructed on some platforms (such as desktop).
@@ skipping 31 matching lines @@
 RemoteSuggestionsSchedulerImpl::FetchingSchedule
 RemoteSuggestionsSchedulerImpl::FetchingSchedule::Empty() {
   return FetchingSchedule{base::TimeDelta(), base::TimeDelta(),
                           base::TimeDelta(), base::TimeDelta()};
 }
 
 bool RemoteSuggestionsSchedulerImpl::FetchingSchedule::operator==(
     const FetchingSchedule& other) const {
   return interval_persistent_wifi == other.interval_persistent_wifi &&
          interval_persistent_fallback == other.interval_persistent_fallback &&
-         interval_soft_on_usage_event == other.interval_soft_on_usage_event &&
-         interval_soft_on_ntp_opened == other.interval_soft_on_ntp_opened;
+         interval_soft_wifi == other.interval_soft_wifi &&
+         interval_soft_fallback == other.interval_soft_fallback;
 }
 
 bool RemoteSuggestionsSchedulerImpl::FetchingSchedule::operator!=(
     const FetchingSchedule& other) const {
   return !operator==(other);
 }
 
 bool RemoteSuggestionsSchedulerImpl::FetchingSchedule::is_empty() const {
   return interval_persistent_wifi.is_zero() &&
          interval_persistent_fallback.is_zero() &&
-         interval_soft_on_usage_event.is_zero() &&
-         interval_soft_on_ntp_opened.is_zero();
+         interval_soft_wifi.is_zero() && interval_soft_fallback.is_zero();
 }
 
 // The TriggerType enum specifies values for the events that can trigger
 // fetching remote suggestions. These values are written to logs. New enum
 // values can be added, but existing enums must never be renumbered or deleted
 // and reused. When adding new entries, also update the array
 // |kTriggerTypeNames| above.
 enum class RemoteSuggestionsSchedulerImpl::TriggerType {
   PERSISTENT_SCHEDULER_WAKE_UP = 0,
   NTP_OPENED = 1,
@@ skipping 24 matching lines @@
           profile_prefs,
           RequestThrottler::RequestType::
               CONTENT_SUGGESTION_FETCHER_ACTIVE_SUGGESTIONS_CONSUMER),
       eula_state_(base::MakeUnique<EulaState>(local_state_prefs, this)),
       profile_prefs_(profile_prefs),
       clock_(std::move(clock)),
       enabled_triggers_(GetEnabledTriggerTypes()) {
   DCHECK(user_classifier);
   DCHECK(profile_prefs);
 
+  // Cleanup procedure in M59. Remove for M60.

[Marc Treib, 2017/04/04 13:28:16]

I'd leave it for a bit longer, maybe M62?

[jkrcal, 2017/04/05 17:32:56]

Done.

+  profile_prefs_->ClearPref(kSnippetSoftFetchingIntervalOnUsageEventDeprecated);
+  profile_prefs_->ClearPref(kSnippetSoftFetchingIntervalOnNtpOpenedDeprecated);
+
   LoadLastFetchingSchedule();
 }
 
 RemoteSuggestionsSchedulerImpl::~RemoteSuggestionsSchedulerImpl() = default;
 
 // static
 void RemoteSuggestionsSchedulerImpl::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
   registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalWifi, 0);
   registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalFallback,
                               0);
-  registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
-                              0);
+  registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalWifi, 0);
+  registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalFallback, 0);
   registry->RegisterInt64Pref(prefs::kSnippetLastFetchAttempt, 0);
-  registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalOnNtpOpened,
+
+  // Cleanup procedure in M59. Remove for M60.
+  registry->RegisterInt64Pref(
+      kSnippetSoftFetchingIntervalOnUsageEventDeprecated, 0);
+  registry->RegisterInt64Pref(kSnippetSoftFetchingIntervalOnNtpOpenedDeprecated,
                               0);
 }
 
 void RemoteSuggestionsSchedulerImpl::SetProvider(
     RemoteSuggestionsProvider* provider) {
   DCHECK(provider);
   provider_ = provider;
 }
 
 void RemoteSuggestionsSchedulerImpl::OnProviderActivated() {
@@ skipping 38 matching lines @@
   OnFetchCompleted(fetch_status);
 }
 
 void RemoteSuggestionsSchedulerImpl::OnPersistentSchedulerWakeUp() {
   RefetchInTheBackgroundIfEnabled(TriggerType::PERSISTENT_SCHEDULER_WAKE_UP);
 }
 
 void RemoteSuggestionsSchedulerImpl::OnBrowserForegrounded() {
   // TODO(jkrcal): Consider that this is called whenever we open or return to an
   // Activity. Therefore, keep work light for fast start up calls.
-  if (!ShouldRefetchInTheBackgroundNow(TriggerType::BROWSER_FOREGROUNDED)) {
+  if (!ShouldRefetchInTheBackgroundNow()) {

[Marc Treib, 2017/04/04 13:28:16]

Doesn't this mean that all triggers will be treated the same now, i.e. a change
in behavior from what we have on stable now?

[jkrcal, 2017/04/05 17:32:56]

I've rewritten the code. Hope the structure is now clearer.

[Marc Treib, 2017/04/06 08:29:23]

Yup, much better, thanks!

     return;
   }
 
   RefetchInTheBackgroundIfEnabled(TriggerType::BROWSER_FOREGROUNDED);
 }
 
 void RemoteSuggestionsSchedulerImpl::OnBrowserColdStart() {
   // TODO(fhorschig|jkrcal): Consider that work here must be kept light for fast
   // cold start ups.
-  if (!ShouldRefetchInTheBackgroundNow(TriggerType::BROWSER_COLD_START)) {
+  if (!ShouldRefetchInTheBackgroundNow()) {
     return;
   }
 
   RefetchInTheBackgroundIfEnabled(TriggerType::BROWSER_COLD_START);
 }
 
 void RemoteSuggestionsSchedulerImpl::OnNTPOpened() {
-  if (!ShouldRefetchInTheBackgroundNow(TriggerType::NTP_OPENED)) {
+  if (!ShouldRefetchInTheBackgroundNow()) {
     return;
   }
 
   RefetchInTheBackgroundIfEnabled(TriggerType::NTP_OPENED);
 }
 
 void RemoteSuggestionsSchedulerImpl::StartScheduling() {
   FetchingSchedule new_schedule = GetDesiredFetchingSchedule();
 
   if (schedule_ == new_schedule) {
@@ skipping 31 matching lines @@
 
 RemoteSuggestionsSchedulerImpl::FetchingSchedule
 RemoteSuggestionsSchedulerImpl::GetDesiredFetchingSchedule() const {
   UserClassifier::UserClass user_class = user_classifier_->GetUserClass();
 
   FetchingSchedule schedule;
   schedule.interval_persistent_wifi =
       GetDesiredFetchingInterval(FetchingInterval::PERSISTENT_WIFI, user_class);
   schedule.interval_persistent_fallback = GetDesiredFetchingInterval(
       FetchingInterval::PERSISTENT_FALLBACK, user_class);
-  schedule.interval_soft_on_usage_event = GetDesiredFetchingInterval(
-      FetchingInterval::SOFT_ON_USAGE_EVENT, user_class);
-  schedule.interval_soft_on_ntp_opened = GetDesiredFetchingInterval(
-      FetchingInterval::SOFT_ON_NTP_OPENED, user_class);
+  schedule.interval_soft_wifi =
+      GetDesiredFetchingInterval(FetchingInterval::SOFT_WIFI, user_class);
+  schedule.interval_soft_fallback =
+      GetDesiredFetchingInterval(FetchingInterval::SOFT_FALLBACK, user_class);
 
   return schedule;
 }
 
 void RemoteSuggestionsSchedulerImpl::LoadLastFetchingSchedule() {
   schedule_.interval_persistent_wifi = base::TimeDelta::FromInternalValue(
       profile_prefs_->GetInt64(prefs::kSnippetPersistentFetchingIntervalWifi));
   schedule_.interval_persistent_fallback =
       base::TimeDelta::FromInternalValue(profile_prefs_->GetInt64(
           prefs::kSnippetPersistentFetchingIntervalFallback));
-  schedule_.interval_soft_on_usage_event =
-      base::TimeDelta::FromInternalValue(profile_prefs_->GetInt64(
-          prefs::kSnippetSoftFetchingIntervalOnUsageEvent));
-  schedule_.interval_soft_on_ntp_opened = base::TimeDelta::FromInternalValue(
-      profile_prefs_->GetInt64(prefs::kSnippetSoftFetchingIntervalOnNtpOpened));
+  schedule_.interval_soft_wifi = base::TimeDelta::FromInternalValue(
+      profile_prefs_->GetInt64(prefs::kSnippetSoftFetchingIntervalWifi));
+  schedule_.interval_soft_fallback = base::TimeDelta::FromInternalValue(
+      profile_prefs_->GetInt64(prefs::kSnippetSoftFetchingIntervalFallback));
 }
 
 void RemoteSuggestionsSchedulerImpl::StoreFetchingSchedule() {
   profile_prefs_->SetInt64(
       prefs::kSnippetPersistentFetchingIntervalWifi,
       schedule_.interval_persistent_wifi.ToInternalValue());
   profile_prefs_->SetInt64(
       prefs::kSnippetPersistentFetchingIntervalFallback,
       schedule_.interval_persistent_fallback.ToInternalValue());
-  profile_prefs_->SetInt64(
-      prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
-      schedule_.interval_soft_on_usage_event.ToInternalValue());
-  profile_prefs_->SetInt64(
-      prefs::kSnippetSoftFetchingIntervalOnNtpOpened,
-      schedule_.interval_soft_on_ntp_opened.ToInternalValue());
+  profile_prefs_->SetInt64(prefs::kSnippetSoftFetchingIntervalWifi,
+                           schedule_.interval_soft_wifi.ToInternalValue());
+  profile_prefs_->SetInt64(prefs::kSnippetSoftFetchingIntervalFallback,
+                           schedule_.interval_soft_fallback.ToInternalValue());
 }
 
 void RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundIfEnabled(
     TriggerType trigger) {
   if (BackgroundFetchesDisabled(trigger)) {
     return;
   }
 
   UMA_HISTOGRAM_ENUMERATION(
       "NewTabPage.ContentSuggestions.BackgroundFetchTrigger",
@@ skipping 10 matching lines @@
   if (!AcquireQuota(/*interactive_request=*/false)) {
     return;
   }
 
   background_fetch_in_progress_ = true;
   provider_->RefetchInTheBackground(base::Bind(
       &RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundFinished,
       base::Unretained(this)));
 }
 
-bool RemoteSuggestionsSchedulerImpl::ShouldRefetchInTheBackgroundNow(
-    TriggerType trigger) {
+bool RemoteSuggestionsSchedulerImpl::ShouldRefetchInTheBackgroundNow() {
   const base::Time last_fetch_attempt_time = base::Time::FromInternalValue(
       profile_prefs_->GetInt64(prefs::kSnippetLastFetchAttempt));
   base::Time first_allowed_fetch_time;
-  switch (trigger) {
-    case TriggerType::NTP_OPENED:
-      first_allowed_fetch_time =
-          last_fetch_attempt_time + schedule_.interval_soft_on_ntp_opened;
-      break;
-    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;
+
+  if (IsOnWifi()) {

[Marc Treib, 2017/04/04 13:28:15]

nit: IMO this would be simpler as an arithmetic if:
first_allowed_fetch_time = last_fetch_attempt_time + (IsOnWifi() ?
schedule_.interval_soft_wifi : schedule_.interval_soft_fallback);

Or, if you find that line too long:
first_allowed_fetch_time = last_fetch_attempt_time;
first_allowed_fetch_time += IsOnWifi() ? schedule_.interval_soft_wifi :
schedule_.interval_soft_fallback;

[jkrcal, 2017/04/05 17:32:56]

Done.

+    first_allowed_fetch_time =
+        last_fetch_attempt_time + schedule_.interval_soft_wifi;
+  } else {
+    first_allowed_fetch_time =
+        last_fetch_attempt_time + schedule_.interval_soft_fallback;
   }
+
   base::Time now = clock_->Now();
-
   return background_fetches_allowed_after_ <= now &&
          first_allowed_fetch_time <= now;
 }
 
 bool RemoteSuggestionsSchedulerImpl::BackgroundFetchesDisabled(
     TriggerType trigger) const {
   if (!provider_) {
     return true;  // Cannot fetch as remote suggestions provider does not exist.
   }
 
@@ skipping 91 matching lines @@
   return enabled_types;
 }
 
 std::set<RemoteSuggestionsSchedulerImpl::TriggerType>
 RemoteSuggestionsSchedulerImpl::GetDefaultEnabledTriggerTypes() {
   return {TriggerType::PERSISTENT_SCHEDULER_WAKE_UP, TriggerType::NTP_OPENED,
           TriggerType::BROWSER_FOREGROUNDED};
 }
 
 }  // namespace ntp_snippets