Add tests for RequestDeviceOptions and clean up.

Add tests for RequestDeviceOptions and clean up.

This is the third of three patches to implement RequestDeviceOptions:

  [1] https://crrev.com/1182973002 (Main Blink implementation)
  [2] https://crrev.com/1172853004 (Chromium side)
  [3] This patch.

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

[Jeffrey Yasskin, 2015-06-15 21:27:16]

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

[Jeffrey Yasskin, 2015-06-15 21:27:17]

PTAL at the third leg of the requestDevice filters patch.

[scheib, 2015-06-15 23:47:15]

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

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:50: }).catch(function(e) {
Would be nice to have a .then(/* verify something worked */, /* assert_unreached
*/)

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:71: 
Hmm, more test ideas to consider:

- Multiple devices each match one of multiple filters.
  e.g. filters: [{services: [glucoseServiceUuid,  
                             heartRateServiceUuid]}]
  (returns either one of them).

- Multiple devices match a single filter?
  e.g. filters: [{services: [glucoseServiceUuid]}]
  (adapter has 2+ glucose devices, any one of them returned)

- More filters exist than devices, but one is returned.
  e.g. filters: [{services: [somethingNotFoundServiceUuid,  
                             heartRateServiceUuid,
                             somethingNotFoundServiceUuid2]}]
  (adapter has only heart rate device, it is returned)

[Jeffrey Yasskin, 2015-06-16 01:31:33]

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

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:50: }).catch(function(e) {
On 2015/06/15 23:47:14, scheib wrote:
> Would be nice to have a .then(/* verify something worked */, /*
assert_unreached
> */)

With your suggestion on the browser side, I can drop the .catch entirely. Simply
getting a device back verifies that it worked.

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:71: 
On 2015/06/15 23:47:14, scheib wrote:
> Hmm, more test ideas to consider:
> 
> - Multiple devices each match one of multiple filters.
>   e.g. filters: [{services: [glucoseServiceUuid]},  
>                  {services: [heartRateServiceUuid]}]
>   (returns either one of them).
> 
> - Multiple devices match a single filter?
>   e.g. filters: [{services: [glucoseServiceUuid]}]
>   (adapter has 2+ glucose devices, any one of them returned)

So these two are basically "doesn't crash" tests? I'd like to be able to assert
that we show _all_ of the devices in the dialog, but we don't have the dialog
hooked up yet, or a pathway to report which devices were shown. Maybe we should
wait on these until we have that?

> - More filters exist than devices, but one is returned.
>   e.g. filters: [{services: [somethingNotFoundServiceUuid]},  
>                  {services: [heartRateServiceUuid]},
>                  {services: [somethingNotFoundServiceUuid2]}]
>   (adapter has only heart rate device, it is returned)

I've added this one, and a couple others.

[scheib, 2015-06-16 03:11:48]

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

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:50: }).catch(function(e) {
On 2015/06/16 01:31:33, Jeffrey Yasskin wrote:
> On 2015/06/15 23:47:14, scheib wrote:
> > Would be nice to have a .then(/* verify something worked */, /*
> assert_unreached
> > */)
> 
> With your suggestion on the browser side, I can drop the .catch entirely.
Simply
> getting a device back verifies that it worked.

This one is nearly identical to "An extra filter doesn\'t prevent matching."
now, though it has optionalServices. Perhaps make them adjacent and name them
similarly or in another way to draw out the small diference. 

I think it's best as in the latter test to have the .then ...
assert_equals(device.name so that the expected result of the request is clear.

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:71: 
On 2015/06/16 01:31:33, Jeffrey Yasskin wrote:
> On 2015/06/15 23:47:14, scheib wrote:
> > Hmm, more test ideas to consider:
> > 
> > - Multiple devices each match one of multiple filters.
> >   e.g. filters: [{services: [glucoseServiceUuid]},  
> >                  {services: [heartRateServiceUuid]}]
> >   (returns either one of them).
> > 
> > - Multiple devices match a single filter?
> >   e.g. filters: [{services: [glucoseServiceUuid]}]
> >   (adapter has 2+ glucose devices, any one of them returned)
> 
> So these two are basically "doesn't crash" tests? 

Yes, not essential, but not a bad idea.

> I'd like to be able to assert
> that we show _all_ of the devices in the dialog, but we don't have the dialog
> hooked up yet, or a pathway to report which devices were shown. Maybe we
should
> wait on these until we have that?

OK, That's a good idea, though. Perhaps file an issue for ship blocking that we
bother to test the results of filters and what is shown? We'd have to plumb back
more info, and I'm unsure if we want to widen the testing API surface so much.
Later perhaps we'll see more nice to have testing API items and plan a design
for it.

> 
> > - More filters exist than devices, but one is returned.
> >   e.g. filters: [{services: [somethingNotFoundServiceUuid]},  
> >                  {services: [heartRateServiceUuid]},
> >                  {services: [somethingNotFoundServiceUuid2]}]
> >   (adapter has only heart rate device, it is returned)
> 
> I've added this one, and a couple others.

[Jeffrey Yasskin, 2015-06-16 17:18:08]

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

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:50: }).catch(function(e) {
On 2015/06/16 03:11:48, scheib wrote:
> On 2015/06/16 01:31:33, Jeffrey Yasskin wrote:
> > On 2015/06/15 23:47:14, scheib wrote:
> > > Would be nice to have a .then(/* verify something worked */, /*
> > assert_unreached
> > > */)
> > 
> > With your suggestion on the browser side, I can drop the .catch entirely.
> Simply
> > getting a device back verifies that it worked.
> 
> This one is nearly identical to "An extra filter doesn\'t prevent matching."
> now, though it has optionalServices. Perhaps make them adjacent and name them
> similarly or in another way to draw out the small diference. 
> 
> I think it's best as in the latter test to have the .then ...
> assert_equals(device.name so that the expected result of the request is clear.

The important thing for this test is the ScanFilterCheckingAdapter, not anything
about the returned data. We're asserting that the platform is asked to scan with
the UUIDs in the filtered services, but not any UUIDs in the optional services.
So any similarity with a later test is probably more distracting than something
to draw out.

I can add a comment to that effect, so it's clearer to the next reader.

There isn't really any expected result of the request, since the work happens in
the mock adapter, not in this test. It's not even important to have the promise
reject when the test fails, since the EXPECT_CALL failing will cause the test to
fail.

https://codereview.chromium.org/1174843002/diff/40001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:71: 
On 2015/06/16 03:11:48, scheib wrote:
> On 2015/06/16 01:31:33, Jeffrey Yasskin wrote:
> > I'd like to be able to assert
> > that we show _all_ of the devices in the dialog, but we don't have the
dialog
> > hooked up yet, or a pathway to report which devices were shown. Maybe we
> should
> > wait on these until we have that?
> 
> OK, That's a good idea, though. Perhaps file an issue for ship blocking that
we
> bother to test the results of filters and what is shown? We'd have to plumb
back
> more info, and I'm unsure if we want to widen the testing API surface so much.
> Later perhaps we'll see more nice to have testing API items and plan a design
> for it.

Here's an issue for wiring up the dialog in a testable way:
https://crbug.com/500989.

[scheib, 2015-06-16 17:25:18]

LGTM

[ortuno, 2015-06-18 22:04:42]

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

https://codereview.chromium.org/1174843002/diff/80001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:60: filters: [{services:
[glucoseServiceUuid]}]
If you pass something other than a string, say an object, the browser crashes.

[Jeffrey Yasskin, 2015-06-25 00:08:44]

On 2015/06/18 22:04:42, ortuno wrote:
>
https://codereview.chromium.org/1174843002/diff/80001/LayoutTests/bluetooth/r...
> File LayoutTests/bluetooth/requestDevice.html (right):
> 
>
https://codereview.chromium.org/1174843002/diff/80001/LayoutTests/bluetooth/r...
> LayoutTests/bluetooth/requestDevice.html:60: filters: [{services:
> [glucoseServiceUuid]}]
> If you pass something other than a string, say an object, the browser crashes.

Yeah, that's the "NOTREACHED() << "Error deserializing message " <<
msg->type();" at
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_message_u...,
caused by the object's string representation not being a valid UUID. It'll also
happen with any non-UUID string. In Release mode, it just causes the message to
be ignored.

This will get better once we call BluetoothUUID.getService() to filter the UUIDs
in Blink, but I think it's an acceptable intermediate state.

[ortuno, 2015-06-25 21:04:36]

lgtm

[Jeffrey Yasskin, 2015-06-25 21:04:57]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2015-06-25 21:05:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1174843002/80001

[commit-bot: I haz the power, 2015-06-25 22:03:26]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197861