[Feed] Add checks to ShouldRefresh().

Bug: 831648
Change-Id: I37ce716d7529a9330ff8fa566c1b5ab686bc78d4
Reviewed-on: https://chromium-review.googlesource.com/1128382Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576073}

chrome/browser/android/feed/feed_host_service_factory.cc

@@ -10,6 +10,7 @@
 
 #include "base/files/file_path.h"
 #include "base/time/default_clock.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/suggestions/image_decoder_impl.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
@@ -84,7 +85,8 @@ KeyedService* FeedHostServiceFactory::BuildServiceInstanceFor(
       std::move(image_fetcher), std::move(image_database));
 
   auto scheduler_host = std::make_unique<FeedSchedulerHost>(
-      profile->GetPrefs(), base::DefaultClock::GetInstance());
+      profile->GetPrefs(), g_browser_process->local_state(),
+      base::DefaultClock::GetInstance());
 
   auto storage_database = std::make_unique<FeedStorageDatabase>(feed_dir);
 

components/feed/DEPS

@@ -6,6 +6,7 @@ include_rules = [
   "+components/prefs",
   "+components/variations",
   "+components/version_info",
+  "+components/web_resource",
   "+net/base",
   "+net/http",
   "+net/traffic_annotation",

components/feed/core/BUILD.gn

@@ -44,6 +44,7 @@ source_set("feed_core") {
     "//components/variations",
     "//components/variations/net",
     "//components/variations/service",
+    "//components/web_resource",
     "//google_apis",
     "//services/identity/public/cpp",
     "//services/network/public/cpp",
@@ -78,6 +79,7 @@ source_set("core_unit_tests") {
     "//components/leveldb_proto:test_support",
     "//components/prefs:test_support",
     "//components/variations:test_support",
+    "//components/web_resource",
     "//net:test_support",
     "//services/identity/public/cpp",
     "//services/identity/public/cpp:test_support",

components/feed/core/feed_scheduler_host.cc

@@ -7,9 +7,11 @@
 #include <map>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/metrics/field_trial_params.h"
 #include "base/stl_util.h"
+#include "base/strings/string_split.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "components/feed/core/pref_names.h"
@@ -17,6 +19,8 @@
 #include "components/feed/feed_feature_list.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/web_resource/web_resource_pref_names.h"
+#include "net/base/network_change_notifier.h"
 
 namespace feed {
 
@@ -30,6 +34,11 @@ struct ParamPair {
   double default_value;
 };
 
+const base::FeatureParam<int> kSuppressRefreshDurationMinutes{
+    &kInterestFeedContentSuggestions, "suppress_refresh_duration_minutes", 30};
+const base::FeatureParam<std::string> kDisableTriggerTypes{
+    &kInterestFeedContentSuggestions, "disable_trigger_types", ""};
+
 // The Cartesian product of TriggerType and UserClass each need a different
 // param name in case we decide to change it via a config change. This nested
 // switch lookup ensures that all combinations are defined, along with a
@@ -44,6 +53,9 @@ ParamPair LookupParam(UserClass user_class, TriggerType trigger) {
           return {"foregrounded_hours_rare_ntp_user", 24.0};
         case TriggerType::FIXED_TIMER:
           return {"fixed_timer_hours_rare_ntp_user", 96.0};
+        case TriggerType::COUNT:
+          NOTREACHED();
+          return {"", 0.0};
       }
     case UserClass::ACTIVE_NTP_USER:
       switch (trigger) {
@@ -53,6 +65,9 @@ ParamPair LookupParam(UserClass user_class, TriggerType trigger) {
           return {"foregrounded_hours_active_ntp_user", 24.0};
         case TriggerType::FIXED_TIMER:
           return {"fixed_timer_hours_active_ntp_user", 48.0};
+        case TriggerType::COUNT:
+          NOTREACHED();
+          return {"", 0.0};
       }
     case UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
       switch (trigger) {
@@ -62,10 +77,44 @@ ParamPair LookupParam(UserClass user_class, TriggerType trigger) {
           return {"foregrounded_hours_active_suggestions_consumer", 12.0};
         case TriggerType::FIXED_TIMER:
           return {"fixed_timer_hours_active_suggestions_consumer", 24.0};
+        case TriggerType::COUNT:
+          NOTREACHED();
+          return {"", 0.0};
       }
   }
 }
 
+// Coverts from base::StringPiece to TriggerType and adds it to the set if the
+// trigger is recognized. Otherwise it is ignored.
+void TryAddTriggerType(base::StringPiece trigger_as_string_piece,
+                       std::set<TriggerType>* set) {
+  static_assert(static_cast<unsigned int>(TriggerType::COUNT) == 3,
+                "New TriggerTypes must be handled below.");
+  if (trigger_as_string_piece == "ntp_shown") {
+    set->insert(TriggerType::NTP_SHOWN);
+  } else if (trigger_as_string_piece == "foregrounded") {
+    set->insert(TriggerType::FOREGROUNDED);
+  } else if (trigger_as_string_piece == "fixed_timer") {
+    set->insert(TriggerType::FIXED_TIMER);
+  }
+}
+
+// Generates a set of disabled triggers.
+std::set<TriggerType> GetDisabledTriggerTypes() {
+  std::set<TriggerType> disabled_triggers;
+
+  // Do not in-line FeatureParam::Get(), |param_value| must stay alive while
+  // StringPieces reference segments.
+  std::string param_value = kDisableTriggerTypes.Get();
+
+  for (auto token :
+       base::SplitStringPiece(param_value, ",", base::TRIM_WHITESPACE,
+                              base::SPLIT_WANT_NONEMPTY)) {
+    TryAddTriggerType(token, &disabled_triggers);
+  }
+  return disabled_triggers;
+}
+
 // Run the given closure if it is valid.
 void TryRun(base::OnceClosure closure) {
   if (closure) {
@@ -75,11 +124,19 @@ void TryRun(base::OnceClosure closure) {
 
 }  // namespace
 
-FeedSchedulerHost::FeedSchedulerHost(PrefService* pref_service,
+FeedSchedulerHost::FeedSchedulerHost(PrefService* profile_prefs,
+                                     PrefService* local_state,
                                      base::Clock* clock)
-    : pref_service_(pref_service),
+    : profile_prefs_(profile_prefs),
       clock_(clock),
-      user_classifier_(pref_service, clock) {}
+      user_classifier_(profile_prefs, clock),
+      disabled_triggers_(GetDisabledTriggerTypes()),
+      eula_accepted_notifier_(
+          web_resource::EulaAcceptedNotifier::Create(local_state)) {
+  if (eula_accepted_notifier_) {
+    eula_accepted_notifier_->Init(this);
+  }
+}
 
 FeedSchedulerHost::~FeedSchedulerHost() = default;
 
@@ -104,7 +161,7 @@ void FeedSchedulerHost::Initialize(
       std::move(schedule_background_task_callback);
 
   base::TimeDelta old_period =
-      pref_service_->GetTimeDelta(prefs::kBackgroundRefreshPeriod);
+      profile_prefs_->GetTimeDelta(prefs::kBackgroundRefreshPeriod);
   base::TimeDelta new_period = GetTriggerThreshold(TriggerType::FIXED_TIMER);
   if (old_period != new_period) {
     ScheduleFixedTimerWakeUp(new_period);
@@ -115,8 +172,18 @@ NativeRequestBehavior FeedSchedulerHost::ShouldSessionRequestData(
     bool has_content,
     base::Time content_creation_date_time,
     bool has_outstanding_request) {
+  // The scheduler may not always know of outstanding requests, but the Feed
+  // should know about them all, and the scheduler should be notified upon
+  // completion of all requests. We should never encounter a scenario where only
+  // the scheduler thinks there is an outstanding request.
+
+  // TODO(skym): Resolve ambiguity around this expectation.
+  // DCHECK(has_outstanding_request || !tracking_oustanding_request_);
+
+  tracking_oustanding_request_ |= has_outstanding_request;
+
   NativeRequestBehavior behavior;
-  if (!has_outstanding_request && ShouldRefresh(TriggerType::NTP_SHOWN)) {
+  if (ShouldRefresh(TriggerType::NTP_SHOWN)) {
     if (!has_content) {
       behavior = REQUEST_WITH_WAIT;
     } else if (IsContentStale(content_creation_date_time)) {
@@ -143,20 +210,24 @@ NativeRequestBehavior FeedSchedulerHost::ShouldSessionRequestData(
 
   // TODO(skym): Record requested behavior into histogram.
   user_classifier_.OnEvent(UserClassifier::Event::NTP_OPENED);
+  DVLOG(2) << "Specifying NativeRequestBehavior of "
+           << static_cast<int>(behavior);
   return behavior;
 }
 
 void FeedSchedulerHost::OnReceiveNewContent(
     base::Time content_creation_date_time) {
-  pref_service_->SetTime(prefs::kLastFetchAttemptTime,
-                         content_creation_date_time);
+  profile_prefs_->SetTime(prefs::kLastFetchAttemptTime,
+                          content_creation_date_time);
   TryRun(std::move(fixed_timer_completion_));
   ScheduleFixedTimerWakeUp(GetTriggerThreshold(TriggerType::FIXED_TIMER));
+  tracking_oustanding_request_ = false;
 }
 
 void FeedSchedulerHost::OnRequestError(int network_response_code) {
-  pref_service_->SetTime(prefs::kLastFetchAttemptTime, clock_->Now());
+  profile_prefs_->SetTime(prefs::kLastFetchAttemptTime, clock_->Now());
   TryRun(std::move(fixed_timer_completion_));
+  tracking_oustanding_request_ = false;
 }
 
 void FeedSchedulerHost::OnForegrounded() {
@@ -190,11 +261,65 @@ void FeedSchedulerHost::OnSuggestionConsumed() {
   user_classifier_.OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
 }
 
+void FeedSchedulerHost::OnHistoryCleared() {
+  // Due to privacy, we should not fetch for a while (unless the user explicitly
+  // asks for new suggestions) to give sync the time to propagate the changes in
+  // history to the server.
+  suppress_refreshes_until_ =
+      clock_->Now() +
+      base::TimeDelta::FromMinutes(kSuppressRefreshDurationMinutes.Get());
+  // After that time elapses, we should fetch as soon as possible.
+  profile_prefs_->ClearPref(prefs::kLastFetchAttemptTime);
+}
+
+void FeedSchedulerHost::OnEulaAccepted() {
+  OnForegrounded();
+}
+
 bool FeedSchedulerHost::ShouldRefresh(TriggerType trigger) {
-  // TODO(skym): Check various other criteria are met, record metrics.
-  return (clock_->Now() -
-          pref_service_->GetTime(prefs::kLastFetchAttemptTime)) >
-         GetTriggerThreshold(trigger);
+  // TODO(skym): Record metrics throughout this method per design.
+
+  if (tracking_oustanding_request_) {
+    DVLOG(2) << "Outstanding request stopped refresh from trigger "
+             << static_cast<int>(trigger);
+    return false;
+  }
+
+  if (base::ContainsKey(disabled_triggers_, trigger)) {
+    DVLOG(2) << "Disabled trigger stopped refresh from trigger "
+             << static_cast<int>(trigger);
+    return false;
+  }
+
+  if (net::NetworkChangeNotifier::IsOffline()) {
+    DVLOG(2) << "Network is offline stopped refresh from trigger "
+             << static_cast<int>(trigger);
+    return false;
+  }
+
+  if (eula_accepted_notifier_ && !eula_accepted_notifier_->IsEulaAccepted()) {
+    DVLOG(2) << "EULA not being accepted stopped refresh from trigger "
+             << static_cast<int>(trigger);
+    return false;
+  }
+
+  if (clock_->Now() < suppress_refreshes_until_) {
+    DVLOG(2) << "Refresh suppression until " << suppress_refreshes_until_
+             << " stopped refresh from trigger " << static_cast<int>(trigger);
+    return false;
+  }
+
+  base::TimeDelta attempt_age =
+      clock_->Now() - profile_prefs_->GetTime(prefs::kLastFetchAttemptTime);
+  if (attempt_age < GetTriggerThreshold(trigger)) {
+    DVLOG(2) << "Last attempt age of " << attempt_age
+             << " stopped refresh from trigger " << static_cast<int>(trigger);
+    return false;
+  }
+
+  DVLOG(2) << "Requesting refresh from trigger " << static_cast<int>(trigger);
+  tracking_oustanding_request_ = true;
+  return true;
 }
 
 bool FeedSchedulerHost::IsContentStale(base::Time content_creation_date_time) {
@@ -213,7 +338,7 @@ base::TimeDelta FeedSchedulerHost::GetTriggerThreshold(TriggerType trigger) {
 }
 
 void FeedSchedulerHost::ScheduleFixedTimerWakeUp(base::TimeDelta period) {
-  pref_service_->SetTimeDelta(prefs::kBackgroundRefreshPeriod, period);
+  profile_prefs_->SetTimeDelta(prefs::kBackgroundRefreshPeriod, period);
   schedule_background_task_callback_.Run(period);
 }
 

components/feed/core/feed_scheduler_host.h

@@ -5,11 +5,15 @@
 #ifndef COMPONENTS_FEED_CORE_FEED_SCHEDULER_HOST_H_
 #define COMPONENTS_FEED_CORE_FEED_SCHEDULER_HOST_H_
 
+#include <memory>
+#include <set>
+
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/feed/core/user_classifier.h"
+#include "components/web_resource/eula_accepted_notifier.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -37,19 +41,19 @@ enum NativeRequestBehavior {
   NO_REQUEST_WITH_TIMEOUT
 };
 
-enum class TriggerType;
-
 // Implementation of the Feed Scheduler Host API. The scheduler host decides
 // what content is allowed to be shown, based on its age, and when to fetch new
 // content.
-class FeedSchedulerHost {
+class FeedSchedulerHost : web_resource::EulaAcceptedNotifier::Observer {
  public:
   // The TriggerType enum specifies values for the events that can trigger
   // refreshing articles.
-  enum class TriggerType { NTP_SHOWN = 0, FOREGROUNDED = 1, FIXED_TIMER = 2 };
+  enum class TriggerType { NTP_SHOWN, FOREGROUNDED, FIXED_TIMER, COUNT };
 
-  FeedSchedulerHost(PrefService* pref_service, base::Clock* clock);
-  ~FeedSchedulerHost();
+  FeedSchedulerHost(PrefService* profile_prefs,
+                    PrefService* local_state,
+                    base::Clock* clock);
+  ~FeedSchedulerHost() override;
 
   using ScheduleBackgroundTaskCallback =
       base::RepeatingCallback<void(base::TimeDelta)>;
@@ -94,12 +98,20 @@ class FeedSchedulerHost {
   // scheduler should be optimizing for.
   void OnSuggestionConsumed();
 
+  // When the user clears history, the scheduler will clear out some stored data
+  // and stop requesting refreshes for a period of time.
+  void OnHistoryCleared();
+
  private:
   FRIEND_TEST_ALL_PREFIXES(FeedSchedulerHostTest, GetTriggerThreshold);
 
+  // web_resource::EulaAcceptedNotifier::Observer:
+  void OnEulaAccepted() override;
+
   // Determines whether a refresh should be performed for the given |trigger|.
   // If this method is called and returns true we presume the refresh will
-  // happen, therefore we report metrics respectively.
+  // happen, therefore we report metrics respectively and update
+  // |tracking_oustanding_request_|.
   bool ShouldRefresh(TriggerType trigger);
 
   // Decides if content whose age is the difference between now and
@@ -115,7 +127,7 @@ class FeedSchedulerHost {
   void ScheduleFixedTimerWakeUp(base::TimeDelta period);
 
   // Non-owning reference to pref service providing durable storage.
-  PrefService* pref_service_;
+  PrefService* profile_prefs_;
 
   // Non-owning reference to clock to get current time.
   base::Clock* clock_;
@@ -135,6 +147,27 @@ class FeedSchedulerHost {
   // the refresh has completed. Is called on refresh success or failure.
   base::OnceClosure fixed_timer_completion_;
 
+  // Set of triggers that should be ignored. By default this is empty.
+  std::set<TriggerType> disabled_triggers_;
+
+  // In some circumstances, such as when history is cleared, the scheduler will
+  // stop requesting refreshes for a given period. During this time, only direct
+  // user interaction with the NTP (and outside of the scheduler's control)
+  // should cause a refresh to occur.
+  base::Time suppress_refreshes_until_;
+
+  // Whether the scheduler is aware of an outstanding refresh or not. There are
+  // cases where a refresh may be occurring without the scheduler knowing about
+  // it, such as user interaction with UI on the NTP. If this field holds a
+  // value of true, it is expected that either OnReceiveNewContent or
+  // OnRequestError will be called eventually, somewhere on the order of seconds
+  // from now, assuming the browser does not shut down.
+  bool tracking_oustanding_request_ = false;
+
+  // May hold a nullptr if the platform does not show the user a EULA. Will only
+  // notify if IsEulaAccepted() is called and it returns false.
+  std::unique_ptr<web_resource::EulaAcceptedNotifier> eula_accepted_notifier_;
+
   DISALLOW_COPY_AND_ASSIGN(FeedSchedulerHost);
 };
 

components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc

@@ -358,7 +358,7 @@ class EulaState final : public web_resource::EulaAcceptedNotifier::Observer {
     eula_notifier_->Init(this);
   }
 
-  ~EulaState() = default;
+  ~EulaState() override = default;
 
   bool IsEulaAccepted() {
     if (!eula_notifier_) {

components/web_resource/eula_accepted_notifier.h

@@ -19,6 +19,7 @@ class EulaAcceptedNotifier {
   // Observes EULA accepted state changes.
   class Observer {
    public:
+    virtual ~Observer() {}
     virtual void OnEulaAccepted() = 0;
   };