598010 - Update Sync UI in Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Sync backend changed recently and Fennec has to make some changes. Instead of attaching patches on many bugs, I am attaching 1 patch to this bug.

See the dependent list of bugs for more info.

Comment 1

[Philipp von Weitershausen [:philikon]]

Comment on attachment 476799 [details] [diff] [review]
patch

>+  hyphenatePassphrase: function(passphrase) {
>+    // Hyphenate a 20 character passphrase in 4 groups of 5
>+    return passphrase.slice(0, 5) + '-'
>+         + passphrase.slice(5, 10) + '-'
>+         + passphrase.slice(10, 15) + '-'
>+         + passphrase.slice(15, 20);
>+  },
>+
>+  normalizePassphrase: function(pp) {
>+    // Remove hyphens as inserted by hyphenatePassphrase()
>+    if (pp.length == 23 && pp[5] == '-' && pp[11] == '-' && pp[17] == '-')
>+      return pp.slice(0, 5) + pp.slice(6, 11) + pp.slice(12, 17) + pp.slice(18, 23);
>+    return pp;

Perhaps make a note here that these can be removed in favour of Weave.Utils.*  once bug 597426 has landed in m-c.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch is the same as the last, but I had to change the way WeaveGlue was listening for Sync observer notifications. We now use Services.obs.addObserver and removeObserver so we can get the topics passed to the observer.

We now use some of the topics to correctly set the "login error" and "sync in progress" strings.

I created a loadInputs method so we could _only_ set the preference inputs when initializing. We used to just call _updateOptions in the WeaveGlue.init method, but this does too many other things in addition to setting the preference inputs.

The rest of the changes are for the dependent bugs, mainly changing the username -> account

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 476838 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/sync.js b/chrome/content/sync.js

>+    // Sync will use the account value and munge it into a username, as needed
>+    Weave.Service.account = this._settings.account.value;
>+    Weave.Service.login(Weave.Service.username, this._settings.pass.value, this.normalizePassphrase(this._settings.secret.value));

Does the comment means we want to use Weave.Service.username as the first parameter because setting Weave.Service.account will update Weave.Service.username?
Basically my question is: do you want to use Weave.Service.account as the first parameter?

> 
>     // Check the lock on a timeout because it's set just after notifying
>     setTimeout(function() {
>       // Prevent certain actions when the service is locked
>       if (Weave.Service.locked) {
>         connect.firstChild.disabled = true;
>         sync.firstChild.disabled = true;
>-        connect.setAttribute("title", syncStr.get("connecting.label"));
>-        sync.setAttribute("title", syncStr.get("lastSyncInProgress.label"));
>+
>+        if (aTopic == "weave:service:login:start")
>+          connect.setAttribute("title", syncStr.get("connecting.label"));
>+
>+        if (aTopic == "weave:service:sync:start")
>+          sync.setAttribute("title", syncStr.get("lastSyncInProgress.label"));
>       } else {

Not related to this bug but I wonder if the Weave team plan to add some constants for those messages or if they can potentially change one day? This will save us if they decide to change a few letters...

oh, looks like they have some but I don't see the one you use 
http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/constants.js#96

Thinking of it again, maybe this is because we are using an observer here.


>-  changeName: function changeName(input) {
>+  loadInputs: function loadInputs() {
>+    this._settings.account.value = Weave.Service.account || "";
>+    this._settings.pass.value = Weave.Service.password || "";
>+    let pp = Weave.Service.passphrase || "";
>+    if (pp.length == 20)
>+      pp = this.hyphenatePassphrase(pp);

Can we rename the var pp to something else (passPhrase?) and encapsulate the length check into hyphenatePassPhrase?
I don't want this pseudo constant to be spread across the code.

>+
>+  normalizePassphrase: function(pp) {
>+    // Remove hyphens as inserted by hyphenatePassphrase()
>+    if (pp.length == 23 && pp[5] == '-' && pp[11] == '-' && pp[17] == '-')
>+      return pp.slice(0, 5) + pp.slice(6, 11) + pp.slice(12, 17) + pp.slice(18, 23);
>+    return pp;

Can you use another var name than pp?
I'm pretty sure we can do this clean up with a nice regular expression, maybe something like return passPhrase.replace(/^(.{5})-(.{5})-(.{5})-(.{5})$/, "$1$2$3$4"); should work?



r+ with comments adressed, I'm particularly worried about the first one (username vs account).

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> Basically my question is: do you want to use Weave.Service.account as the first
> parameter?

Nope. The signature of Weave.Service.login() hasn't changed. You want to set Weave.Service.account first. That will update Weave.Service.username which you can then use to call login().

> Not related to this bug but I wonder if the Weave team plan to add some
> constants for those messages or if they can potentially change one day? This
> will save us if they decide to change a few letters...

If these topics change, we'd break a lot of code. So no, I don't think we're going to change them.

> oh, looks like they have some but I don't see the one you use 
> http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/constants.js#96

These aren't observer topics, these are possible values for Weave.Status.login.

> >-  changeName: function changeName(input) {
> >+  loadInputs: function loadInputs() {
> >+    this._settings.account.value = Weave.Service.account || "";
> >+    this._settings.pass.value = Weave.Service.password || "";
> >+    let pp = Weave.Service.passphrase || "";
> >+    if (pp.length == 20)
> >+      pp = this.hyphenatePassphrase(pp);
> 
> Can we rename the var pp to something else (passPhrase?) and encapsulate the
> length check into hyphenatePassPhrase?

Note that hyphenatePassphrase and normalizePassphrase are going to be available from Weave.Utils soon (=after the next merge). See comment 1.

> >+  normalizePassphrase: function(pp) {
> >+    // Remove hyphens as inserted by hyphenatePassphrase()
> >+    if (pp.length == 23 && pp[5] == '-' && pp[11] == '-' && pp[17] == '-')
> >+      return pp.slice(0, 5) + pp.slice(6, 11) + pp.slice(12, 17) + pp.slice(18, 23);
> >+    return pp;
> 
> Can you use another var name than pp?
> I'm pretty sure we can do this clean up with a nice regular expression, maybe
> something like return passPhrase.replace(/^(.{5})-(.{5})-(.{5})-(.{5})$/,
> "$1$2$3$4"); should work?

See above (comment 1). Also: regular expressions are icky. :)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

I think Philipp handled the review comments, but the summary:
* Weave.Service.login() need username
* The observer topics shouldn't change
* hyphenatePassphrase and normalizePassphrase are temporary and will leave
Fennec soon, so I didn't change the code.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/2b0205277b24

Comment 7

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b7pre) Gecko/20100924 Firefox/4.0b7pre Fennec/2.0b1pre	

and 

Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20100924 Firefox/4.0b7pre Fennec/2.0b1pre

Comment 8

[Tony Chung [:tchung]]

Litmus tests:
https://litmus.mozilla.org/show_test.cgi?id=12590
https://litmus.mozilla.org/show_test.cgi?id=12966
https://litmus.mozilla.org/show_test.cgi?id=12967