1131412 - Implement a reading list engine scheduler

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

We need a component so the reading-list sync-engine can be scheduled. Examples of this are:

* The reading-list UI has made a change, so it wants the new state to be synced ASAP - however, we may currently be offline or behind a captive-portal etc.  The scheduler should just be told to Sync ASAP and take care of the details.

* We will periodically want to Sync to get new state from other devices.  The scheduler will be responsible for maintaining a timer to manage this.

* If the user clicks the "Sync" toolbar button, the reading-list service should also "Sync now" (which will not come automatically as the reading list engine is not a "sync engine")

* The server may issue "backoff" requests - the scheduler is responsible for noticing and managing this.

Note that Sync already has a scheduler, but it isn't generic enough to be reused in this context.  However, it may require some changes to the sync scheduler to support this.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1131412-implement-a-scheduler-for-the-readinglis.patch

The scheduler.  Still a bit of a WIP while we wait for the sync engine, but I think this is OK as a first cut.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8566395 [details] [diff] [review]
0002-Bug-1131412-implement-a-scheduler-for-the-readinglis.patch

Review of attachment 8566395 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anything wrong! :-)

::: browser/components/readinglist/Scheduler.jsm
@@ +182,5 @@
> +  },
> +
> +  // checkStatus checks the current state and the environment to see when
> +  // we should next sync.
> +  _checkStatus(ignoreBlockingErrors = false) {

This wants to be named _(re)setTimer I think.  Something with "timer".

@@ +258,5 @@
> +      // Write a pref in the same format used to services/sync to indicate
> +      // the last success.
> +      prefs.set("lastSync", new Date().toString());
> +      Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
> +      this.state = this.STATE_OK;

Seems like observers should see this.state == OK when they're called.

@@ +265,5 @@
> +      this.log.error("Sync failed", err);
> +      Services.obs.notifyObservers(null, "readinglist:sync:error", null);
> +      // XXX - how to detect an auth error?
> +      this.state = err == this._engine.ERROR_AUTHENTICATION ?
> +                   this.STATE_ERROR_AUTHENTICATION : this.STATE_ERROR_OTHER;

Similarly here.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1131412-implement-a-scheduler-for-the-readinglis.patch

Addressed all feedback

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8566957 [details] [diff] [review]
0002-Bug-1131412-implement-a-scheduler-for-the-readinglis.patch

Review of attachment 8566957 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nits.

::: browser/components/readinglist/Scheduler.jsm
@@ +24,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, 'clearTimeout',
> +  'resource://gre/modules/Timer.jsm');
> +
> +XPCOMUtils.defineLazyModuleGetter(this, 'Task',
> +  'resource://gre/modules/Task.jsm');

Nit: It doesn't make sense to lazy-load this since methods below are defined using Task.async.  But it's not, like, wrong either, so that's why this is only a nit.

@@ +52,5 @@
> +  sync: Task.async(function* () {
> +  }),
> +}
> +
> +let prefs = new Preferences("readinglist.scheduler.");

Nit: I'm normally not a fan of prefixes in variable names, but the G prefix for globals does help me see that I shouldn't look for a random G variable in a function to be defined there somewhere.  Up to you.

@@ +55,5 @@
> +
> +let prefs = new Preferences("readinglist.scheduler.");
> +
> +// A helper to manage our interval values.
> +let intervals = {

Same nit here I guess.

@@ +91,5 @@
> +  _nextScheduledSync: null,
> +  // The time when the most-recent "backoff request" expires - we will never
> +  // schedule a new timer before this.
> +  _backoffUntil: 0,
> +  _timer: null, // Our current timer.

Nit: Moving the comment above the line, like the other comments here, would make this part easier to read I think.

@@ +100,5 @@
> +  _engine: engine,
> +
> +  // Our state variable and constants.
> +  state: "ok",
> +  STATE_OK: "ok",

Nit: Initialize `state` in init() so you can set it to STATE_OK instead of repeating STATE_OK's value.

@@ +114,5 @@
> +    this._nextScheduledSync = Date.now() + intervals.initial;
> +    this._createTimer();
> +  },
> +
> +  // XXX - only called by tests.

