Add listener for AdapterDiscoveringChanged on BT chooser-controller

Add listener for AdapterDiscoveringChanged on BT chooser-controller

|AdapterDiscoveringChanged| can be called by device/bluetooth
when BT discovery stopped.

We add listener for that on bluetooth chooser-controller.
Then it will stops the discovery session and notifies
the chooser when discovery stops accidentally.

BUG=611852

[deejay, 2017-01-30 04:25:32]

Description was changed from

==========
Add listener for AdapterDiscoveringChanged on BT chooser-controller

|AdapterDiscoveringChanged| can be called by device/bluetooth
when BT discovery stopped.

We add listener for that on bluetooth chooser-controller.
Then it will stops the discovery session and notifies
the chooser when discovery stops accidentally.

BUG=611852
==========

to

==========
Add listener for AdapterDiscoveringChanged on BT chooser-controller

|AdapterDiscoveringChanged| can be called by device/bluetooth
when BT discovery stopped.

We add listener for that on bluetooth chooser-controller.
Then it will stops the discovery session and notifies
the chooser when discovery stops accidentally.

BUG=611852
==========

[deejay, 2017-01-30 04:25:32]

djmix.kim@samsung.com changed reviewers:
+ jyasskin@chromium.org

[deejay, 2017-01-30 04:26:47]

Dear Yasskin,
I've updated patch for Issue 611852 that is reported by you.
Please take a look this!

[deejay, 2017-02-13 14:23:07]

djmix.kim@samsung.com changed reviewers:
+ jlebel@chromium.org, ortuno@chromium.org, scheib@chromium.org

[deejay, 2017-02-13 14:23:47]

Dear reviewer, please take a look this patch!

[scheib, 2017-02-13 21:06:54]

Thank you for the patch, this looks reasonable. But, it would be good to
understand how this situation was encountered? Would you describe how you
discovered this? Then, I'd like to think if there is a test that is appropriate
to create.

