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.

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 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.

Addressed all feedback

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?

(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 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

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

https://hg.mozilla.org/integration/fx-team/rev/c2d7b5760af1 Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9dbcd29c1fe