1127004 - WiFi auth server dialog for B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With bug 1103120 landed, we should override the server dialogs used on FxOS to include more connection details and scan QR codes.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: bz://1127004/jryans

/r/3271 - Bug 1127004 - Use promises and session in b2g allowConnection. r=past
/r/3273 - Bug 1127004 - Receive OOB auth images from Gaia. r=janx

Pull down these commits:

hg pull review -r 43f550bc2e87e188932bbb1f59afe6605db2b219

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cef1978d79a

Comment 3

[GitHub Autolander Bot]

Attachment: [PullReq] jryans:devtools-qr to mozilla-b2g:master

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

To test this locally, it's still a bit cumbersome...

You'd need a device or b2g desktop build with these patches, plus a Firefox desktop build with patches from bug 1126965.

To enable everything, see how to toggle the feature flags[1].

Then, you should be able to connect from WebIDE.

[1]: https://wiki.mozilla.org/DevTools/WiFi_Debugging#Toggle_Feature_Flags

Comment 5

[Jan Keromnes [:janx]]

Attachment: keon with bad prompt

Comment 6

[Jan Keromnes [:janx]]

Nice to see this moving forward! Code looks good at first glance, I tried it, but instead of a QR code scanner I was asked for a token (see screenshot). In Firefox, clicking on "No QR scanner prompt?" shows me a screen asking me to copy "501e4c444e9d45ece90e5f1fc1d060b5cec4097305a1d9775a8908ef9e7e2325ed9e947d2aa4b96883d45ab5d9fcc2" to the device. Not typing that by hand :) I'll try something else.

Comment 7

[Jan Keromnes [:janx]]

Attachment: abort problem

Another problem I noticed, when you don't follow through on QR connection and abort, WebIDE looks as if you're connected to the device.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #6)
> Nice to see this moving forward! Code looks good at first glance, I tried
> it, but instead of a QR code scanner I was asked for a token (see
> screenshot). In Firefox, clicking on "No QR scanner prompt?" shows me a
> screen asking me to copy
> "501e4c444e9d45ece90e5f1fc1d060b5cec4097305a1d9775a8908ef9e7e2325ed9e947d2aa4
> b96883d45ab5d9fcc2" to the device. Not typing that by hand :) I'll try
> something else.

Yeah, this is really a backup plan and not the intended method! :)

In any case, I filed bug 1128978 to compress the token to at least reduce it's length somewhat.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #6)
> Nice to see this moving forward! Code looks good at first glance, I tried
> it, but instead of a QR code scanner I was asked for a token (see
> screenshot). In Firefox, clicking on "No QR scanner prompt?" shows me a
> screen asking me to copy
> "501e4c444e9d45ece90e5f1fc1d060b5cec4097305a1d9775a8908ef9e7e2325ed9e947d2aa4
> b96883d45ab5d9fcc2" to the device. Not typing that by hand :) I'll try
> something else.

Oh, and you should not reach the token text prompt on device assuming all patches were applied successfully.  The Gecko patch in this bug changes b2g to use the QR scanner instead.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

So far, I've only tested on a Flame, so I tried my Keon just now to compare with Jan's results.

For me, I do reach the QR scanning window, but it fails to show the camera output.  This seems to occur because |getUserMedia| doesn't exist on the Keon. :(

