[Background fetching] Background fetching when opening an NTP.

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
index 71dd4f8c487c1db86a7047cb25e4d82576a603d7..9e90d7657827d7ee393b722b27d7fbc58cd18aa0 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
@@ -8,6 +8,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/macros.h"
@@ -20,6 +21,10 @@
 class PrefRegistrySimple;
 class PrefService;
 
+namespace base {
+class Clock;
+}
+
 namespace ntp_snippets {
 
 struct Status;
@@ -96,9 +101,23 @@ class SchedulingRemoteSuggestionsProvider final
       const DismissedSuggestionsCallback& callback) override;
   void ClearDismissedSuggestionsForDebugging(Category category) override;
 
+  // Overrides internal clock for testing purposes.
+  void SetClockForTesting(std::unique_ptr<base::Clock> clock) {
+    clock_ = std::move(clock);

[tschumann, 2017/01/04 10:43:52]

can we dependency inject this into the constructor? I don't think we have many
places to update and that would be the superior approach.
(ForTesting() methods are crutches applicable as temporary solutions where the
actual cleanup is too costly for the moment).

[jkrcal, 2017/01/04 14:19:03]

Done.

+  }
+
  private:
   // Abstract description of the fetching schedule.
-  struct FetchingSchedule;
+  struct FetchingSchedule {
+      base::TimeDelta interval_persistent_wifi;

[tschumann, 2017/01/04 10:43:52]

members go after methods.

[jkrcal, 2017/01/04 14:19:03]

Done.

+      base::TimeDelta interval_persistent_fallback;
+      base::TimeDelta interval_soft_on_usage_event;
+
+      static FetchingSchedule Empty();
+      bool operator==(const FetchingSchedule& other) const;
+      bool operator!=(const FetchingSchedule& other) const;
+      bool is_empty() const;
+  };
 
   // Callback that is notified whenever the status of |provider_| changes.
   void OnProviderStatusChanged(
@@ -114,20 +133,31 @@ class SchedulingRemoteSuggestionsProvider final
   // schedule.
   void StopScheduling();
 
+  // Checks whether it is time to perform a soft background fetch, according to
+  // |schedule|.
+  bool ShouldRefetchInTheBackgroundNow();
+
   // Callback after Fetch is completed.
   void FetchFinished(const FetchDoneCallback& callback,
-                     Status status_code,
+                     Status fetch_status,
                      std::vector<ContentSuggestion> suggestions);
 
+  // Callback after RefetchInTheBackground is completed.
+  void RefetchInTheBackgroundFinished(
+      std::unique_ptr<FetchStatusCallback> callback,
+      Status fetch_status);
+
+  // Common function to call after a fetch of any type is finished.
+  void OnFetchCompleted(Status fetch_status);
+
   FetchingSchedule GetDesiredFetchingSchedule() const;
-  FetchingSchedule GetLastFetchingSchedule() const;
-  void StoreLastFetchingSchedule(const FetchingSchedule& schedule);
 
-  // Common function to call after each fetch.
-  void OnFetchCompleted(Status status);
+  // Load and store |schedule_|.
+  void LoadLastFetchingSchedule();
+  void StoreFetchingSchedule();
 
-  // Applies the provided |schedule|.
-  void ApplyFetchingSchedule(const FetchingSchedule& schedule);
+  // Applies the schedule given by |schedule_|.
+  void ApplyFetchingSchedule(bool also_apply_persistent_schedule);
 
   // Interface for doing all the actual work (apart from scheduling).
   std::unique_ptr<RemoteSuggestionsProvider> provider_;
@@ -136,11 +166,18 @@ class SchedulingRemoteSuggestionsProvider final
   // null.
   PersistentScheduler* persistent_scheduler_;
 
+  FetchingSchedule schedule_;
+

[tschumann, 2017/01/04 10:43:52]

nit: in general, we should aim to limit vertical white space. In the members
section, there's no comment addressing multiple members, so I'd vote for
removing the blank lines between them.

[jkrcal, 2017/01/04 14:19:03]

Done.

+  bool background_fetch_in_progress_;
+
   // Used to adapt the schedule based on usage activity of the user. Not owned.
   const UserClassifier* user_classifier_;
 
   PrefService* pref_service_;
 
+  // Allow for an injectable clock for testing.

[tschumann, 2017/01/04 10:43:52]

this comment is not necessary.

[jkrcal, 2017/01/04 14:19:03]

Done.

+  std::unique_ptr<base::Clock> clock_;
+
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProvider);
 };