Test that the right events are sent to the Bluetooth chooser.

Test that the right events are sent to the Bluetooth chooser.

And that the chooser can control which device is returned from requestDevice().

Depends on https://codereview.chromium.org/1325953002 for testing functions, which are specified in https://webbluetoothcg.github.io/web-bluetooth/tests/.

BUG=500989
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202647

[Jeffrey Yasskin, 2015-09-02 01:51:46]

jyasskin@chromium.org changed reviewers:
+ ortuno@chromium.org, scheib@chromium.org

[Jeffrey Yasskin, 2015-09-17 21:23:54]

Patchset #3 (id:40001) has been deleted

[Jeffrey Yasskin, 2015-09-17 21:24:34]

The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 21:24:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304353004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304353004/20001

[Jeffrey Yasskin, 2015-09-17 21:27:24]

PTAL

[commit-bot: I haz the power, 2015-09-17 22:55:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-17 22:55:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2015-09-18 00:21:02]

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:140: let deadline = Date.now() + 500;
do we really need this? testharness will timeout the test after a while. Also
couldn't this potentially make the test flaky if the machine is under heavy
load?

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:143: Array.prototype.push.apply(events,
Array spreads! :p

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:149: let addDeviceRegex =
/^add-device\(([^)]+)\)=([0-9:]+)$/
Semicolon. I wish we had something like jshint to catch these.

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:151: let device =
addDeviceRegex.exec(addedDevice);
What about:

assert_regexp_match(events[1], GetRegexForDeviceEvent('Heart Rate Device'));
assert_regexp_match(events[2], GetRegexForDeviceEvent('Glucose Device'));


testRunner.sendBluetoothManualChooserEvent('selected',
                                             GetDeviceID(event[2]));


// bluetooth-helpers.js

function GetRegexForDeviceEvent(device_name) {
  return new RegExp("^add-device\\(" + device_name + "\\)=[0-9:]+$");
}

function GetDeviceIDFromEvent(event) {
  return /^add-device\([^)]+\)=([0-9:]+)$/.exec(event)[1];
}

[ortuno, 2015-09-18 00:23:38]

Also we should add a TODO or open an issue to remind us that we still don't have
test for when devices are added after the discovery session starts.

[Jeffrey Yasskin, 2015-09-18 01:27:44]

On 2015/09/18 00:23:38, ortuno wrote:
> Also we should add a TODO or open an issue to remind us that we still don't
have
> test for when devices are added after the discovery session starts.

Done: https://crbug.com/533189

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:140: let deadline = Date.now() + 500;
On 2015/09/18 00:21:02, ortuno wrote:
> do we really need this? testharness will timeout the test after a while. Also
> couldn't this potentially make the test flaky if the machine is under heavy
> load?

I guess we can do without it.

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:143: Array.prototype.push.apply(events,
On 2015/09/18 00:21:02, ortuno wrote:
> Array spreads! :p

Done. :)

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:149: let addDeviceRegex =
/^add-device\(([^)]+)\)=([0-9:]+)$/
On 2015/09/18 00:21:02, ortuno wrote:
> Semicolon. I wish we had something like jshint to catch these.

Done. I've also moved this to a class in resources/bluetooth-helpers.js because
I'm using it again in my next test.

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:151: let device =
addDeviceRegex.exec(addedDevice);
On 2015/09/18 00:21:02, ortuno wrote:
> What about:
> 
> assert_regexp_match(events[1], GetRegexForDeviceEvent('Heart Rate Device'));
> assert_regexp_match(events[2], GetRegexForDeviceEvent('Glucose Device'));
> 
> 
> testRunner.sendBluetoothManualChooserEvent('selected',
>                                              GetDeviceID(event[2]));
> 
> 
> // bluetooth-helpers.js
> 
> function GetRegexForDeviceEvent(device_name) {
>   return new RegExp("^add-device\\(" + device_name + "\\)=[0-9:]+$");
> }
> 
> function GetDeviceIDFromEvent(event) {
>   return /^add-device\([^)]+\)=([0-9:]+)$/.exec(event)[1];
> }

If we get live devices working, the order of these events may be difficult to
stabilize. The current code avoids depending on that order.

