[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..95033b75490f3ea7e35532a53583100f862f3daf 100644
--- a/components/ntp_snippets/remote/remote_suggestions_scheduler.h
+++ b/components/ntp_snippets/remote/remote_suggestions_scheduler.h
@@ -6,56 +6,20 @@
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_SCHEDULER_H_
 
 #include "base/macros.h"
-#include "base/time/time.h"
 
 namespace ntp_snippets {
 
-// 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
-//  should get triggered according to their schedule even if Chrome is not
-//  running at the given moment. This is OS-dependent, may be unavilable on
-//  some platforms.
-//  - "soft" ones that get triggered only if Chrome stays running until the
-//  scheduled point.
+// Interface for informing the scheduler of fetches of remote suggestions about

[Marc Treib, 2016/12/19 12:59:23]

nit: "scheduler of fetches of remote suggestions" is hard to parse for me. I
think just "scheduler" is enough; the rest is in the class name anyway.

[jkrcal, 2016/12/20 16:39:46]

Done.

+// relevant external events.
 class RemoteSuggestionsScheduler {
  public:
-  // Interface to perform the scheduled update.
-  class Updater {
-    virtual void HardUpdate();
-    virtual void SoftUpdate();
-  };
+  // Fetch content suggestions.
+  virtual void OnFetchDue() = 0;

[tschumann, 2016/12/19 11:07:19]

This sounds a bit odd on this interface. The scheduler is making the scheduling
decisions, so it's weird that somebody else tells it to fetch.

Why not name the signal very explicitly, like OnPersistentSchedulerWakeUp() or
something like that?

Would be good to transport the proper context of the signal so that the 
scheduling decisions are easy to follow.

[jkrcal, 2016/12/20 16:39:46]

Done. Great suggestions, thanks!

 
-  // The passed in |updater| is called when an update is due according to the
-  // schedule. Note that hard fetches get access to the |updater| via the keyed
-  // 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);
-
-  // Schedules both "soft" and "hard" fetches. First removes existing schedule
-  // before scheduling new updates.
-  void Schedule();
-
-  // Removes any existing 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;
-
-  // Cancel any scheduled tasks.
-  virtual bool Unschedule() = 0;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsHardScheduler);
+  // Force rescheduling of fetching.
+  virtual void RescheduleFetching() = 0;
 };

[Marc Treib, 2016/12/19 12:59:23]

DISALLOW_COPY_AND_ASSIGN?

[jkrcal, 2016/12/20 16:39:46]

Is it needed? Are these default constructors created when I have no constructor
at all?
(If I add this macro, it forces me to also add a constructor here.)

 
 }  // namespace ntp_snippets
 
-#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_HARD_SCHEDULER_H_
+#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_SCHEDULER_H_