Nit: The XXX isn't necessary.

@@ +184,5 @@
> +  },
> +
> +  // _createTimer checks the current state and the environment to see when
> +  // we should next sync and creates the timer with the appropriate delay.
> +  _createTimer(ignoreBlockingErrors = false) {

Nit: createTimer seems like not a great name either since this method can actually clear the timer, too.  I think _setUpTimer would be better, but if you disagree that's fine, I won't ask you to change it.

@@ +222,5 @@
> +    if (!delay && !this._nextScheduledSync) {
> +      this.log.debug("_maybeReschedule ignoring a backoff request while running");
> +      return;
> +    }
> +    // So we have a delay and a time for the next schedule - resolve them.

This comment confuses me.  It says that we have a time for the next schedule, but then the code immediately following it checks for !this._nextScheduledSync.  And that code doesn't help me understand what "resolve" means.  I think maybe this should just be moved to the other block of comments below?

@@ +246,5 @@
> +    this._timerRunning = true;
> +    // flag that there's no new schedule yet, so a request coming in while
> +    // we are running does the right thing.
> +    this._nextScheduledSync = 0;
> +    let onDone = (nextDelay) => {

I think you can chain this to the promise chain below instead of calling it directly:

this._engine.sync().then(() => {
  ...
  return intervals.schedule;
}).then(null, err => { // or .catch(err => {
  ...
  return intervals.retry;
}).then(nextDelay => {
  ...
});

Maybe this would be nicer inside a Task.spawn.

@@ +316,5 @@
> +  get state() internalScheduler.state,
> +};
> +
> +// These functions are exposed purely for tests, which manage to grab them
> +// via a BackstafePass.

Backsta_g_ePass

::: browser/components/readinglist/test/xpcshell/head.js
@@ +8,5 @@
> +
> +// Setup logging prefs before importing the scheduler module.
> +Services.prefs.setCharPref("readinglist.log.appender.dump", "Trace");
> +
> +let {createTestableScheduler} = Cu.import("resource:///modules/readinglist/Scheduler.jsm", {});

Do Scheduler, Preferences, and setTimeout really need to be in head.js?  I have an xpcshell test for the storage and it doesn't need any of that stuff.

::: browser/components/readinglist/test/xpcshell/test_scheduler.js
@@ +19,5 @@
> +function createScheduler(options) {
> +  // avoid typos in the test and other footguns in the options.
> +  let allowedOptions = ["expectedDelay", "expectNewTimer", "syncFunction"];
> +  for (let key of Object.keys(options)) {
> +    if (allowedOptions.indexOf(key) == -1) {

Nit: Use array.includes().

::: browser/components/readinglist/test/xpcshell/xpcshell.ini
@@ +1,3 @@
> +[DEFAULT]
> +head = head.js
> +firefox-appdir = browser

What's this do?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Drew Willcoxon :adw from comment #4)
> > +firefox-appdir = browser
> 
> What's this do?

Without that, xpcshell tests can't access modules in /browser/ :( See bug 810617.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8566957 [details] [diff] [review]
0002-Bug-1131412-implement-a-scheduler-for-the-readinglis.patch

Review of attachment 8566957 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/readinglist/Scheduler.jsm
@@ +27,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, 'Task',
> +  'resource://gre/modules/Task.jsm');
> +
> +// Note the scheduler is a singleton, so we only expose an instance.
> +this.EXPORTED_SYMBOLS = ["readingListScheduler"];

Convention throughout the entire tree (well, except services...) is to upper-camel-case exported symbols. ie: ReadingListScheduler

Comment 7

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/a226fd427f01

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2094005&repo=fx-team

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/c2d7b5760af1

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9dbcd29c1fe

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c2d7b5760af1

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d3dc7ed4b28