I'm also wary of building regexps from runtime data like device_name. What if
the name happens to include a '\'? We could escape it, but there's no built-in
function for that, and parsing it out isn't significantly more complicated. It
also avoids needing two similar regexps.

I've also noticed that [0-9:] isn't future-proof for IDs. I've switched to
allowing any character.

[Jeffrey Yasskin, 2015-09-18 01:30:25]

The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-18 01:30:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304353004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304353004/60001

[commit-bot: I haz the power, 2015-09-18 02:52:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-18 02:52:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2015-09-18 19:56:20]

lgtm

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:151: let device =
addDeviceRegex.exec(addedDevice);
On 2015/09/18 at 01:27:44, Jeffrey Yasskin wrote:
> On 2015/09/18 00:21:02, ortuno wrote:
> > What about:
> > 
> > assert_regexp_match(events[1], GetRegexForDeviceEvent('Heart Rate Device'));
> > assert_regexp_match(events[2], GetRegexForDeviceEvent('Glucose Device'));
> > 
> > 
> > testRunner.sendBluetoothManualChooserEvent('selected',
> >                                              GetDeviceID(event[2]));
> > 
> > 
> > // bluetooth-helpers.js
> > 
> > function GetRegexForDeviceEvent(device_name) {
> >   return new RegExp("^add-device\\(" + device_name + "\\)=[0-9:]+$");
> > }
> > 
> > function GetDeviceIDFromEvent(event) {
> >   return /^add-device\([^)]+\)=([0-9:]+)$/.exec(event)[1];
> > }
> 
> If we get live devices working, the order of these events may be difficult to
stabilize. The current code avoids depending on that order.
> 
> I'm also wary of building regexps from runtime data like device_name. What if
the name happens to include a '\'? We could escape it, but there's no built-in
function for that, and parsing it out isn't significantly more complicated. It
also avoids needing two similar regexps.
> 

Ahh good point.

> I've also noticed that [0-9:] isn't future-proof for IDs. I've switched to
allowing any character.

https://codereview.chromium.org/1304353004/diff/60001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1304353004/diff/60001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:141: while (events.length < 5) {
You do something similar in your other test. Have you noticed any pattern that
we can abstract? Something like:

let tester = new EventTester(testRunner);

tester.assert_events([
  {event: 'chooser-opened', domain: 'file:///'},
  {event: 'devices', devices: ['Heart Rate Device', 'Glucose Device']},
  {event: 'discovering'},
  {event: 'discovery-idle'}
]};

tester.getDevice('Glucose Device');

Or something like that. Just thinking out loud.

[Jeffrey Yasskin, 2015-09-18 21:06:39]

