Closed Bug 696823 Opened 8 years ago Closed 8 years ago

follow-up: adjust progress bar max to account for client engine sync notification

Categories

(Firefox :: Sync, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: ally, Assigned: ally)

Details

(Whiteboard: [verified-in-services])

Attachments

(1 file, 3 obsolete files)

as a follow up to 675822, the progress bar max should actually be 
Weave.Engines.getEnabled().length + 1 to account for the special client engine sync. (ie if n engines are syncing, we actually get n+1 notifications)
Attached patch v(0) add 1 to progress bar max (obsolete) — Splinter Review
Attachment #569124 - Flags: feedback?(philipp)
Attachment #569124 - Attachment is patch: true
Comment on attachment 569124 [details] [diff] [review]
v(0) add 1 to progress bar max

>   if (Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
>-    gProgressBar.max = Weave.Engines.getEnabled().length;
>+    // Account for additional notification from client sync engine in max calculation.

s/client/Clients/

r=me with that.
Attachment #569124 - Flags: feedback?(philipp) → review+
Comment on attachment 569124 [details] [diff] [review]
v(0) add 1 to progress bar max

># HG changeset patch
># Parent 66cfcedcc8530f5344479e18e77de51e6e46d0d4
># User Allison Naaktgeboren <ally@mozilla.com>
>Bug 696823 – follow-up: adjust progress bar max to account for client engine sync notification. r?philikon

Those are interesting characters before the "follow-up" :)

>   if (Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
>-    gProgressBar.max = Weave.Engines.getEnabled().length;
>+    // Account for additional notification from client sync engine in max calculation.
>+    gProgressBar.max = Weave.Engines.getEnabled().length + 1;

Actually, it just occurred to me that we should do the lazy evaluation of the maximum that we did in bug 675824 here too. Because we're showing the progress far for existing accounts and for the same reason that exists on mobile (not all engines might be enabled), the initial "getEnabled().length" might be wrong.
Attachment #569124 - Flags: review+
Attached patch v(1) post disscussion changes (obsolete) — Splinter Review
gFirstEngineSync probably needs a better name. it represents ' we received our first real tick yet && we are showing the progress bar at all' it does not make sense to increment the progress bar if it stays hidden.
Attachment #569124 - Attachment is obsolete: true
Attachment #569549 - Flags: feedback?(philipp)
Comment on attachment 569549 [details] [diff] [review]
v(1) post disscussion changes

> function onLoad(event) {
>-  Services.obs.addObserver(increaseProgressBar, "weave:engine:sync:finish", false);
>-  Services.obs.addObserver(increaseProgressBar, "weave:engine:sync:error", false);
>+  Services.obs.addObserver(onEngineSync, "weave:engine:sync:finish", false);
>+  Services.obs.addObserver(onEngineSync, "weave:engine:sync:error", false);
>   gProgressBar = document.getElementById('uploadProgressBar');
>+  gProgressBar.style.display = "none";
> 
>-  if (Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
>-    gProgressBar.max = Weave.Engines.getEnabled().length;
>-    gProgressBar.style.display = "inline";
>-  }
>-  else {
>-    gProgressBar.style.display = "none";
>-  }

Ugh, we shouldn't move this (see below).

> }
> 
> function onUnload(event) {
>-  Services.obs.removeObserver(increaseProgressBar, "weave:engine:sync:finish");
>-  Services.obs.removeObserver(increaseProgressBar, "weave:engine:sync:error");
>+  Services.obs.removeObserver(onEngineSync, "weave:engine:sync:finish");
>+  Services.obs.removeObserver(onEngineSync, "weave:engine:sync:error");
> }
> 
>-function increaseProgressBar(){
>-  gCounter += 1;
>-  gProgressBar.setAttribute("value", gCounter);
>+function onEngineSync(subject, topic, data){
>+  // We don't know how many engines are synced until first sync,
>+  // which happens to be the special Clients engine sync, which we want
>+  // to ignore.

This is incorrect, or at least phrased in a misleading way. We determine how many engines are synced *during* the first sync, which happens in this order:
1) Clients engine
2) determine which engines to sync
3) sync those engines
We want to ignore the Clients engine in (1) and wait until (3) to make sure step (2) has really happened :)

