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

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 39
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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+

Updated

4 years ago
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb

Updated

4 years ago
Blocks: 1132074
(Assignee)

Comment 1

4 years ago
Created attachment 8566396 [details] [diff] [review]
0003-Bug-1131413-part-1-add-tests-for-browser-syncui.js.-.patch

This just adds tests for the existing functionality.
Attachment #8566396 - Flags: review?(adw)
(Assignee)

Comment 2

4 years ago
Created attachment 8566397 [details] [diff] [review]
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.
Attachment #8566397 - Flags: feedback?(adw)

Comment 3

4 years ago
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 4

4 years ago
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+
(Assignee)

Updated

4 years ago
Component: General → Reading List
(Assignee)

Comment 5

4 years ago
Created attachment 8566958 [details] [diff] [review]
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.
Attachment #8566397 - Attachment is obsolete: true
Attachment #8566958 - Flags: review?(adw)

Comment 6

4 years ago
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+

Updated

4 years ago
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
(Assignee)

Updated

4 years ago
Depends on: 1137087
https://hg.mozilla.org/mozilla-central/rev/d41af7cc8362
https://hg.mozilla.org/mozilla-central/rev/9fd3d9330f82
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
(Assignee)

Updated

3 years ago
Depends on: 1146054
(Assignee)

Updated

3 years ago
Depends on: 1170079
(Assignee)

Updated

3 years ago
Depends on: 1170926
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.