Closed Bug 1127004 Opened 7 years ago Closed 7 years ago

WiFi auth server dialog for B2G

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
2.2 S5 (6feb)
Tracking Status
firefox38 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(6 files, 2 obsolete files)

With bug 1103120 landed, we should override the server dialogs used on FxOS to include more connection details and scan QR codes.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1127004/jryans (obsolete) —
/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
Attachment #8558136 - Flags: review?(past)
Attachment #8558136 - Flags: review?(janx)
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
Attached image keon with bad prompt (obsolete) —
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.
Attached image 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.
(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.
(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.
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.
(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.
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?
Attachment #8558136 - Flags: review?(past) → review+
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
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.
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 on attachment 8558136 [details]
MozReview Request: bz://1127004/jryans

ReviewBoard is strange.
Attachment #8558136 - Flags: review?(janx) → review+
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.
Attachment #8558193 - Flags: review?(janx) → review-
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.
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. :)
(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.
Attachment #8558193 - Flags: review- → review?(janx)
Comment on attachment 8558193 [details] [review]
[PullReq] jryans:devtools-qr to mozilla-b2g:master

r+ with one nit, also assuming tests pass.
Attachment #8558193 - Flags: review?(janx) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/c0783a0a2d41
https://hg.mozilla.org/mozilla-central/rev/78af92a0f945
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
Attached image 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"?
Attachment #8558536 - Attachment description: IMG_0024.jpg → keon with bad prompt
Attachment #8558536 - Attachment is obsolete: true
(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.
Flags: needinfo?(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.
Flags: needinfo?(matej)
Comment on attachment 8562308 [details] [review]
[PullReq] jryans:qr-text-tweak to mozilla-b2g:master

Ship it!
Attachment #8562308 - Flags: review?(janx) → review+
Attachment #8558136 - Attachment is obsolete: true
Attachment #8619238 - Flags: review+
Attachment #8619239 - Flags: review+
You need to log in before you can comment on or make changes to this bug.