I'll double check an official build to see what happens there.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> So far, I've only tested on a Flame, so I tried my Keon just now to compare
> with Jan's results.
> 
> For me, I do reach the QR scanning window, but it fails to show the camera
> output.  This seems to occur because |getUserMedia| doesn't exist on the
> Keon. :(

Okay, this was a false alarm. |getUserMedia| works fine.  However, I initially got very distorted video (probably a driver issue).

A full flash of the latest Keon nightly gives working gUM with clear video output.  I then tried to re-pull blobs from the Keon and build again with my patches, but this caused the device to stop connecting to WiFi, similar to Jan.

I am not quite sure how to produce a functional Keon build with these patches applied at the moment.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

https://reviewboard.mozilla.org/r/3271/#review2723

::: toolkit/devtools/security/auth.js
(Diff revision 1)
> -   * Prompt the user to accept or decline the incoming connection. The default
> +   * Prompt the user to accept or decline the incoming connection.  The default

Oh, so the two spaces after a fullstop are intentional? How come?

::: b2g/chrome/content/devtools/debugger.js
(Diff revision 1)
> -    while(!this._promptDone) {
> +    return this._promptingForAllow;
> -      Services.tm.currentThread.processNextEvent(true);
> -    }

So, to satisfy my intellectual curiosity, how are we now making the dialog prompt modal without this hack?

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8558136 [details]
MozReview Request: bz://1127004/jryans

https://reviewboard.mozilla.org/r/3269/#review2725

Ship It!

Comment 14

[Jan Keromnes [:janx]]

Good news, I got your patches to work on my Flame! Really cool experience :)

- Back-camera image was slightly stretched, maybe because it can't use 100% of screen height.
- It's not clear what you're supposed to do with the camera. At first I looked for a "Take picture"-type button, but there was only "Cancel", so I tapped the screen. The second time that didn't work, so I thought maybe the video feed is being analyzed.
- It looks like having text in the top of the camera feed confuses the QR scanner. Maybe increasing the space around the QR code could help?
- When you take a long time to get the QR code right, WebIDE says "operation timed out" but connects you anyway.

This is great work! I'll take a little more time to review, but I'm confident this is really close to landing. Also, please remove the CSS file from the ignore-list as mentioned on GitHub, we'll fix the linter instead.

(In reply to Panos Astithas [:past] from comment #12)
> Oh, so the two spaces after a fullstop are intentional? How come?

It's supposed to improve readability of monospaced fonts by clearly separating sentences.[1] I don't really like it, but it's yet another thing people love to argue about on the web :) funny how finding the proper way to place letters on a screen can spawn so many heated discussions.

[1] http://en.wikipedia.org/wiki/Full_stop#Spacing_after_a_full_stop

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Last night, I tried this on my Nexus 4, and it shows the scanning prompt with video, but never seems to read the code successfully.  Anyway, I'll debug this soon, as I hope it's something trivial.

For the Keon, I am not sure what can be done, as there seems to be a mix of issues, some unrelated to this work entirely, that prevent the device from functioning, so I'll probably have to ignore it for now.

Comment 16

[Jan Keromnes [:janx]]

https://reviewboard.mozilla.org/r/3273/#review2729

Looks good to me! With two nits.

::: b2g/chrome/content/devtools/debugger.js
(Diff revision 1)
> -      return;
> +      return this._handleAllowResult(detail);

I don't think you need `return` here.

::: b2g/chrome/content/devtools/debugger.js
(Diff revision 1)
> -      this._handleAllowResult(detail.value);
> +      return this._handleAuthEvent(detail);

Same as above.

Comment 17

[Jan Keromnes [:janx]]

Comment on attachment 8558136 [details]
MozReview Request: bz://1127004/jryans

ReviewBoard is strange.

Comment 18

[Jan Keromnes [:janx]]

Comment on attachment 8558193 [details] [review]
[PullReq] jryans:devtools-qr to mozilla-b2g:master

Mostly looks good to me, but please 1) fix the localization strings and 2) remove the CSS file from the ignore list.

Landing strings we're planning on changing soon causes unnecessary translation work, and adding new entries to xfail.list should be avoided.

Comment 19

[Jan Keromnes [:janx]]

As for the other small issues I've mentioned in previous comments, I'm OK with leaving them to follow-ups, because all the prefs still default to off.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/3271/#review2749

> So, to satisfy my intellectual curiosity, how are we now making the dialog prompt modal without this hack?

In Gaia, the prompts just hide or show a set of elements, so |while| was not needed here for it to be modal.

It was present before only to wait for the dialog's result, so we would spin a loop until we had recevied an event with the dialog's result.

> Oh, so the two spaces after a fullstop are intentional? How come?

