[Remote suggestions] Prioritize wifi for soft fetches.

components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc

Index: components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
index 253d41593190acab34598994a9ba3d83b099f069..5b9739dcf47fcf8b275fdef9f8e4b23e988f136a 100644
--- a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
@@ -22,6 +22,7 @@
 #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 {
 
@@ -38,8 +39,8 @@ namespace {
 enum class FetchingInterval {
   PERSISTENT_FALLBACK,
   PERSISTENT_WIFI,
-  SOFT_ON_USAGE_EVENT,
-  SOFT_ON_NTP_OPENED,
+  SOFT_FALLBACK,
+  SOFT_WIFI,
   COUNT
 };
 
@@ -51,7 +52,7 @@ enum class FetchingInterval {
 // 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};
@@ -61,18 +62,18 @@ const double kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[] = {
 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) ==
@@ -98,6 +99,11 @@ 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(
@@ -132,6 +138,16 @@ base::TimeDelta GetDesiredFetchingInterval(
   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 {
@@ -183,8 +199,8 @@ 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!=(
@@ -195,8 +211,7 @@ bool RemoteSuggestionsSchedulerImpl::FetchingSchedule::operator!=(
 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
@@ -241,6 +256,10 @@ RemoteSuggestionsSchedulerImpl::RemoteSuggestionsSchedulerImpl(
   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();
 }
 
@@ -252,10 +271,14 @@ void RemoteSuggestionsSchedulerImpl::RegisterProfilePrefs(
   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);
 }
 
@@ -314,7 +337,7 @@ void RemoteSuggestionsSchedulerImpl::OnPersistentSchedulerWakeUp() {
 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;
   }
 
@@ -324,7 +347,7 @@ void RemoteSuggestionsSchedulerImpl::OnBrowserForegrounded() {
 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;
   }
 
@@ -332,7 +355,7 @@ void RemoteSuggestionsSchedulerImpl::OnBrowserColdStart() {
 }
 
 void RemoteSuggestionsSchedulerImpl::OnNTPOpened() {
-  if (!ShouldRefetchInTheBackgroundNow(TriggerType::NTP_OPENED)) {
+  if (!ShouldRefetchInTheBackgroundNow()) {
     return;
   }
 
@@ -384,10 +407,10 @@ RemoteSuggestionsSchedulerImpl::GetDesiredFetchingSchedule() const {
       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;
 }
@@ -398,11 +421,10 @@ void RemoteSuggestionsSchedulerImpl::LoadLastFetchingSchedule() {
   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() {
@@ -412,12 +434,10 @@ void RemoteSuggestionsSchedulerImpl::StoreFetchingSchedule() {
   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(
@@ -448,28 +468,20 @@ void RemoteSuggestionsSchedulerImpl::RefetchInTheBackground() {
       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();
 
+  base::Time now = clock_->Now();
   return background_fetches_allowed_after_ <= now &&
          first_allowed_fetch_time <= now;
 }