690616 - Notify JPAKEClient controller when pairing is starting

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

The receiving device displays the PIN. As soon as the PIN is entered on the sending device and the pairing process starts, the receiving device might want to show something else, e.g. "We're waiting for the thing to do its stuff".

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Implement onPairingStart

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Fix up UI

Implement for now empty onPairingStart methods in UI code. Bug 675823 will make use of them.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 563615 [details] [diff] [review]
Part 1 (v1): Implement onPairingStart

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

Just nits!

::: services/sync/modules/jpakeclient.js
@@ +106,5 @@
> + *   displayPIN(pin) -- Called when a PIN has been generated and is ready to
> + *     be displayed to the user. Only called on the client where the pairing
> + *     was initiated with 'receiveNoPIN()'.
> + * 
> + *   onPairingStart() -- Called when the pairing has started, in other words,

Comma is incorrect here. Either

  Called when the pairing has started; in other words,

or

  Called when the pairing has started, and messages are…

@@ +194,5 @@
>                  this._putStep,
>                  this._getStep,
>                  function(callback) {
> +                  // We fetched the first response from the other client,
> +                  // notify controller of the pairing starting.

s/,/./
s/notify/Notify/

@@ +202,4 @@
>                    // Now we can switch back to the smaller timeout.
>                    this._maxTries = Svc.Prefs.get("jpake.maxTries");
>                    callback();
>                  },

Any reason for this not being a _step like the others? Seems like _notifyControllerStep or _pairingStartedStep would be a decent name.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 563616 [details] [diff] [review]
Part 2 (v1): Fix up UI

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

Well, that was easy.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #3)
> >                    // Now we can switch back to the smaller timeout.
> >                    this._maxTries = Svc.Prefs.get("jpake.maxTries");
> >                    callback();
> >                  },
> 
> Any reason for this not being a _step like the others? Seems like
> _notifyControllerStep or _pairingStartedStep would be a decent name.

Because it's not a step of the JPAKE exchange itself, it's more of an implementation detail of the receiveNoPIN() method, so I feel like it should live in there.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Implement onPairingStart

Addressed nits and rebased on top of latest changes in bug 689428.

Comment 7

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/services-central/rev/9a842709365b
https://hg.mozilla.org/services/services-central/rev/55984bb11732

Comment 8

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/9a842709365b
https://hg.mozilla.org/mozilla-central/rev/55984bb11732