J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Description • 10 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 1 • 10 years ago

/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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 2 • 10 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 4 • 10 years ago

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

	Jan Keromnes [:janx]
	Comment 6 • 10 years ago

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.

	Jan Keromnes [:janx]
	Comment 7 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 8 • 10 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 9 • 10 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 10 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 11 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 12 • 10 years ago

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?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 13 • 10 years ago

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

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

Ship It!

	Jan Keromnes [:janx]
	Comment 14 • 10 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 15 • 10 years ago

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.

	Jan Keromnes [:janx]
	Comment 16 • 10 years ago

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.

	Jan Keromnes [:janx]
	Comment 17 • 10 years ago

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

ReviewBoard is strange.

	Jan Keromnes [:janx]
	Comment 18 • 10 years ago

Comment on attachment 8558193 [details]
[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.

	Jan Keromnes [:janx]
	Comment 19 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 20 • 10 years ago

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. :)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 21 • 10 years ago

(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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 22 • 10 years ago

Landed gecko side:

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

	Jan Keromnes [:janx]
	Comment 23 • 10 years ago

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

r+ with one nit, also assuming tests pass.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 24 • 10 years ago

Gaia merged:

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

	Francesco Lodolo [:flod]
	Comment 25 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 10 years ago

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

	Jan Keromnes [:janx]
	Comment 27 • 10 years ago

(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"?

	Francesco Lodolo [:flod]
	Comment 28 • 10 years ago

(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.

	Matej Novak [:matej]
	Comment 29 • 10 years ago

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.

	Jan Keromnes [:janx]
	Comment 31 • 10 years ago

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

Ship it!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 32 • 10 years ago

Merged text tweak and l10n fix:

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