Add functions to let tests read and control the Bluetooth chooser state.

Add functions to let tests read and control the Bluetooth chooser state.

These functions are specified at https://webbluetoothcg.github.io/web-bluetooth/tests/.

See tests using this in https://codereview.chromium.org/1304353004/.

BUG=500989
Committed: https://crrev.com/8057c00a4b45dd766660fa199d0e2aebf41119cc
Cr-Commit-Position: refs/heads/master@{#348202}

[Jeffrey Yasskin, 2015-09-02 01:52:02]

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

[Jeffrey Yasskin, 2015-09-02 01:54:26]

What do you think? This gives us the ability to check what events a chooser has
seen, but doesn't have a "wait for X" function aside from spinning. I don't see
another test function with a callback, so "wait for X" may be complicated.

[ortuno, 2015-09-02 16:14:25]

So many layers of indirection D: Just a couple of comments/questions. Also what
would we use 'wait for X' for?

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc
(right):

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:15:
// showing a dialog, selects the
Line wrapping.

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:35:
if (!factory_) {
Q: When would the factory be destroyed before the chooser?

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:111:
std::vector<Chooser*> choosers_copy(choosers_.begin(), choosers_.end());
This code is the same as SendEvent. Are calls to other functions usually not
done inside a destructor?

[scheib, 2015-09-02 16:43:41]

We should decide if we're intending chooser tests to be portable to other
browser implementations. To date the fake adapters are fairly well documented
(though we'd want to do a better job) and portable. The main exception being the
calls to generate all forms of GATT errors being implementation specific.

Some of the chooser design is easily portable. E.g. selecting / canceling the
chooser. Recording the events is probably portable and would have value, but is
using implementation specific method names. 

If we want to make tests portable, I think we should document the design. If we
decide that chooser implementation details warrant implementation specific tests
we may want to partition layout tests earlier than later to avoid cleanup work.
Alternatively any impl specific chooser tests may be best as unit tests instead
of layout?

Design wise, I don't think we currently have a way for the chooser to receive
devices after the chooser starts? The mock adapter will be set and then at
requestDevice all known devices will be added, right? What if we get device
updates with a different set of advertised services, and only a 2nd update
passes chooser filters?

[Jeffrey Yasskin, 2015-09-02 17:00:45]

On 2015/09/02 16:43:41, scheib wrote:
> We should decide if we're intending chooser tests to be portable to other
> browser implementations. To date the fake adapters are fairly well documented
> (though we'd want to do a better job) and portable. The main exception being
the
> calls to generate all forms of GATT errors being implementation specific.
> 
> Some of the chooser design is easily portable. E.g. selecting / canceling the
> chooser. Recording the events is probably portable and would have value, but
is
> using implementation specific method names. 

I think we do want the chooser tests to be portable, and I'm happy to change the
event names to make that easier. Do you prefer any particular style?

> If we want to make tests portable, I think we should document the design. If
we
> decide that chooser implementation details warrant implementation specific
tests
> we may want to partition layout tests earlier than later to avoid cleanup
work.
> Alternatively any impl specific chooser tests may be best as unit tests
instead
> of layout?

Where are we currently documenting the signatures and behaviors of testRunner
methods?

> Design wise, I don't think we currently have a way for the chooser to receive
> devices after the chooser starts? The mock adapter will be set and then at
> requestDevice all known devices will be added, right? What if we get device
> updates with a different set of advertised services, and only a 2nd update
> passes chooser filters?

Right, the "advance test step" call isn't in this CL, and would need to call
through to the mock adapter.

I'm getting more nervous about the lack of "wait for X", since when we tell the
adapter to change its state, like with the discovery system changing state, that
may not happen before the renderer next calls getBluetoothManualChooserEvents,
which would force another delay loop. :-/ I'll look into how to do that sort of
callback, and whether there's a straightforward way to return a promise from
components/test_runner.

[scheib, 2015-09-02 17:29:08]

On 2015/09/02 17:00:45, Jeffrey Yasskin wrote:
> On 2015/09/02 16:43:41, scheib wrote:
> > We should decide if we're intending chooser tests to be portable to other
> > browser implementations. To date the fake adapters are fairly well
documented
> > (though we'd want to do a better job) and portable. The main exception being
> the
> > calls to generate all forms of GATT errors being implementation specific.
> > 
> > Some of the chooser design is easily portable. E.g. selecting / canceling
the
> > chooser. Recording the events is probably portable and would have value, but
> is
> > using implementation specific method names. 
> 
> I think we do want the chooser tests to be portable, and I'm happy to change
the
> event names to make that easier. Do you prefer any particular style?

My imagination hasn't had me think of how we'll use these events in tests yet.
If we had an example test or two in a design doc it would help.

> 
> > If we want to make tests portable, I think we should document the design. If
> we
> > decide that chooser implementation details warrant implementation specific
> tests
> > we may want to partition layout tests earlier than later to avoid cleanup
> work.
> > Alternatively any impl specific chooser tests may be best as unit tests
> instead
> > of layout?
> 
> Where are we currently documenting the signatures and behaviors of testRunner
> methods?

https://docs.google.com/document/d/1MBOLc1J09xhmEy9As17Rii4asM7u60LluRIv-OPPA...
has the overview of the testing plan with mock data sets, and the mock data sets
were initially described in
https://docs.google.com/document/d/1_QsBzcc84SwF7oaBWbO8rBzn39MISQ3w-6QM9hk0L...
and are more accurately described in the header in code
src/content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h

I'm not sure when -- perhaps fairly soon -- we should try to actually formally
spec the test data plans. We may want to open discussions with Mozilla if
they're closer to implementing and do this in a way that will work for them.

> 
> > Design wise, I don't think we currently have a way for the chooser to
receive
> > devices after the chooser starts? The mock adapter will be set and then at
> > requestDevice all known devices will be added, right? What if we get device
> > updates with a different set of advertised services, and only a 2nd update
> > passes chooser filters?
> 
> Right, the "advance test step" call isn't in this CL, and would need to call
> through to the mock adapter.
> 
> I'm getting more nervous about the lack of "wait for X", since when we tell
the
> adapter to change its state, like with the discovery system changing state,
that
> may not happen before the renderer next calls getBluetoothManualChooserEvents,
> which would force another delay loop. :-/ I'll look into how to do that sort
of
> callback, and whether there's a straightforward way to return a promise from
> components/test_runner.

[Jeffrey Yasskin, 2015-09-02 18:24:01]

On Wed, Sep 2, 2015 at 10:29 AM, <scheib@chromium.org> wrote:

> On 2015/09/02 17:00:45, Jeffrey Yasskin wrote:
>
>> On 2015/09/02 16:43:41, scheib wrote:
>> > We should decide if we're intending chooser tests to be portable to
>> other
>> > browser implementations. To date the fake adapters are fairly well
>>
> documented
>
>> > (though we'd want to do a better job) and portable. The main exception
>> being
>> the
>> > calls to generate all forms of GATT errors being implementation
>> specific.
>> >
>> > Some of the chooser design is easily portable. E.g. selecting /
>> canceling
>>
> the
>
>> > chooser. Recording the events is probably portable and would have
>> value, but
>> is
>> > using implementation specific method names.
>>
>
> I think we do want the chooser tests to be portable, and I'm happy to
>> change
>>
> the
>
>> event names to make that easier. Do you prefer any particular style?
>>
>
> My imagination hasn't had me think of how we'll use these events in tests
> yet.
> If we had an example test or two in a design doc it would help.


In addition to the tests in
https://codereview.chromium.org/1304353004/diff/1/LayoutTests/bluetooth/reque...,
we'll want to check that when the adapter changes state (presence,
powered-on/off, new devices, removed devices), the chooser is informed in
the right ways. I think we don't need to put the "help button clicked"
events in the public tests, since those UI facets are
implementation-specific.

> If we want to make tests portable, I think we should document the design.
>> If
>> we
>> > decide that chooser implementation details warrant implementation
>> specific
>> tests
>> > we may want to partition layout tests earlier than later to avoid
>> cleanup
>> work.
>> > Alternatively any impl specific chooser tests may be best as unit tests
>> instead
>> > of layout?
>>
>
> Where are we currently documenting the signatures and behaviors of
>> testRunner
>> methods?
>>
>
>
>
https://docs.google.com/document/d/1MBOLc1J09xhmEy9As17Rii4asM7u60LluRIv-OPPA...
> has the overview of the testing plan with mock data sets, and the mock
> data sets
> were initially described in
>
>
https://docs.google.com/document/d/1_QsBzcc84SwF7oaBWbO8rBzn39MISQ3w-6QM9hk0L...
> and are more accurately described in the header in code
>
>
src/content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
>
> I'm not sure when -- perhaps fairly soon -- we should try to actually
> formally
> spec the test data plans. We may want to open discussions with Mozilla if
> they're closer to implementing and do this in a way that will work for
> them.


They're a couple weeks away from implementing, so it does make sense to
start talking to them about sharing the tests. We'll eventually want to put
the tests in https://github.com/w3c/web-platform-tests/, but for now, maybe
https://github.com/WebBluetoothCG/web-bluetooth/tests? I can start a
testRunner.md there that documents the per-browser API, both the adapters
and the chooser events.

> Design wise, I don't think we currently have a way for the chooser to
>>
> receive
>
>> > devices after the chooser starts? The mock adapter will be set and then
>> at
>> > requestDevice all known devices will be added, right? What if we get
>> device
>> > updates with a different set of advertised services, and only a 2nd
>> update
>> > passes chooser filters?
>>
>
> Right, the "advance test step" call isn't in this CL, and would need to
>> call
>> through to the mock adapter.
>>
>
> I'm getting more nervous about the lack of "wait for X", since when we tell
>>
> the
>
>> adapter to change its state, like with the discovery system changing
>> state,
>>
> that
>
>> may not happen before the renderer next calls
>> getBluetoothManualChooserEvents,
>> which would force another delay loop. :-/ I'll look into how to do that
>> sort
>>
> of
>
>> callback, and whether there's a straightforward way to return a promise
>> from
>> components/test_runner.
>>
>
>
> https://codereview.chromium.org/1325953002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2015-09-09 20:47:22]

On 2015/09/02 17:29:08, scheib wrote:
> On 2015/09/02 17:00:45, Jeffrey Yasskin wrote:
> > On 2015/09/02 16:43:41, scheib wrote:
> > > We should decide if we're intending chooser tests to be portable to other
> > > browser implementations. To date the fake adapters are fairly well
> documented
> > > (though we'd want to do a better job) and portable. The main exception
being
> > the
> > > calls to generate all forms of GATT errors being implementation specific.
> > > 
> > > Some of the chooser design is easily portable. E.g. selecting / canceling
> the
> > > chooser. Recording the events is probably portable and would have value,
but
> > is
> > > using implementation specific method names. 
> > 
> > I think we do want the chooser tests to be portable, and I'm happy to change
> the
> > event names to make that easier. Do you prefer any particular style?
> 
> My imagination hasn't had me think of how we'll use these events in tests yet.
> If we had an example test or two in a design doc it would help.
> 
> > 
> > > If we want to make tests portable, I think we should document the design.
If
> > we
> > > decide that chooser implementation details warrant implementation specific
> > tests
> > > we may want to partition layout tests earlier than later to avoid cleanup
> > work.
> > > Alternatively any impl specific chooser tests may be best as unit tests
> > instead
> > > of layout?
> > 
> > Where are we currently documenting the signatures and behaviors of
testRunner
> > methods?
> 
>
https://docs.google.com/document/d/1MBOLc1J09xhmEy9As17Rii4asM7u60LluRIv-OPPA...
> has the overview of the testing plan with mock data sets, and the mock data
sets
> were initially described in
>
https://docs.google.com/document/d/1_QsBzcc84SwF7oaBWbO8rBzn39MISQ3w-6QM9hk0L...
> and are more accurately described in the header in code
> src/content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
> 
> I'm not sure when -- perhaps fairly soon -- we should try to actually formally
> spec the test data plans. We may want to open discussions with Mozilla if
> they're closer to implementing and do this in a way that will work for them.

I've started a spec in http://webbluetoothcg.github.io/web-bluetooth/tests/,
with a branch documenting the new interfaces from this CL at
https://rawgit.com/jyasskin/web-bluetooth-1/chooser-test-apis/tests/index.html.
I've also updated the tests to match the new names:
https://codereview.chromium.org/1304353004. Think this is ready to send to the
content/ owners?

[ortuno, 2015-09-09 20:49:29]

On 2015/09/09 at 20:47:22, jyasskin wrote:
> On 2015/09/02 17:29:08, scheib wrote:
> > On 2015/09/02 17:00:45, Jeffrey Yasskin wrote:
> > > On 2015/09/02 16:43:41, scheib wrote:
> > > > We should decide if we're intending chooser tests to be portable to
other
> > > > browser implementations. To date the fake adapters are fairly well
> > documented
> > > > (though we'd want to do a better job) and portable. The main exception
being
> > > the
> > > > calls to generate all forms of GATT errors being implementation
specific.
> > > > 
> > > > Some of the chooser design is easily portable. E.g. selecting /
canceling
> > the
> > > > chooser. Recording the events is probably portable and would have value,
but
> > > is
> > > > using implementation specific method names. 
> > > 
> > > I think we do want the chooser tests to be portable, and I'm happy to
change
> > the
> > > event names to make that easier. Do you prefer any particular style?
> > 
> > My imagination hasn't had me think of how we'll use these events in tests
yet.
> > If we had an example test or two in a design doc it would help.
> > 
> > > 
> > > > If we want to make tests portable, I think we should document the
design. If
> > > we
> > > > decide that chooser implementation details warrant implementation
specific
> > > tests
> > > > we may want to partition layout tests earlier than later to avoid
cleanup
> > > work.
> > > > Alternatively any impl specific chooser tests may be best as unit tests
> > > instead
> > > > of layout?
> > > 
> > > Where are we currently documenting the signatures and behaviors of
testRunner
> > > methods?
> > 
> >
https://docs.google.com/document/d/1MBOLc1J09xhmEy9As17Rii4asM7u60LluRIv-OPPA...
> > has the overview of the testing plan with mock data sets, and the mock data
sets
> > were initially described in
> >
https://docs.google.com/document/d/1_QsBzcc84SwF7oaBWbO8rBzn39MISQ3w-6QM9hk0L...
> > and are more accurately described in the header in code
> >
src/content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
> > 
> > I'm not sure when -- perhaps fairly soon -- we should try to actually
formally
> > spec the test data plans. We may want to open discussions with Mozilla if
> > they're closer to implementing and do this in a way that will work for them.
> 
> I've started a spec in http://webbluetoothcg.github.io/web-bluetooth/tests/,
with a branch documenting the new interfaces from this CL at
https://rawgit.com/jyasskin/web-bluetooth-1/chooser-test-apis/tests/index.html.
I've also updated the tests to match the new names:
https://codereview.chromium.org/1304353004. Think this is ready to send to the
content/ owners?

You still haven't address my comments :p

[Jeffrey Yasskin, 2015-09-09 21:26:08]

Oops, sorry Gio!

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc
(right):

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:15:
// showing a dialog, selects the
On 2015/09/02 16:14:25, ortuno wrote:
> Line wrapping.

Actually, the content's all wrong too. Fixed.

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:35:
if (!factory_) {
On 2015/09/02 16:14:25, ortuno wrote:
> Q: When would the factory be destroyed before the chooser?

Good point: the factory does cancel all the choosers when it's destroyed, so it
turns out I can CHECK this instead of returning. I think I'd like to keep the
WeakPtrs so that this becomes a clear assertion of the logic in
BluetoothDispatcherHost instead of something that looks like it might accept
another fix if it fails.

https://codereview.chromium.org/1325953002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:111:
std::vector<Chooser*> choosers_copy(choosers_.begin(), choosers_.end());
On 2015/09/02 16:14:25, ortuno wrote:
> This code is the same as SendEvent. Are calls to other functions usually not
> done inside a destructor?

Calls to non-virtual functions are fine in destructors. Fixed; thanks.

[scheib, 2015-09-09 21:43:24]

LGTM

[Jeffrey Yasskin, 2015-09-09 22:26:18]

jyasskin@chromium.org changed reviewers:
+ mkwst@chromium.org

[Jeffrey Yasskin, 2015-09-09 22:26:19]

Hi Mike, can you take a look at the new testing functions? There's a spec for
them at
https://rawgit.com/jyasskin/web-bluetooth-1/chooser-test-apis/tests/index.html.

[ortuno, 2015-09-09 22:29:10]

lgtm

https://codereview.chromium.org/1325953002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc
(right):

https://codereview.chromium.org/1325953002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:15:
// showing a dialog, selects the
Line wrapping.

[ortuno, 2015-09-09 22:29:35]

On 2015/09/09 at 22:29:10, ortuno wrote:
> lgtm
> 
>
https://codereview.chromium.org/1325953002/diff/20001/content/shell/browser/l...
> File
content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc
(right):
> 
>
https://codereview.chromium.org/1325953002/diff/20001/content/shell/browser/l...
> content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc:15:
// showing a dialog, selects the
> Line wrapping.

Oops, left over comment. Ignore.

[Mike West, 2015-09-10 06:49:50]

LGTM.

Please put a link to the (nicely done!) spec in the CL description.

[Jeffrey Yasskin, 2015-09-10 16:47:28]

On 2015/09/10 06:49:50, Mike West (gardening) wrote:
> LGTM.
> 
> Please put a link to the (nicely done!) spec in the CL description.

Good point. I've also added a comment in test_runner.cc linking to the spec.

[Jeffrey Yasskin, 2015-09-10 16:47:45]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-09-10 16:47:45]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
ortuno@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1325953002/#ps80001
(title: "Comment where the test functions are specified.")

[commit-bot: I haz the power, 2015-09-10 16:48:37]

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

[commit-bot: I haz the power, 2015-09-10 17:14:51]

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

[commit-bot: I haz the power, 2015-09-10 17:14:52]

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

[Jeffrey Yasskin, 2015-09-10 17:38:20]

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

[commit-bot: I haz the power, 2015-09-10 17:38:51]

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

[Jeffrey Yasskin, 2015-09-10 17:39:05]

jyasskin@chromium.org changed reviewers:
+ sky@chromium.org

[Jeffrey Yasskin, 2015-09-10 17:39:06]

Scott, ptal at components/html_viewer.

[Jeffrey Yasskin, 2015-09-10 17:53:41]

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

[commit-bot: I haz the power, 2015-09-10 17:54:22]

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

[sky, 2015-09-10 18:06:18]

LGTM

[Jeffrey Yasskin, 2015-09-10 18:28:03]

The CQ bit was unchecked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-09-10 18:28:39]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-09-10 18:28:39]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
ortuno@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1325953002/#ps120001
(title: "Add a forgotten semicolon. :-/")

[commit-bot: I haz the power, 2015-09-10 18:28:57]

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

[commit-bot: I haz the power, 2015-09-10 19:10:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-10 19:10:59]

Patchset 7 (id:??) landed as
https://crrev.com/8057c00a4b45dd766660fa199d0e2aebf41119cc
Cr-Commit-Position: refs/heads/master@{#348202}

[commit-bot: I haz the power, 2015-09-23 12:12:48]

Patchset 7 (id:??) landed as
https://crrev.com/8057c00a4b45dd766660fa199d0e2aebf41119cc
Cr-Commit-Position: refs/heads/master@{#348202}