[remote suggestions] Add separte fetch interval for NTP open trigger

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
index 788ac534fe4bbe6901bd49e23bfeaf21c35f8e9a..c44fd035fab2a340344ff16a5826fd836a465c21 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
@@ -37,6 +37,7 @@ enum class FetchingInterval {
   PERSISTENT_FALLBACK,
   PERSISTENT_WIFI,
   SOFT_ON_USAGE_EVENT,
+  NTP_OPEN,

[jkrcal, 2017/02/15 16:35:34]

nit: please update consistently with the FetchingSchedule discussion

[markusheintz_, 2017/02/15 21:19:29]

Done.

   COUNT
 };
 
@@ -46,25 +47,30 @@ enum class FetchingInterval {
 // 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};
-const double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 2.0};
+const double kDefaultFetchingIntervalHoursRareNtpUser[] = {48.0, 24.0, 12.0,
+                                                           6.0};
+const double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 2.0,
+                                                             2.0};
 const double kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[] = {
-    24.0, 6.0, 2.0};
+    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_fetching_interval_hours-active-rare_ntp_user",
+    "ntp_open_interval_hours-rare_ntp_user"};

[jkrcal, 2017/02/15 16:35:34]

nit: please update consistently with the FetchingSchedule discussion

[markusheintz_, 2017/02/15 21:19:29]

Done.

 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_fetching_interval_hours-active-active_ntp_user",
+    "ntp_open_interval_hours-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_fetching_interval_hours-active-active_suggestions_consumer",
+    "ntp_open_interval_hours-active_suggestions_consumer"};
 
 static_assert(
     static_cast<unsigned int>(FetchingInterval::COUNT) ==
@@ -220,7 +226,7 @@ void SchedulingRemoteSuggestionsProvider::OnPersistentSchedulerWakeUp() {
 void SchedulingRemoteSuggestionsProvider::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()) {
+  if (!ShouldRefetchInTheBackgroundNow(TriggerType::BROWSER_FOREGROUNDED)) {
     return;
   }
 
@@ -230,7 +236,7 @@ void SchedulingRemoteSuggestionsProvider::OnBrowserForegrounded() {
 void SchedulingRemoteSuggestionsProvider::OnBrowserColdStart() {
   // TODO(fhorschig|jkrcal): Consider that work here must be kept light for fast
   // cold start ups.
-  if (!ShouldRefetchInTheBackgroundNow()) {
+  if (!ShouldRefetchInTheBackgroundNow(TriggerType::BROWSER_COLD_START)) {
     return;
   }
 
@@ -238,7 +244,7 @@ void SchedulingRemoteSuggestionsProvider::OnBrowserColdStart() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
-  if (!ShouldRefetchInTheBackgroundNow()) {
+  if (!ShouldRefetchInTheBackgroundNow(TriggerType::NTP_OPENED)) {
     return;
   }
 
@@ -396,6 +402,8 @@ SchedulingRemoteSuggestionsProvider::GetDesiredFetchingSchedule() const {
       FetchingInterval::PERSISTENT_FALLBACK, user_class);
   schedule.interval_soft_on_usage_event = GetDesiredFetchingInterval(
       FetchingInterval::SOFT_ON_USAGE_EVENT, user_class);
+  schedule.interval_ntp_open =
+      GetDesiredFetchingInterval(FetchingInterval::NTP_OPEN, user_class);
 
   return schedule;
 }
@@ -434,11 +442,25 @@ void SchedulingRemoteSuggestionsProvider::RefetchInTheBackgroundIfEnabled(
   RefetchInTheBackground(/*callback=*/nullptr);
 }
 
-bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow() {
-  base::Time first_allowed_fetch_time =
-      base::Time::FromInternalValue(
-          pref_service_->GetInt64(prefs::kSnippetLastFetchAttempt)) +
-      schedule_.interval_soft_on_usage_event;
+bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow(
+    SchedulingRemoteSuggestionsProvider::TriggerType trigger) {
+  final base::Time last_fetch_attempt_time = base::Time::FromInternalValue(
+      pref_service_->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_ntp_open;
+      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:

[markusheintz_, 2017/02/15 21:19:29]

Changed to default in order to prevent having to use case COUNT:

[jkrcal, 2017/02/16 07:32:54]

I would actually rather have both of them listed here. This way, when you later
add a new entry into the enum, the compiler will force you to add the entry here
as well.

Not a big deal because if you forget, it will crash here anyway in a debug build
so I do not feel strongly.

[markusheintz_, 2017/02/16 13:45:53]

Done

+      NOTREACHED();
+      break;
+  }
   return first_allowed_fetch_time <= clock_->Now();
 }