Closed Bug 1131413 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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).
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Blocks: 1132074
This just adds tests for the existing functionality.
Attachment #8566396 - Flags: review?(adw)
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.
Attachment #8566397 - Flags: feedback?(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.
Attachment #8566396 - Flags: review?(adw) → review+
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?
Attachment #8566397 - Flags: feedback?(adw) → feedback+
Component: General → Reading List
(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.
Attachment #8566397 - Attachment is obsolete: true
Attachment #8566958 - Flags: review?(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.
Attachment #8566958 - Flags: review?(adw) → review+
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Depends on: 1137087
Depends on: 1146054
Depends on: 1170079
Depends on: 1170926
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.