[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_scheduler.h

Index: components/ntp_snippets/remote/remote_suggestions_scheduler.h
diff --git a/components/ntp_snippets/remote/remote_suggestions_scheduler.h b/components/ntp_snippets/remote/remote_suggestions_scheduler.h
index bad65c9519b266ec84ec1d2424eaec178f149eb3..ab0098be7f31cd8ea3abca9a0adade0555566101 100644
--- a/components/ntp_snippets/remote/remote_suggestions_scheduler.h
+++ b/components/ntp_snippets/remote/remote_suggestions_scheduler.h
@@ -8,8 +8,14 @@
 #include "base/macros.h"
 #include "base/time/time.h"
 
+class PrefRegistrySimple;
+class PrefService;
+
 namespace ntp_snippets {
 
+class RemoteSuggestionsHardScheduler;
+class UserClassifier;
+
 // Class to take care of scheduling of periodic updates of snippets. There are
 // two types of scheduled updates:
 //  - "hard" ones that should outlive current running instance of Chrome. These
@@ -22,8 +28,8 @@ class RemoteSuggestionsScheduler {
  public:
   // Interface to perform the scheduled update.
   class Updater {

[markusheintz_, 2016/12/12 14:36:16]

I think we don't need this interface. Instead we can call the
RemoteSuggestionsProvider directly.

[jkrcal, 2016/12/14 09:42:10]

I do not want to couple the scheduler with the provider. I'll do it via a
registered callback, as Tim suggests.

-    virtual void HardUpdate();
-    virtual void SoftUpdate();
+   public:
+    virtual void UpdateRemoteSuggestionsBySchedule() = 0;
   };
 
   // The passed in |updater| is called when an update is due according to the
@@ -31,31 +37,66 @@ class RemoteSuggestionsScheduler {
   // ContentSuggestionService because the concrete instance passed to
   // RemoteSuggestionsScheduler when the hard fetch was scheduled may not exist
   // any more when the hard update is due.
-  explicit RemoteSuggestionsScheduler(Updater* updater);
+  explicit RemoteSuggestionsScheduler(

[Marc Treib, 2016/12/09 12:25:27]

"explicit" not needed

[jkrcal, 2016/12/14 09:42:10]

Done.

+      RemoteSuggestionsHardScheduler* hard_scheduler,
+      const UserClassifier* user_classifier,
+      PrefService* pref_service);
+
+  ~RemoteSuggestionsScheduler();
+
+  // Sets the object responsible for performing the scheduled updates. If an
+  // update is due before an updater is specified by this function, the update
+  // is skipped.

[Marc Treib, 2016/12/09 12:25:27]

Document the lifetime of |updater|: Must outlive us.

[jkrcal, 2016/12/14 09:42:10]

Done.

+  void SetUpdater(Updater* updater);
 
-  // Schedules both "soft" and "hard" fetches. First removes existing schedule
-  // before scheduling new updates.
+  // One of the following two functions must be called on startup of Chrome for
+  // the scheduler to work correctly.

[tschumann, 2016/12/09 17:27:28]

that's odd. I'd assume that if I don't call Schedule/Unschedule, i'd have it in
the same state as after calling Unschedule. Can we fix this?

[jkrcal, 2016/12/14 09:42:10]

I am not sure I understand. Maybe not relevant any more?

+  // After the call, updates will be scheduled in the future. Idempotent, can be
+  // run any time later without impacting the current schedule.
+  // If you want to enforce rescheduling, call Unschedule() and then Schedule().
   void Schedule();

[tschumann, 2016/12/09 17:27:28]

without the comments, I would have guessed completely different semantics of
Schedule()/Unschedule(). Rename them to StartScheduling() / StopScheduling()?

[markusheintz_, 2016/12/12 14:36:16]

I would actually rename them to "OnChromeStartup" because the Scheduler needs to
do something on this particular "event/occasion".

[markusheintz_, 2016/12/12 15:32:28]

On a second though I guess this can be "OnEnterReadyState" or something similar.
Because this is the "event" the scheduler reacts to.

The "Unschedule" below could be "OnEnterDisableState" or something similar.

[jkrcal, 2016/12/14 09:42:10]

I like Start/StopScheduling quite a lot.

 
-  // Removes any existing schedule.
+  // After the call, no updates will happen before another call to Schedule().
+  // Idempotent, can be run any time later without impacting the current
+  // schedule.
   void Unschedule();
 
-  // Schedule periodic fetching of snippets, with different periods depending on
-  // network state. Once per period, the concrete implementation should call
-  // RemoteSuggestionsUpdater::HardUpdate where RemoteSuggestionsUpdater is
-  // obtained from ContentSuggestionsService.
-  // Any of the periods can be zero to indicate that the corresponding task
-  // should not be scheduled.
-  virtual bool Schedule(base::TimeDelta period_wifi,
-                        base::TimeDelta period_fallback) = 0;
+  // Must be called by the OS dependent hard scheduler when hard update is due.
+  // Initiates the hard update.
+  void PerformHardUpdate();

[markusheintz_, 2016/12/12 14:36:16]

I think this method should live somewhere else.

It's not the responsibility of the scheduler to perform updates.

[jkrcal, 2016/12/14 09:42:10]

Actually, it stayed in the scheduling layer. Do you still oppose it?

 
-  // Cancel any scheduled tasks.
-  virtual bool Unschedule() = 0;
+  // Must be called after an update successfully finishes (no matter whether the
+  // scheduler is currently active or not active).
+  void OnSuccessfulUpdate();
+
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);

[markusheintz_, 2016/12/12 14:36:16]

nit: I'm not sure whether we (inside the team) have agreed on a particular order
in class files. 

I'm not used to having static methods at the top (of the public section).

[jkrcal, 2016/12/14 09:42:10]

Did you mean that you _are_ used to have them at the top?
Anyway, done.

 
  private:
-  DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsHardScheduler);
+  struct UpdateSchedule {
+    base::TimeDelta interval_wifi;
+    base::TimeDelta interval_fallback;
+  };
+
+  void ResetUpdateScheduleFromNow(UpdateSchedule schedule);
+
+  UpdateSchedule GetCurrentUpdateSchedule();
+  UpdateSchedule GetLastUpdateSchedule();

[Marc Treib, 2016/12/09 12:25:27]

const?

[jkrcal, 2016/12/14 09:42:10]

Done.

+  void StoreLastUpdateSchedule(UpdateSchedule schedule);
+
+  // Interface for scheduling hard fetches, OS dependent. Not owned.

[Marc Treib, 2016/12/09 12:25:27]

May be null.

[jkrcal, 2016/12/14 09:42:10]

Done.

+  RemoteSuggestionsHardScheduler* hard_scheduler_;
+
+  // Interface for doing the actual updates, when they are due. Not owned.
+  Updater* updater_;
+
+  // Interface for doing the actual updates, when they are due. Not owned.

[Marc Treib, 2016/12/09 12:25:27]

Update comment

[jkrcal, 2016/12/14 09:42:10]

Done.

+  const UserClassifier* user_classifier_;
+
+  PrefService* pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsScheduler);
 };
 
 }  // namespace ntp_snippets
 
-#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_HARD_SCHEDULER_H_
+#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_SCHEDULER_H_