On 2015/09/18 19:56:20, ortuno wrote:
> lgtm
> 
>
https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
> File LayoutTests/bluetooth/requestDevice.html (right):
> 
>
https://codereview.chromium.org/1304353004/diff/20001/LayoutTests/bluetooth/r...
> LayoutTests/bluetooth/requestDevice.html:151: let device =
> addDeviceRegex.exec(addedDevice);
> On 2015/09/18 at 01:27:44, Jeffrey Yasskin wrote:
> > On 2015/09/18 00:21:02, ortuno wrote:
> > > What about:
> > > 
> > > assert_regexp_match(events[1], GetRegexForDeviceEvent('Heart Rate
Device'));
> > > assert_regexp_match(events[2], GetRegexForDeviceEvent('Glucose Device'));
> > > 
> > > 
> > > testRunner.sendBluetoothManualChooserEvent('selected',
> > >                                              GetDeviceID(event[2]));
> > > 
> > > 
> > > // bluetooth-helpers.js
> > > 
> > > function GetRegexForDeviceEvent(device_name) {
> > >   return new RegExp("^add-device\\(" + device_name + "\\)=[0-9:]+$");
> > > }
> > > 
> > > function GetDeviceIDFromEvent(event) {
> > >   return /^add-device\([^)]+\)=([0-9:]+)$/.exec(event)[1];
> > > }
> > 
> > If we get live devices working, the order of these events may be difficult
to
> stabilize. The current code avoids depending on that order.
> > 
> > I'm also wary of building regexps from runtime data like device_name. What
if
> the name happens to include a '\'? We could escape it, but there's no built-in
> function for that, and parsing it out isn't significantly more complicated. It
> also avoids needing two similar regexps.
> > 
> 
> Ahh good point.
> 
> > I've also noticed that [0-9:] isn't future-proof for IDs. I've switched to
> allowing any character.
> 
>
https://codereview.chromium.org/1304353004/diff/60001/LayoutTests/bluetooth/r...
> File LayoutTests/bluetooth/requestDevice.html (right):
> 
>
https://codereview.chromium.org/1304353004/diff/60001/LayoutTests/bluetooth/r...
> LayoutTests/bluetooth/requestDevice.html:141: while (events.length < 5) {
> You do something similar in your other test. Have you noticed any pattern that
> we can abstract? Something like:
> 
> let tester = new EventTester(testRunner);
> 
> tester.assert_events([
>   {event: 'chooser-opened', domain: 'file:///'},
>   {event: 'devices', devices: ['Heart Rate Device', 'Glucose Device']},
>   {event: 'discovering'},
>   {event: 'discovery-idle'}
> ]};
> 
> tester.getDevice('Glucose Device');
> 
> Or something like that. Just thinking out loud.

As discussed over IM, we'll postpone this until we have a few more tests, to
make sure it's the right abstraction.

This CL now depends on https://codereview.chromium.org/1351393002/ to fix the
tests on Windows.

[Jeffrey Yasskin, 2015-09-22 16:56:18]

The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 16:56:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304353004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304353004/100001

[Jeffrey Yasskin, 2015-09-22 17:14:27]

Hey Gio, can you look again? I had to switch to callbacks in
https://codereview.chromium.org/1351393002/ to avoid the windows error, and that
resulted in a new helper function.

[commit-bot: I haz the power, 2015-09-22 18:38:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 18:38:45]

Dry run: This issue passed the CQ dry run.

[ortuno, 2015-09-22 19:00:02]

lgtm

https://codereview.chromium.org/1304353004/diff/100001/LayoutTests/bluetooth/...
File LayoutTests/bluetooth/resources/bluetooth-helpers.js (right):

https://codereview.chromium.org/1304353004/diff/100001/LayoutTests/bluetooth/...
LayoutTests/bluetooth/resources/bluetooth-helpers.js:67: if (expected_count ===
undefined || events.length >= expected_count) {
Would it be better if the first line was:
expected_count = expected_count !== undefined ? expected_count : 0;

and then this could be simplified to:
if (events.length >= expected_count)

That way when we get default function parameters we can just remove the first
line.

nit: you shouldn't compare to undefined:
http://stackoverflow.com/questions/27509/detecting-an-undefined-object-proper...

[Jeffrey Yasskin, 2015-09-22 20:01:34]

https://codereview.chromium.org/1304353004/diff/100001/LayoutTests/bluetooth/...
File LayoutTests/bluetooth/resources/bluetooth-helpers.js (right):

https://codereview.chromium.org/1304353004/diff/100001/LayoutTests/bluetooth/...
LayoutTests/bluetooth/resources/bluetooth-helpers.js:67: if (expected_count ===
undefined || events.length >= expected_count) {
On 2015/09/22 19:00:02, ortuno wrote:
> Would it be better if the first line was:
> expected_count = expected_count !== undefined ? expected_count : 0;
> 
> and then this could be simplified to:
> if (events.length >= expected_count)
> 
> That way when we get default function parameters we can just remove the first
> line.

Good idea. Done.

> nit: you shouldn't compare to undefined:
>
http://stackoverflow.com/questions/27509/detecting-an-undefined-object-proper...

This isn't true anymore. Trying to overwrite 'undefined' has no effect.

[Jeffrey Yasskin, 2015-09-22 20:01:49]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-09-22 20:01:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/1304353004/#ps120001
(title: "Give expected_count a default.")

[commit-bot: I haz the power, 2015-09-22 20:02:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304353004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304353004/120001

[commit-bot: I haz the power, 2015-09-22 21:13:38]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202647