Closed Bug 690616 Opened 8 years ago Closed 8 years ago

Notify JPAKEClient controller when pairing is starting

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Whiteboard: [fixed in services][qa-])

Attachments

(2 files, 1 obsolete file)

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".
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #563615 - Flags: review?(rnewman)
Implement for now empty onPairingStart methods in UI code. Bug 675823 will make use of them.
Attachment #563616 - Flags: review?(rnewman)
Attachment #563616 - Attachment description: Part (v2): Fix up UI → Part 2 (v1): Fix up UI
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.
Attachment #563615 - Flags: review?(rnewman) → review+
Comment on attachment 563616 [details] [diff] [review]
Part 2 (v1): Fix up UI

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

Well, that was easy.
Attachment #563616 - Flags: review?(rnewman) → review+
(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.
Addressed nits and rebased on top of latest changes in bug 689428.
Attachment #563615 - Attachment is obsolete: true
Attachment #563985 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9a842709365b
https://hg.mozilla.org/mozilla-central/rev/55984bb11732
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Blocks: sync2sm
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.