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

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Allison Naaktgeboren :ally]

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)

Comment 1

[Allison Naaktgeboren :ally]

Attachment: v(0) add 1 to progress bar max

Comment 2

[Philipp von Weitershausen [:philikon]]

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.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569124 [details] [diff] [review]
v(0) add 1 to progress bar max

># HG changeset patch
># Parent 66cfcedcc8530f5344479e18e77de51e6e46d0d4
># User Allison Naaktgeboren <[e-mail]>
>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.

Comment 4

[Allison Naaktgeboren :ally]

Attachment: v(1) post disscussion changes

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.

Comment 5

[Philipp von Weitershausen [:philikon]]

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);
>+  }
> }

Comment 6

[Allison Naaktgeboren :ally]

Attachment: v(2) follow up changes

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)

Comment 7

[Philipp von Weitershausen [:philikon]]

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.

Comment 8

[Allison Naaktgeboren :ally]

Attachment: v(3) followup changes

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569833 [details] [diff] [review]
v(3) followup changes

Woo!

Comment 10

[Allison Naaktgeboren :ally]

\o/
https://hg.mozilla.org/services/services-central/rev/30efc2d895b7

Comment 11

[Allison Naaktgeboren :ally]

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.

Comment 12

[Allison Naaktgeboren :ally]

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%

Comment 13

[Tracy Walker [:tracy]]

verified on s-c with both the zero engine case and less than all engine case.

Comment 14

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/30efc2d895b7