675824 - End mobile setup wizard with first sync download status/progress bar

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

2 bars, one for desktop, one for mobile

    2 phase:

    hi, waiting for data to make it up to the server, indeterminate progress bar

    once it can download data to the phone, progress bar on download

    mobile: requires explicit exit from status page.

    desktop: second desktop should start downloading as soon as the second computer is hooked in.

Comment 1

[Allison Naaktgeboren :ally]

Attachment: Part 1(v-1) mobile progress bar WIP, broken

Comment 2

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v0) sync progress bar

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568179 [details] [diff] [review]
Part 1 (v0) sync progress bar

>       onComplete: function onComplete(aCredentials) {
>+

Nit: spurious whitespace. Please remove.

>         self.jpake = null;
>-        self.close();
>+
>+        let progressBar = document.getElementById("syncsetup-progressbar");
>+        progressBar.mode = "determined";
>+        progressBar.max = Weave.Engines.getEnabled().length;
>+        progressBar.value = 0;
>+        document.getElementById("syncsetup-waiting-desc").hidden = true;
>+        document.getElementById("syncsetup-waitingdownload-desc").hidden = false;
>+
>+        onFirstSync: function onFirstSync() {

I'm stumped that this is valid syntax! That `onFirstSync: ` stuff is for defining methods on objects, but here you're just defining a function. Please remove!

>+          let counter = parseInt(progressBar.value) + 1;

Why are you doing this? You could just increment `counter` as a number. (You need to define and initialize `counter` outside the scope of this function, of course.)

(Also, if you were using parseInt, make sure to always use it with the base argument, like parseInt(str, 10).)

>+          progressBar.setAttribute("value", counter);
>+          if(progressBar.value == progressBar.max) {

Nit: space after `if`. Also, we should really be checking `counter` here against Weave.Engines.getEnabled().length (please save the latter expression in a variable to avoid recomputing it everytime.)

>+            Services.obs.removeObserver(onFirstSync,"weave:engine:sync:finish",false);
>+            Services.obs.removeObserver(onFirstSync,"weave:engine:sync:error",false);
>+            self.close();

Hmm, do we want to automatically close here? We're not doing that on desktop, and comment 0 say "mobile: requires explicit exit from status page." Not sure what exactly rnewman meant with this somewhat cryptic comment. Then again, since on mobile the wizard is essentially modal, maybe we do? I can see good points for both. Will try to get some UX feedback here.

>+        Services.obs.addObserver(onFirstSync,"weave:engine:sync:finish",false);
>+        Services.obs.addObserver(onFirstSync,"weave:engine:sync:error",false);

Nit: space after commas (also on the removeObserver calls above.)

>@@ -221,16 +242,22 @@
> 
>     // Clear the UI so it's ready for next time
>     this._elements.account.value = "";
>     this._elements.password.value = "";
>     this._elements.synckey.value = "";
>     this._elements.usecustomserver.checked = false;
>     this._elements.customserver.disabled = true;
>     this._elements.customserver.value = "";
>+    document.getElementById("syncsetup-waiting-desc").hidden = false;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    let progressBar = document.getElementById("syncsetup-progressbar");
>+    progressBar.max= 0 ;

Nit: spaces are all sorts of wrong here :)

>+    progressBar.value = 0;
>+    progressBar.mode = "indeterminate";
> 
>     // Close the connect UI
>     document.getElementById("syncsetup-container").hidden = true;
>     BrowserUI.popDialog();
>   },
> 
>   toggleCustomServer: function toggleCustomServer() {
>     let useCustomServer = this._elements.usecustomserver.checked;
>diff --git a/mobile/locales/en-US/chrome/sync.dtd b/mobile/locales/en-US/chrome/sync.dtd
>--- a/mobile/locales/en-US/chrome/sync.dtd
>+++ b/mobile/locales/en-US/chrome/sync.dtd
>@@ -15,8 +15,9 @@
> <!ENTITY sync.password              "Password">
> <!ENTITY sync.recoveryKey           "Recovery Key">
> <!ENTITY sync.customServer          "Use custom server">
> <!ENTITY sync.serverURL             "Server URL">
> <!ENTITY sync.setup.connect         "Connect">
> <!ENTITY sync.setup.cancel          "Cancel">
> <!ENTITY sync.setup.tutorial        "Show me how">
> <!ENTITY sync.setup.waiting         "Pairing in progressâ¦">
>+<!ENTITY sync.setup.waitingdownload "Your data is now being encrypted and downloaded in the background.">

This string doesn't make sense on several levels. First off, downloading goes with *de*crypting, not *en*crypting. Though I don't think it's very useful to go into that detail in the mobile UI. I think it's probably better to explain that users can close the dialog at this point. So I would say something like "Your data is now being downloaded in the background. You can close this window at any time."

On that note: I think the setup wizard displays a "Cancel" button at the bottom. Once the pairing is complete and we start syncing (onComplete() and onwards), we want to replace that with a "Close" button because at this point you can no longer cancel the setup -- we're already set up and just doing the first sync. You can just close the dialog at this point.

Comment 4

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1) sync progress bar

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568294 [details] [diff] [review]
Part 1 (v1) sync progress bar

>         <vbox id="syncsetup-waiting" class="syncsetup-page" flex="1" hidden="true">
>-          <description class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <progressmeter id="syncsetup-progressbar" mode="undetermined"/>
>+          <description id="syncsetup-waiting-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <description id="syncsetup-waitingdownload-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waitingdownload;</description>

Add a default hidden="true"? You get to save some code down below.

>+         <hbox class="prompt-buttons" pack="center">
>+            <button id= "syncsetup-waiting-close" class="prompt-button" oncommand="WeaveGlue.close();">&sync.setup.close;</button>

Same here.

>+            <separator/>
>+        </hbox>
>         </vbox>

Nit: indent the <hbox> properly (2 spaces below the <vbox>, and its children 2 spaces below that).

>       try {
>         this._elements.device.value = Services.prefs.getCharPref("services.sync.client.name");
>       } catch(e) {}
>     } else if (Weave.Status.login != Weave.LOGIN_FAILED_NO_USERNAME) {
>       this.loadSetupData();
>     }
>+    this._progressBar = document.getElementById("syncsetup-progressbar");
>+
>   },