>+  // We also want the progress bar to stay hidden if we did
>+  // not come from the setup dialog (represented by the existence of
>+  // firstSync pref), such as in the case of session restore.

Yes, but we don't want the progress bar to appear out of nowhere, either. Moving the visibility change from onLoad to onEngineSync means the progress bar will first be hidden and then magically appear as the first sync engine is done syncing. There's a lot of stuff happening until then.

>+  if (data == "clients") {
>+    return;
>+  }
>+
>+  if( !gFirstEngineSync &&
>+      Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {

Nit: space before the `(`, not after.

Also, is `gFirstEngineSync` really needed? It seems to me we can just check `gCounter`. The edge case to consider is of course when all sync engines are disabled for some reason. But then we won't be making it this far in the first place. The only time we'd get the notification would be for the Clients engine and we bail out on that.

>+    gProgressBar.max = Weave.Engines.getEnabled().length;
>+    gProgressBar.style.display = "inline";
>+    gFirstEngineSync = true;
>+  }
>+
>+  if( gFirstEngineSync ) {

Nit: should be `if (gFirstEngineSync)`. Though I don't see why you would need this check at all.

>+    gCounter += 1;
>+    gProgressBar.setAttribute("value", gCounter);
>+  }
> }
Attachment #569549 - Flags: feedback?(philipp) → feedback-
Attached patch v(2) follow up changes (obsolete) — Splinter Review
as per the IRL discussions, this also includes additional listeners to fill progress bar on the end of a sync (service:finish, service:error) and unsubscribe the observers. This does introduce a case of trying to double remove the observers (in the case where the sync finishes and then the user hits the close button)
Attachment #569549 - Attachment is obsolete: true
Attachment #569809 - Flags: feedback?(philipp)
Comment on attachment 569809 [details] [diff] [review]
v(2) follow up changes

>+function onEngineSync(subject, topic, data) {
>+  if (data == "clients") {
>+    return;
>+  }

In the mobile patch we had a nice comment for this `if`. Can you put this in there, too, please?

>+
>+  if (gCounter==0 &&

Nit: spaces before and after operators. Also, you can simply test `!gCounter`.

>+      Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
>+    gProgressBar.max = Weave.Engines.getEnabled().length;
>+  }
>+
>   gCounter += 1;
>   gProgressBar.setAttribute("value", gCounter);
> }
> 
>+function onServiceSync(subject, topic, data) {
>+  gCounter = gProgressBar.max;
>+  gProgressBar.setAttribute("value", gCounter);

You can just do

  gProgressBar.setAttribute("value", gProgressBar.max);

right? gCounter isn't used for anything else after this point, I think. Also, please add a comment as to why we're doing this.
Attachment #569809 - Flags: feedback?(philipp)
Attachment #569809 - Attachment is obsolete: true
Attachment #569833 - Flags: feedback?(philipp)
Comment on attachment 569833 [details] [diff] [review]
v(3) followup changes

Woo!
Attachment #569833 - Flags: feedback?(philipp) → review+
STR for QA:
- in the case where 0 engines are sync'ed, when the sync finishes the progress bar will fill up. Before the progress bar would remain empty in this case. 
- should have minimal impact on UI otherwise.
Additional STR for QA (sorry guys!)
- when less than max engines are synced, progress bar should still reach 100%
-- this should match promised behavior in the original bug.

- start an existing account
--- select <n engines (I used history + bookmarks)
--- sync now, so the account updates that it should only be syncing those engines
-- pair a new device
--- go through normal pairing dialog
--- about:sync-progress page should pop up and the progress bar should reach 100%
verified on s-c with both the zero engine case and less than all engine case.
Whiteboard: [fixed-in-services] → [verified-in-services]
https://hg.mozilla.org/mozilla-central/rev/30efc2d895b7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.