Haha, yeah.  Maybe Jan's link would back me up, but Wikipedia gives me a 503 error at the moment... :/

Anyway, I tend to use two spaces when writing in monospaced fonts, and one space in variable width fonts...  Not sure if that makes any real amount of sense though. :)

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #14)
> - Back-camera image was slightly stretched, maybe because it can't use 100%
> of screen height.

Filed bug 1130091.

> - It's not clear what you're supposed to do with the camera. At first I
> looked for a "Take picture"-type button, but there was only "Cancel", so I
> tapped the screen. The second time that didn't work, so I thought maybe the
> video feed is being analyzed.

I've made the header text sound more active.

> - It looks like having text in the top of the camera feed confuses the QR
> scanner. Maybe increasing the space around the QR code could help?

Perhaps, I've tweaked the client code to add a bit more space.

> - When you take a long time to get the QR code right, WebIDE says "operation
> timed out" but connects you anyway.

Filed bug 1130084.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Landed gecko side:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c0783a0a2d41
remote:   https://hg.mozilla.org/integration/fx-team/rev/78af92a0f945

Comment 23

[Jan Keromnes [:janx]]

Comment on attachment 8558193 [details] [review]
[PullReq] jryans:devtools-qr to mozilla-b2g:master

r+ with one nit, also assuming tests pass.

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Gaia merged:

https://github.com/mozilla-b2g/gaia/commit/d7ce178259a7622aea0e9458681a1e0a0f86cbe0

Comment 25

[Francesco Lodolo [:flod]]

I'm looking at the string landed on Gaia but I can't understand it at all.

devtools-auth-scan=Scanning for the QR code from your other device…

What exactly is the procedure? Am I supposed to scan a QR code on Firefox OS, or this means "Waiting/searching for the QR code from your other device…"? If it's latter, the choice of "scan" is extremely confusing.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0783a0a2d41
https://hg.mozilla.org/mozilla-central/rev/78af92a0f945

Comment 27

[Jan Keromnes [:janx]]

Attachment: scanning

(In reply to Francesco Lodolo [:flod] from comment #25)
> I'm looking at the string landed on Gaia but I can't understand it at all.
> 
> devtools-auth-scan=Scanning for the QR code from your other device…
> 
> What exactly is the procedure? Am I supposed to scan a QR code on Firefox
> OS, or this means "Waiting/searching for the QR code from your other
> device…"? If it's latter, the choice of "scan" is extremely confusing.

As you can see in the attached picture, the phone turns the camera on and is looking for a QR code displayed on the computer screeen, authenticating the WiFi "pairing".

Maybe the wording is not ideal, do you see a better way of saying "Firefox OS is analyzing this video feed, looking for a QR code, so please film your computer screen where the code is displayed"?

Comment 28

[Francesco Lodolo [:flod]]

(In reply to Jan Keromnes [:janx] from comment #27)
> Maybe the wording is not ideal, do you see a better way of saying "Firefox
> OS is analyzing this video feed, looking for a QR code, so please film your
> computer screen where the code is displayed"?

Current message: "Scanning for the QR code from your other device…"
Maybe something like "Scanning QR code displayed on the pairing/other device…" would be clearer?

Asking Matej who's definitely better than me with English.

Comment 29

[Matej Novak [:matej]]

Here are a few options that I think address this:

Scan the QR code displayed to complete pairing.

Waiting for QR code on the other device.

Looking for QR code generated by the other device.

Comment 30

[GitHub Autolander Bot]

Attachment: [PullReq] jryans:qr-text-tweak to mozilla-b2g:master

Comment 31

[Jan Keromnes [:janx]]

Comment on attachment 8562308 [details] [review]
[PullReq] jryans:qr-text-tweak to mozilla-b2g:master

Ship it!

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Merged text tweak and l10n fix:

https://github.com/mozilla-b2g/gaia/commit/0a6c4b791b3f2ac47a402dbc79f81b2e037f3b0a

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1127004 - Receive OOB auth images from Gaia. r=janx

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1127004 - Use promises and session in b2g allowConnection. r=past