1131413 - Change browser-syncui and browser-fxaccounts to handle reading-list UI requirements

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

We have a couple of modules used to manage surfacing sync and FxA state in the UI. We need to ensure these modules are also capable of managing similar states from the reading-list engine.

For example, these modules handle "authentication errors" to show an infobar and allow you to sign back in.  We only want one such infobar regardless of the service that raises the error.  Similarly, these modules handle clicking of the "Sync" button and managing its animation - this facility needs to also handle reading list (eg, animate even when only the reading list is syncing, and ensuring both services sync when it is clicked).

Comment 1

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0003-Bug-1131413-part-1-add-tests-for-browser-syncui.js.-.patch

This just adds tests for the existing functionality.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0004-Bug-1131413-part-2-add-readinglist-support-to-browse.patch

This is the "real" patch - it adds reading-list support and updates the test in part 1.  It also adds a gSyncUI.log object, and touches browser.js to add the lazy import there.

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 8566396 [details] [diff] [review]
0003-Bug-1131413-part-1-add-tests-for-browser-syncui.js.-.patch

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

Nice!  Seems straightforward enough.

::: browser/base/content/test/general/browser_syncui.js
@@ +40,5 @@
> +  Weave.Status.login = Weave.LOGIN_SUCCEEDED;
> +  Services.obs.notifyObservers(null, "weave:ui:sync:error", null);
> +
> +  let subject = yield promiseNotificationAdded;
> +  let notification = subject.wrappedJSObject.object; // sync's observer abstraction is abstract!

Heh.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8566397 [details] [diff] [review]
0004-Bug-1131413-part-2-add-readinglist-support-to-browse.patch

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

Seems fine to me.

::: browser/base/content/browser-syncui.js
@@ +412,5 @@
> +    // and reading-list time.
> +    try {
> +      let lastRLSync = new Date(Services.prefs.getCharPref("readinglist.scheduler.lastSync"));
> +      if (!lastSync || lastRLSync > lastSync) {
> +        lastSync = lastRLSync;

So this shows the biggest (most recent) sync time of either Sync or Reading List.

@@ +447,5 @@
> +  // consistency, we just use the sync prefs.
> +  isProlongedReadingListError() {
> +    let lastSync, threshold, prolonged;
> +    try {
> +      lastSync = new Date(Services.prefs.getCharPref("readinglist.scheduler.lastSync"));

getCharPref?  Isn't this an int pref in terms of seconds?

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0004-Bug-1131413-part-2-add-readinglist-support-to-browse.patch

(In reply to Drew Willcoxon :adw from comment #4)

> So this shows the biggest (most recent) sync time of either Sync or Reading
> List.

Yep, and I added a comment to reflect that.  I hope I didn't miss some other point you were making :)

> @@ +447,5 @@
> > +  // consistency, we just use the sync prefs.
> > +  isProlongedReadingListError() {
> > +    let lastSync, threshold, prolonged;
> > +    try {
> > +      lastSync = new Date(Services.prefs.getCharPref("readinglist.scheduler.lastSync"));
> 
> getCharPref?  Isn't this an int pref in terms of seconds?

The scheduler patch actually writes this as a formatted date:

> prefs.set("lastSync", new Date().toString());

which is also exactly what Sync does.  I'm happy to change this if you feel strongly, but consistency with Sync and the fact the pref value is human readable make me think this is fine.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8566958 [details] [diff] [review]
0004-Bug-1131413-part-2-add-readinglist-support-to-browse.patch

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

(In reply to Mark Hammond [:markh] from comment #5)
> Yep, and I added a comment to reflect that.  I hope I didn't miss some other
> point you were making :)

Nope, just picture me stroking my beard as I was typing that. :-)  Just wanted to make sure I understood.

> The scheduler patch actually writes this as a formatted date:

I misunderstood.

::: browser/base/content/browser-syncui.js
@@ +189,5 @@
>      }
> +  },
> +
> +  onActivityStop() {
> +    if (!gBrowser)

Do you know why this gBrowser check was in onActivityStart in the first place?

Is it possible gBrowser is there on onActivityStart but gone by the time onActivityStop is called?  _numActiveSyncTasks would get screwed up.

@@ +193,5 @@
> +    if (!gBrowser)
> +      return;
> +    this.log.debug("onActivityStop with numActive", this._numActiveSyncTasks);
> +    if (--this._numActiveSyncTasks)
> +      return; // active tasks are still ongoing...

There should probably be an assertion that _numActiveSyncTasks >= 0.  This code can't really do anything about it if it fails, but at least we can know it's happening and try to fix it.

@@ +610,5 @@
> +    }
> +    // Now non-activity state (eg, enabled, errors, etc)
> +    // Note that sync uses the ":ui:" notifications for errors because sync.
> +    // ReadingList has no such concept (yet?; hopefully the :error is enough!)
> +    switch (topic) {

Nit: You don't really need two switches here.  When I first saw this I wondered if the first switch possibly changed the value of topic, and that's why you used two switches.  But I don't want to crimp your style, so only a nit.

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Had to disable the test on e10s :( - bug 1137087

https://hg.mozilla.org/integration/fx-team/rev/d41af7cc8362
https://hg.mozilla.org/integration/fx-team/rev/9fd3d9330f82

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

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d41af7cc8362
https://hg.mozilla.org/mozilla-central/rev/9fd3d9330f82

Comment 9

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bf115416751
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce90526403d8