Closed Bug 598010 Opened 9 years ago Closed 9 years ago

Update Sync UI in Fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #476799 - Flags: review?(21)
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.
Attached patch patch 2Splinter Review
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
Assignee: nobody → mark.finkle
Attachment #476799 - Attachment is obsolete: true
Attachment #476838 - Flags: review?(21)
Attachment #476799 - Flags: review?(21)
tracking-fennec: --- → ?
OS: Linux → All
Hardware: x86_64 → All
tracking-fennec: ? → 2.0b2+
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).
Attachment #476838 - Flags: review?(21) → review+
(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. :)
Whiteboard: [fennec-checkin-postb1]
Flags: in-testsuite?
Flags: in-litmus?
Flags: in-litmus? → in-litmus?(tchung)
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.
pushed:
http://hg.mozilla.org/mobile-browser/rev/2b0205277b24
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb1]
Depends on: 598063
Blocks: 598063
No longer depends on: 598063
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
Status: RESOLVED → VERIFIED
Blocks: 599500
You need to log in before you can comment on or make changes to this bug.