624028 - Provide easy-setup from mobile to desktop

Status: VERIFIED FIXED

(Cloud Services :: General, defect)

Description

[Philipp von Weitershausen [:philikon]]

If your desktop machine dies or if you have two physically separate desktops, having a mobile connected to Sync can be handy if we exposed the Sync Key in the UI somehow (bug 620493) or, even better, allowed easy setup from the mobile to the desktop.

If we're fine with doing the typing on the mobile (right now you type the code into the device that sends credentials), this is purely a UI matter for Fennec. If we want to always have the desktop do the typing, we need to:

* implement receiveWithPIN and sendNoPIN methods in jpakeclient.js
* extend the Firefox UI to either show you a code or as you for one, depending on whether you're setting up from another desktop or a mobile
* provide an "Add a Device" UI on Fennec.

Comment 2

[Philipp von Weitershausen [:philikon]]

Bug 624028 already has a WIP patch, just FYI (especially rnewman)

Comment 4

[Richard Newman [:rnewman]]

(Fleshy answer intended for Bug 686478.)

See

  https://wiki.mozilla.org/Firefox/Features/Sync_Setup_Improvements

which transitively links to 

  http://people.mozilla.com/~faaborg/files/projects/sync/allFlows-i2.png

which includes flows for setup in ways which we do not currently support.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Implement "Pair a Device" dialog

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Add "Pair a Device" link to the home page

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Add "Pair a Device" to Sync prefs subgroup

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot of part 1 (v1)

Excuse the awful colour scheme on the Fennec desktop build.

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot of part 2 (v1)

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot of part 3 (v1)

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 568198 [details] [diff] [review]
Part 1 (v1): Implement "Pair a Device" dialog

Review of attachment 568198 [details] [diff] [review]:
-----------------------------------------------------------------

N.B., code review only; I haven't run this myself, because I'd be here all day waiting for a build.

Two personal opinions:

* I dislike the centered text in the wizard. The orphan ("device" in your screenshot) looks silly hanging out in the middle of the page.
* Seeing it in the flesh, so to speak, "Pair a Device" seems kinda lost and confusing on the home screen. No implication of *why*, or what this will involve.

::: mobile/chrome/content/sync.js
@@ +542,5 @@
>    }
>  };
> +
> +
> +const PIN_PART_LENGTH = 4;

Large parts of this section of the patch are identical to the contents of syncAddDevice.js.

They're not identical, but cross-linked comments would go a long way to avoid unintentional divergence in the future. ("Hey, look at syncAddDevice.js!".)

I just filed Bug 695879 to address one such divergence.

@@ +550,5 @@
> +
> +  open: function open() {
> +    this.code1.setAttribute("maxlength", PIN_PART_LENGTH);
> +    this.code2.setAttribute("maxlength", PIN_PART_LENGTH);
> +    this.code3.setAttribute("maxlength", PIN_PART_LENGTH);

Why do we use code[123] here, and pin[123] in syncAddDevice? Please either use 'pin' or file a followup to align syncAddDevice if you think that 'code' is significantly better.

(I err on the side of 'pin', given that we use PIN_PART_LENGTH...)

Comment 12

[Tony Chung [:tchung]]

Flagging for in-testsuite and in-litmus.  Will need testcases when this feature lands

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #11)
> Two personal opinions:

... that I'm going to ignore (not because I don't agree with them but because we need to move this feature forward, and this is the UX we've got).

> > +  open: function open() {
> > +    this.code1.setAttribute("maxlength", PIN_PART_LENGTH);
> > +    this.code2.setAttribute("maxlength", PIN_PART_LENGTH);
> > +    this.code3.setAttribute("maxlength", PIN_PART_LENGTH);
> 
> Why do we use code[123] here, and pin[123] in syncAddDevice? Please either
> use 'pin' or file a followup to align syncAddDevice if you think that 'code'
> is significantly better.

The rest of the Fennec code uses 'code', so I stuck with that. In terms of "existing style predominates", the Fennec code is "closer" to this code than the Firefox code, IMHO.

Unless there are any other objections, I'm going to land the patches as is, given the r+ and feedback from you and mfinkle.

Comment 14

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/services-central/rev/f76db08d998a
https://hg.mozilla.org/services/services-central/rev/1f46f62ddfe2
https://hg.mozilla.org/services/services-central/rev/15abbaa36850

Comment 15

[Tracy Walker [:tracy]]

easy setup with s-c from mobile to desktop is good.  Pair a Device works from both home page and prefs

Comment 16

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/f76db08d998a
https://hg.mozilla.org/mozilla-central/rev/1f46f62ddfe2
https://hg.mozilla.org/mozilla-central/rev/15abbaa36850