https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
(!discovering && discovery_session_.get() &&
Remove ".get()", it is not required.

[scheib, 2017-02-13 21:08:43]

>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> (!discovering && discovery_session_.get() &&
> Remove ".get()", it is not required.

http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool

[deejay, 2017-02-14 08:56:59]

On 2017/02/13 21:06:54, scheib wrote:
> Thank you for the patch, this looks reasonable. But, it would be good to
> understand how this situation was encountered? Would you describe how you
> discovered this? Then, I'd like to think if there is a test that is
appropriate
> to create.
> 
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):
> 
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> (!discovering && discovery_session_.get() &&
> Remove ".get()", it is not required.

Thanks for reviewing.
You're right. I will consider test case that can help verifying current issue.
After that, I will update to new patch-set if I find related TCs.

Thanks!

[scheib, 2017-02-14 18:21:49]

On 2017/02/14 08:56:59, deejay wrote:
> On 2017/02/13 21:06:54, scheib wrote:
> > Thank you for the patch, this looks reasonable. But, it would be good to
> > understand how this situation was encountered? Would you describe how you
> > discovered this? Then, I'd like to think if there is a test that is
> appropriate
> > to create.
> > 
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > File content/browser/bluetooth/bluetooth_device_chooser_controller.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> > (!discovering && discovery_session_.get() &&
> > Remove ".get()", it is not required.
> 
> Thanks for reviewing.
> You're right. I will consider test case that can help verifying current issue.
> After that, I will update to new patch-set if I find related TCs.
> 
> Thanks!

I'd still love to hear how you encountered this situation?

For tests, creating a unittest is probably best. My second suggestion would be a
LayoutTest, but this much more complicated until we complete a test refactoring.

This file:
content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
has a test fixture that in SetUp() uses CreateWebBluetoothServiceForTesting() to
create a WebBluetoothServiceImpl.

This one shows how to set a fake adapter:
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
and there are examples here showing how to create fake adapters:
https://cs.chromium.org/chromium/src/content/shell/browser/layout_test/layout...
but for this test only a simple one will be needed I think

[jlebel, 2017-03-01 15:39:12]

jlebel@chromium.org changed reviewers:
- jlebel@chromium.org

[scheib, 2017-03-08 21:55:06]

If you have any questions please ask, can do it here or over email
web-bluetooth@chromium.org or chat on IRC or hangouts.

[deejay, 2017-03-22 16:50:48]

On 2017/02/14 18:21:49, scheib wrote:
> On 2017/02/14 08:56:59, deejay wrote:
> > On 2017/02/13 21:06:54, scheib wrote:
> > > Thank you for the patch, this looks reasonable. But, it would be good to
> > > understand how this situation was encountered? Would you describe how you
> > > discovered this? Then, I'd like to think if there is a test that is
> > appropriate
> > > to create.
> > > 
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > File content/browser/bluetooth/bluetooth_device_chooser_controller.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> > > (!discovering && discovery_session_.get() &&
> > > Remove ".get()", it is not required.
> > 
> > Thanks for reviewing.
> > You're right. I will consider test case that can help verifying current
issue.
> > After that, I will update to new patch-set if I find related TCs.
> > 
> > Thanks!
> 
> I'd still love to hear how you encountered this situation?
> 
> For tests, creating a unittest is probably best. My second suggestion would be
a
> LayoutTest, but this much more complicated until we complete a test
refactoring.
> 
> This file:
> content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
> has a test fixture that in SetUp() uses CreateWebBluetoothServiceForTesting()
to
> create a WebBluetoothServiceImpl.
> 
> This one shows how to set a fake adapter:
>
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
> and there are examples here showing how to create fake adapters:
>
https://cs.chromium.org/chromium/src/content/shell/browser/layout_test/layout...
> but for this test only a simple one will be needed I think

Dear scheib,
Sorry to late reply!
I've spent a holiday on Feb and I could not take care of it continuously.
Now I will start focusing current issue.

> Q : I'd still love to hear how you encountered this situation?
Honestly, I met this issue through the description of 'BUG=611852'.
It means that I could not verify yet with some test-case.
So I tried to make TCs following your suggestion, but it is still failing.

1) In case of UnitTests,
as you mentioned it, we can access to |WebBluetoothServiceImpl| via
CreateWebBluetoothServiceForTesting(). 
But AdapterDiscoveringChanged() is a private function, so we need to change to
public for testing. Is it OK?
Furthermore we need to create fake |BluetoothDeviceChooserController|, also it
should support start/stop
to discovery for checking discovery status.
If this class is only for testing a "AdapterDiscoveringChanged", it's a burden.
I'm not sure that is a necessary task or not.

2) In case of LayoutTests,
according from spec (https://webbluetoothcg.github.io/web-bluetooth/tests.html),

Each adapters from "setBluetoothMockDataSet" are tightly coupled with specific
use-case.
Is it possible to add new MockDataSet if I cannot find a fake adapter for
testing "AdapterDiscoveringChanged"?

3) Also refactoring is proceeding now as below. How about add TCs for this after
finishing?
- https://bugs.chromium.org/p/chromium/issues/detail?id=569709

[scheib, 2017-03-23 00:05:06]

On 2017/03/22 16:50:48, deejay wrote:
> On 2017/02/14 18:21:49, scheib wrote:
> > On 2017/02/14 08:56:59, deejay wrote:
> > > On 2017/02/13 21:06:54, scheib wrote:
> > > > Thank you for the patch, this looks reasonable. But, it would be good to
> > > > understand how this situation was encountered? Would you describe how
you
> > > > discovered this? Then, I'd like to think if there is a test that is
> > > appropriate
> > > > to create.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > > File content/browser/bluetooth/bluetooth_device_chooser_controller.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > > content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> > > > (!discovering && discovery_session_.get() &&
> > > > Remove ".get()", it is not required.
> > > 
> > > Thanks for reviewing.
> > > You're right. I will consider test case that can help verifying current
> issue.
> > > After that, I will update to new patch-set if I find related TCs.
> > > 
> > > Thanks!
> > 
> > I'd still love to hear how you encountered this situation?
> > 
> > For tests, creating a unittest is probably best. My second suggestion would
be
> a
> > LayoutTest, but this much more complicated until we complete a test
> refactoring.
> > 
> > This file:
> > content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
> > has a test fixture that in SetUp() uses
CreateWebBluetoothServiceForTesting()
> to
> > create a WebBluetoothServiceImpl.
> > 
> > This one shows how to set a fake adapter:
> >
>
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
> > and there are examples here showing how to create fake adapters:
> >
>
https://cs.chromium.org/chromium/src/content/shell/browser/layout_test/layout...
> > but for this test only a simple one will be needed I think
> 
> Dear scheib,
> Sorry to late reply!
> I've spent a holiday on Feb and I could not take care of it continuously.
> Now I will start focusing current issue.
> 
> > Q : I'd still love to hear how you encountered this situation?
> Honestly, I met this issue through the description of 'BUG=611852'.
> It means that I could not verify yet with some test-case.
> So I tried to make TCs following your suggestion, but it is still failing.
> 
> 1) In case of UnitTests,
> as you mentioned it, we can access to |WebBluetoothServiceImpl| via
> CreateWebBluetoothServiceForTesting(). 
> But AdapterDiscoveringChanged() is a private function, so we need to change to
> public for testing. Is it OK?
> Furthermore we need to create fake |BluetoothDeviceChooserController|, also it
> should support start/stop
> to discovery for checking discovery status.
> If this class is only for testing a "AdapterDiscoveringChanged", it's a
burden.
> I'm not sure that is a necessary task or not.
> 
> 2) In case of LayoutTests,
> according from spec
(https://webbluetoothcg.github.io/web-bluetooth/tests.html),
> 
> Each adapters from "setBluetoothMockDataSet" are tightly coupled with specific
> use-case.
> Is it possible to add new MockDataSet if I cannot find a fake adapter for
> testing "AdapterDiscoveringChanged"?
> 
> 3) Also refactoring is proceeding now as below. How about add TCs for this
after
> finishing?
> - https://bugs.chromium.org/p/chromium/issues/detail?id=569709

Thanks. I'll be OOO a few days, but will look and reply soon.

[scheib, 2017-03-27 20:17:50]

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

[commit-bot: I haz the power, 2017-03-27 20:18:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scheib, 2017-03-27 21:02:04]

On 2017/03/22 16:50:48, deejay wrote:
> On 2017/02/14 18:21:49, scheib wrote:
> > On 2017/02/14 08:56:59, deejay wrote:
> > > On 2017/02/13 21:06:54, scheib wrote:
> > > > Thank you for the patch, this looks reasonable. But, it would be good to
> > > > understand how this situation was encountered? Would you describe how
you
> > > > discovered this? Then, I'd like to think if there is a test that is
> > > appropriate
> > > > to create.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > > File content/browser/bluetooth/bluetooth_device_chooser_controller.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2663693002/diff/1/content/browser/bluetooth/b...
> > > > content/browser/bluetooth/bluetooth_device_chooser_controller.cc:471: if
> > > > (!discovering && discovery_session_.get() &&
> > > > Remove ".get()", it is not required.
> > > 
> > > Thanks for reviewing.
> > > You're right. I will consider test case that can help verifying current
> issue.
> > > After that, I will update to new patch-set if I find related TCs.
> > > 
> > > Thanks!
> > 
> > I'd still love to hear how you encountered this situation?
> > 
> > For tests, creating a unittest is probably best. My second suggestion would
be
> a
> > LayoutTest, but this much more complicated until we complete a test
> refactoring.
> > 
> > This file:
> > content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc
> > has a test fixture that in SetUp() uses
CreateWebBluetoothServiceForTesting()
> to
> > create a WebBluetoothServiceImpl.
> > 
> > This one shows how to set a fake adapter:
> >
>
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
> > and there are examples here showing how to create fake adapters:
> >
>
https://cs.chromium.org/chromium/src/content/shell/browser/layout_test/layout...
> > but for this test only a simple one will be needed I think
> 
> Dear scheib,
> Sorry to late reply!
> I've spent a holiday on Feb and I could not take care of it continuously.
> Now I will start focusing current issue.
> 
> > Q : I'd still love to hear how you encountered this situation?
> Honestly, I met this issue through the description of 'BUG=611852'.
> It means that I could not verify yet with some test-case.
> So I tried to make TCs following your suggestion, but it is still failing.

Thank you. Yes this does seem useful still as that scenario may happen.

> 
> 1) In case of UnitTests,

Let's do a layout test, because it will validate what happens at the Javascript
API which is pretty valuable, and by the time that happens will have same
coverage the unit test would.

But, here are a few notes w.r.t. unit tests:

> as you mentioned it, we can access to |WebBluetoothServiceImpl| via
> CreateWebBluetoothServiceForTesting(). 
> But AdapterDiscoveringChanged() is a private function, so we need to change to
> public for testing. Is it OK?

It would be OK to make the test fixture class a friend. This is done commonly in
chromium code. Sometimes a specific test is friended (FRIEND_TEST_ALL_PREFIXES),
but I advise creating a test fixture class and friending it, then having the
test fixture provide a helper method. That way one or multiple tests can be
added without needing to add new friend lines to the WebBluetoothServiceImpl
class.

> Furthermore we need to create fake |BluetoothDeviceChooserController|, also it
> should support start/stop
> to discovery for checking discovery status.

Do we? I'd prefer if we could use the real class as modified in this patch.

> If this class is only for testing a "AdapterDiscoveringChanged", it's a
burden.
> I'm not sure that is a necessary task or not.


> 
> 2) In case of LayoutTests,
> according from spec
(https://webbluetoothcg.github.io/web-bluetooth/tests.html),

That's out of date, (we do have a note at top of doc that says so). Yes,
refactoring is under way. The chooser control will likely be pretty similar. 

> 
> Each adapters from "setBluetoothMockDataSet" are tightly coupled with specific
> use-case.
> Is it possible to add new MockDataSet if I cannot find a fake adapter for
> testing "AdapterDiscoveringChanged"?

Use tests similar to
WebKit/LayoutTests/bluetooth/requestDevice/chooser/

which set up the mock adapter and then expect certain events to happen in the
chooser.

The mock adapters are created in 
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
and one can be modified, added (copy-pasted), as needed to cause the
AdapterDiscoveringChanged event to fire at a particular time. For example, see
the DisconnectingDuringServiceRetrievalAdapter or NoNameHeartRateAdapter (look
around at others).

content/shell/browser/layout_test/layout_test_bluetooth_chooser_factory.cc
implements a BluetoothChooser -- which is fetched via
LayoutTestBluetoothChooserFactory::RunBluetoothChooser from inside
BluetoothDeviceChooserController::GetDevice (delegate->RunBluetoothChooser).

> 
> 3) Also refactoring is proceeding now as below. How about add TCs for this
after
> finishing?
> - https://bugs.chromium.org/p/chromium/issues/detail?id=569709

I think refactoring will take a while, let's get this landed with a test. The
refactor will pass over it the same as those other layout tests.

[commit-bot: I haz the power, 2017-03-27 22:19:32]

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

[commit-bot: I haz the power, 2017-03-27 22:19:33]

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

[cco3, 2017-06-05 21:24:52]

cco3@chromium.org changed reviewers:
+ cco3@chromium.org

[cco3, 2017-06-05 21:24:53]

Hi deejay, would you be able to create an updated patch for this change?

[deejay, 2017-06-07 00:17:00]

On 2017/06/05 21:24:53, cco3 wrote:
> Hi deejay, would you be able to create an updated patch for this change?

Hi cco3, I've seen this comments now due to vacation.
Sure! I will update new patch-set until this week.!!

[cco3, 2017-06-07 16:39:29]

On 2017/06/07 00:17:00, deejay wrote:
> On 2017/06/05 21:24:53, cco3 wrote:
> > Hi deejay, would you be able to create an updated patch for this change?
> 
> Hi cco3, I've seen this comments now due to vacation.
> Sure! I will update new patch-set until this week.!!

Thanks, deejay!