Nit: remove empty line before closing brace.

>     // Show the connect UI
>     container.hidden = false;
>     document.getElementById("syncsetup-simple").hidden = false;
>     document.getElementById("syncsetup-waiting").hidden = true;
>     document.getElementById("syncsetup-fallback").hidden = true;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    document.getElementById("syncsetup-waiting-close").hidden = true;

With the default hidden="true" attributes in XUL and proper clean up in close() (which you have), this isn't necessary.

>       onComplete: function onComplete(aCredentials) {
>         self.jpake = null;
>-        self.close();
>+        self.onSyncFinish = self.onFirstSync.bind(self);

You can do this just once in init()! Also, why two different names for the same method?

>+  onFirstSync: function onFirstSync() {

This name is not very accurate. I suggest something like onEngineSync(). Or just take some inspiration from your work on the desktop progress bar, e.g. increaseFirstSyncProgressBar.

>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    if (this._progressMax == null) {
>+    this._progressMax = Weave.Engines.getEnabled().length;
>+    this._progressBar.max = this._progressMax;
>+    }

First off, we should put a comment here explaining why we're initializing _progressMax lazily here (because we don't know which/how many engines are going to be enabled until we sync). On that note, it occurred to me that the special "clients" engine syncs before we even find that out! So to that end we want to add a check here:

  if (this._progressMax == null && data != "clients") {

'data' is the nsIObserver::observe() argument that we get passed into this function.

Also nit: Indention!

>+    if (this._progressValue >= this._progressMax) {
>+      this.close();
>+    }

It just occurred to me that listening to "weave:service:sync:finish" / "weave:service:sync:error" is much more reliable, because if we error out mid-sync, we will never hit progressValue >= progressMax.

>   close: function close() {
>+    Services.obs.removeObserver(this.onSyncFinish, "weave:engine:sync:finish", false);
>+    Services.obs.removeObserver(this.onSyncFinish, "weave:engine:sync:error", false);
>+
>     if (this.jpake)
>       this.abortEasySetup();
>-

Spurious whitespace change. Please revert.

>+    document.getElementById("syncsetup-waiting-desc").hidden = false;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    document.getElementById("syncsetup-waiting-close").hidden = true;
>+    this._progressMax = null;
>+    this._progressValue = 0;
>+    this._progressBar.max = 0;
>+    this._progressBar.value = 0;
>+    this._progressBar.mode = "indeterminate";

This is not a valid value for the "mode" attribute. I think you mean "undetermined"?

Comment 6

[Allison Naaktgeboren :ally]

>       onComplete: function onComplete(aCredentials) {
>         self.jpake = null;
>-        self.close();
>+        self.onSyncFinish = self.onFirstSync.bind(self);

You can do this just once in init()! Also, why two different names for the same method?

what do you mean two names for the same method? we're saving it so it can be used properly in the clean up in close so it would be invoked on the correct object and in the correct scope later.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to :Ally Naaktgeboren from comment #6)
> what do you mean two names for the same method? we're saving it so it can be
> used properly in the clean up in close so it would be invoked on the correct
> object and in the correct scope later.

Right. You're binding it to the object it's defined on so that every invocation, no matter how it's performed, will guarantee the right 'this' object. This is a pretty common pattern. My point is why do this.A = this.B.bind(this) rather than this.A = this.A.bind(this) or at least this._boundA = this.A.bind(this). Related names for related things are better than unrelated names :)

Comment 8

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v2) sync progress bar

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568571 [details] [diff] [review]
Part 1 (v2) sync progress bar

>+  onEngineSync: function onEngineSync(data) {
>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {
>+      this._progressMax = Weave.Engines.getEnabled().length;

Yeah, so I kinda forgot the Clients engine here. My bad. This needs a + 1 (cf bug 696823)

>   close: function close() {
>+    Services.obs.removeObserver(this._boundOnEngineSync, "weave:engine:sync:finish");
>+    Services.obs.removeObserver(this._boundOnEngineSync, "weave:engine:sync:error");
>+    Services.obs.removeObserver(this._boundOnServiceSync, "weave:service:sync:finish");
>+    Services.obs.removeObserver(this._boundOnServiceSync, "weave:service:sync:error");

These can throw if the dialog gets closed before onComplete() has been called above. So let's wrap them in a try/catch block to make sure we don't error out here and won't perform any of the other clean up below.

r=me with those two things. Can you upload screenshots of the two stages?

Comment 10

[Allison Naaktgeboren :ally]

Attachment: stage 2 screen shot, determinate progress bar

Comment 11

[Allison Naaktgeboren :ally]

Attachment: stage 1 screen shot, pairing in progress indeterminate progress bar

Comment 12

[Allison Naaktgeboren :ally]

STR for QA:
- applies to mobile only. no impact on desktop.
- click set up sync on about:home
- enter jpake code from mobile on desktop
- mobile will display stage 1: pairing in progress with indeterminate bar
-- should not have any buttons, cannot close during this
- once credentials are transferred to mobile & mobile starts syncing, text will change & progress bar will become determinate
-- progress will be uneven (1/n increment per engine)
- close button can be used and closes dialog
- when progress bar reaches 100%, dialog closes itself 
-- this is different from desktop progress bar page

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 568571 [details] [diff] [review]
Part 1 (v2) sync progress bar

Based on the screenshots, I'm withdrawing my r+. Just a few small UI nits:

* The undetermined progress bar page should probably keep the "Cancel" button that's visible on the previous page where the pairing code is shown.

* The "Close" button for the determined progress bar is not bottom-aligned like the Cancel button.

Comment 14

[Allison Naaktgeboren :ally]

Attachment: stage 1 screen shot, pairing in progress

Comment 15

[Allison Naaktgeboren :ally]

Attachment: stage 2 screen shot, determinate progress bar during first sync

Comment 16

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v3) sync progress bar mobile

assuming the screen shots pass muster

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569433 [details]
stage 2 screen shot, determinate progress bar during first sync

Nice!

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569431 [details]
stage 1 screen shot, pairing in progress

The button should say "Cancel" because at this point closing that dialog will still have the side-effect of canceling the JPAKE exchange.

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569436 [details] [diff] [review]
Part 1 (v3) sync progress bar mobile

>         <vbox id="syncsetup-waiting" class="syncsetup-page" flex="1" hidden="true">
>-          <description class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <progressmeter id="syncsetup-progressbar" mode="undetermined"/>
>+          <vbox id="syncsetup-waiting-top" align="center" flex="1">
>+            <description id="syncsetup-waiting-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+            <description id="syncsetup-waitingdownload-desc" class="syncsetup-desc syncsetup-center" hidden="true" flex="1">&sync.setup.waitingdownload;</description>
>+          </vbox>
>+          <hbox class="prompt-buttons" pack="center" align="end">
>+            <button id= "syncsetup-waiting-close" class="prompt-button" oncommand="WeaveGlue.close();">&sync.setup.close;</button>

Nit: no space after id=

And what I said in comment 13 and comment 18: While the progress bar is undetermined, the button should say Cancel.

Comment 20

[Allison Naaktgeboren :ally]

Attachment: stage 1 indeterminate, pairing in progress

Comment 21

[Allison Naaktgeboren :ally]

Attachment: part 1 (v4) sync progress bar on mobile

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569478 [details]
stage 1 indeterminate, pairing in progress

woo!

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569481 [details] [diff] [review]
part 1 (v4) sync progress bar on mobile

Very nice! Shippit!

Comment 24

[Allison Naaktgeboren :ally]

i thought the official mozilla term was 'land it' :D

Comment 25

[Richard Newman [:rnewman]]

(In reply to :Ally Naaktgeboren from comment #24)
> i thought the official mozilla term was 'land it' :D

Epic burn, ^5

Comment 26

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569481 [details] [diff] [review]
part 1 (v4) sync progress bar on mobile

>+  onEngineSync: function onEngineSync(data) {

Just noticed this: 'data' is the 3rd argument for nsIObserver::observe, so you want onEngineSync(subject, topic, data)

>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {
>+      // Account for additional notification from client sync engine in max 
>+      // calculation.
>+      this._progressMax = Weave.Engines.getEnabled().length + 1;
>+      this._progressBar.max = this._progressMax;
>+    }

After some discussion, we came to the conclusion that it'd be better to just ignore the clients collection altogether as it provides more predictable behaviour. So we can bail out of this function if data == "clients" and then use Weave.Engines.getEnabled().length (without the additional 1 for the clients engine) as the max.

Comment 27

[Allison Naaktgeboren :ally]

Attachment: part 1(v4) progress bar on mobile

Comment 28

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569558 [details] [diff] [review]
part 1(v4) progress bar on mobile

>+  onEngineSync: function onEngineSync(subject, topic, data) {
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.

You just duplicated this comment from 3 lines below. A better comment to replace both of these would be something like:

  // The Clients engine syncs first. At this point we don't necessarily know yet
  // how many engines will be enabled, so we'll ignore the Clients engine and
  // evaluate how many engines are enabled when the first "real" engine syncs.

>+    if(data == 'clients') {

Nit: space after `if`

>+      return;
>+    }
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {

You can get rid of the `data != "clients"` check since we're now bailing out at the top of the function. Please remove it.

I'm assuming you have tested this patch with an account that's not syncing all of the engines and verified that the progressbar works as expected, right?

Comment 29

[Allison Naaktgeboren :ally]

Attachment: part1(v4.1) progress bar on mobile

yes, it was tested with < n engines sync'ed, including 1 engine and 0 engines. It behaves correctly in those cases as well.

Comment 30

[Philipp von Weitershausen [:philikon]]

Comment on attachment 569714 [details] [diff] [review]
part1(v4.1) progress bar on mobile

r+ without the dump statements ;)

Comment 31

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/services/services-central/rev/72827a90dbf6

STR for QA are in comment 12

Comment 32

[Tracy Walker [:tracy]]

verified with s-c on Galaxy Tab, progress bar works as expected and closes on automatically on completion.

Comment 33

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/72827a90dbf6