bluetooth: Move FactoryWrapper to device and expose a function for testing in chooser controller

bluetooth: Move FactoryWrapper to device and expose a function for testing in chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528
Committed: https://crrev.com/189e95876f948145773b4a1151716eadb58ca1a0
Cr-Commit-Position: refs/heads/master@{#404472}

[ortuno, 2016-06-10 16:16:26]

Description was changed from

==========
bluetooth: Move FactoryWrapper to device and add scan duration to client

This avoids the need access a member from RenderProcessHostImpl and
allows Clients to set their own scan duration.

BUG=615319
==========

to

==========
bluetooth: Move FactoryWrapper to device and add scan duration to client

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl

Adding GetBluetoothScanDuration to the embedder interfaces allows Clients to set
their own scan duration.

BUG=615319
==========

[ortuno, 2016-06-10 16:58:32]

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

[ortuno, 2016-06-10 16:58:33]

jyasskin: PTAL. I know we talked about merging BluetoothAdapterFactoryWrapper
with BluetoothAdapterFactory, but BluetoothAdapterFactoryWrapper is quite big
and I didn't think it was worth adding functions that we are planning on
removing.

[ortuno, 2016-06-10 16:58:37]

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

[commit-bot: I haz the power, 2016-06-10 16:58:50]

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

[commit-bot: I haz the power, 2016-06-10 17:11:10]

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

[commit-bot: I haz the power, 2016-06-10 17:11:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ortuno, 2016-06-10 17:17:56]

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

[commit-bot: I haz the power, 2016-06-10 17:18:15]

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

[commit-bot: I haz the power, 2016-06-10 22:23:24]

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

[commit-bot: I haz the power, 2016-06-10 22:23:25]

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, 2016-06-20 14:53:17]

Description was changed from

==========
bluetooth: Move FactoryWrapper to device and add scan duration to client

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl

Adding GetBluetoothScanDuration to the embedder interfaces allows Clients to set
their own scan duration.

BUG=615319
==========

to

==========
bluetooth: Move FactoryWrapper to device and add scan duration to client

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl

Adding GetBluetoothScanDuration to the embedder interfaces allows Clients to set
their own scan duration.

BUG=621528
==========

[ortuno, 2016-06-20 16:10:27]

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

[ortuno, 2016-06-20 16:10:28]

scheib: PTAL since jyasskin is OOO

[scheib, 2016-06-22 03:44:22]

LGTM with some suggestions:

https://codereview.chromium.org/2059543002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2059543002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:44: //
|scan_duration| is how long will a discovery session be active.
Remove comment for scan duration.

https://codereview.chromium.org/2059543002/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2059543002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:406: // Session should be active
for.
should be active. [delete 'for']

https://codereview.chromium.org/2059543002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory_wrapper.cc (right):

https://codereview.chromium.org/2059543002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory_wrapper.cc:60:
BluetoothAdapter::Observer* observer) {
If the thread checks were useful we can keep the general intent with
base/threading/thread_checker.h

[ortuno, 2016-06-23 21:55:00]

Thanks!

https://codereview.chromium.org/2059543002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2059543002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:44: //
|scan_duration| is how long will a discovery session be active.
On 2016/06/22 at 03:44:22, scheib wrote:
> Remove comment for scan duration.

Done.

https://codereview.chromium.org/2059543002/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2059543002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:406: // Session should be active
for.
On 2016/06/22 at 03:44:22, scheib wrote:
> should be active. [delete 'for']

Done.

https://codereview.chromium.org/2059543002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory_wrapper.cc (right):

https://codereview.chromium.org/2059543002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory_wrapper.cc:60:
BluetoothAdapter::Observer* observer) {
On 2016/06/22 at 03:44:22, scheib wrote:
> If the thread checks were useful we can keep the general intent with
base/threading/thread_checker.h

Done.

[ortuno, 2016-06-23 21:55:04]

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

[commit-bot: I haz the power, 2016-06-23 21:55:28]

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

[ortuno, 2016-06-23 22:43:29]

ortuno@chromium.org changed reviewers:
+ jam@chromium.org

[ortuno, 2016-06-23 22:43:30]

jam: PTAL at chrome/, content/browser/renderer_host/, content/public/ and
content/shell.

[commit-bot: I haz the power, 2016-06-24 00:53:18]

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

[commit-bot: I haz the power, 2016-06-24 00:53:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-06-27 16:21:08]

Do we have any use cases for letting the scan frequency be set by the embedder?

[ortuno, 2016-06-27 16:33:59]

On 2016/06/27 at 16:21:08, jam wrote:
> Do we have any use cases for letting the scan frequency be set by the
embedder?

Two use cases come to mind:

1. Content Shell should be able to set it to 0 for testing.
2. Electron currently restarts the scan after the timeout[1] because they want
to scan for longer. They could just set the timeout to whatever value they
wanted and remove the rescan logic.

[1]
https://github.com/electron/electron/blob/master/atom/browser/lib/bluetooth_c...

[ortuno, 2016-06-27 17:42:16]

On 2016/06/27 at 16:33:59, ortuno wrote:
> On 2016/06/27 at 16:21:08, jam wrote:
> > Do we have any use cases for letting the scan frequency be set by the
embedder?
> 
> Two use cases come to mind:
> 
> 1. Content Shell should be able to set it to 0 for testing.
> 2. Electron currently restarts the scan after the timeout[1] because they want
to scan for longer. They could just set the timeout to whatever value they
wanted and remove the rescan logic.
> 
> [1]
https://github.com/electron/electron/blob/master/atom/browser/lib/bluetooth_c...

Oops I misread your reply. This is not about a scanning frequency. It's for how
long a scan should be active for. See previous email for use cases.

[jam, 2016-06-27 19:42:19]

On 2016/06/27 17:42:16, ortuno wrote:
> On 2016/06/27 at 16:33:59, ortuno wrote:
> > On 2016/06/27 at 16:21:08, jam wrote:
> > > Do we have any use cases for letting the scan frequency be set by the
> embedder?
> > 
> > Two use cases come to mind:
> > 
> > 1. Content Shell should be able to set it to 0 for testing.

How does this affect tests? Is it just layout tests? How do they work now?

> > 2. Electron currently restarts the scan after the timeout[1] because they
want
> to scan for longer. They could just set the timeout to whatever value they
> wanted and remove the rescan logic.

How much of that file is that logic? I couldn't tell by the link for sure, but
it appeared small. If so, given that there's already a way for them to achieve
this we don't need to add a new API?

> > 
> > [1]
>
https://github.com/electron/electron/blob/master/atom/browser/lib/bluetooth_c...
> 
> Oops I misread your reply. This is not about a scanning frequency. It's for
how
> long a scan should be active for. See previous email for use cases.

[ortuno, 2016-06-27 19:59:30]

On 2016/06/27 at 19:42:19, jam wrote:
> On 2016/06/27 17:42:16, ortuno wrote:
> > On 2016/06/27 at 16:33:59, ortuno wrote:
> > > On 2016/06/27 at 16:21:08, jam wrote:
> > > > Do we have any use cases for letting the scan frequency be set by the
> > embedder?
> > > 
> > > Two use cases come to mind:
> > > 
> > > 1. Content Shell should be able to set it to 0 for testing.
> 
> How does this affect tests? Is it just layout tests? How do they work now?
> 

It's actually pretty bad right now. We have a class called
BluetoothAdapterFactoryWrapper that has a function called
SetBluetoothAdapterForTesting. An instance of that object lives in
RenderProcessHostImpl. When a LayoutTest starts it calls
BluetoothAdapterFactoryWrapper::SetBluetoothAdapterForTesting which sets the
scan time to 0. In WebBluetoothServiceImpl, we downcast RenderProcessHost to
RenderProcessHostImpl to access the scan time in its
BluetoothAdapterFactoryWrapper. Because the RenderProcessHost that
WebBluetoothServiceImpl is not always a RenderProcessHostImpl, it's an invalid
cast. This patch adds the scan time function to the embedder interface to avoid
the cast. I'm open to suggestions though, my goal is to avoid the cast in
WebBluetoothServiceImpl from RenderProcessHost to RenderProcessHostImpl and be
able to set a different scan duration in tests.

> > > 2. Electron currently restarts the scan after the timeout[1] because they
want
> > to scan for longer. They could just set the timeout to whatever value they
> > wanted and remove the rescan logic.
> 
> How much of that file is that logic? I couldn't tell by the link for sure, but
it appeared small. If so, given that there's already a way for them to achieve
this we don't need to add a new API?
> 

It's only a couple of lines but the downside is that the scan has to be
restarted every five seconds.

> > > 
> > > [1]
> >
https://github.com/electron/electron/blob/master/atom/browser/lib/bluetooth_c...
> > 
> > Oops I misread your reply. This is not about a scanning frequency. It's for
how
> > long a scan should be active for. See previous email for use cases.

[ortuno, 2016-07-07 20:31:03]

Description was changed from

==========
bluetooth: Move FactoryWrapper to device and add scan duration to client

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl

Adding GetBluetoothScanDuration to the embedder interfaces allows Clients to set
their own scan duration.

BUG=621528
==========

to

==========
bluetooth: Move FactoryWrapper to device and expose a function for testing in
chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528
==========

[ortuno, 2016-07-07 20:31:24]

Description was changed from

==========
bluetooth: Move FactoryWrapper to device and expose a function for testing in
chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528
==========

to

==========
bluetooth: Move FactoryWrapper to device and expose a function for testing in
chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528
==========

[ortuno, 2016-07-07 20:32:34]

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

[commit-bot: I haz the power, 2016-07-07 20:33:20]

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

[commit-bot: I haz the power, 2016-07-07 22:35:07]

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

[commit-bot: I haz the power, 2016-07-07 22:35:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[ortuno, 2016-07-07 23:14:50]

scheib: PTAL again. This latest patch no longer uses the client interface.
BluetoothDeviceChooserController exposes a static function that can be used so
that new instances of the class have a scan duration of 0.

[scheib, 2016-07-08 00:46:03]

LGTM

[ortuno, 2016-07-08 16:09:03]

ortuno@chromium.org changed reviewers:
+ nick@chromium.org

[ortuno, 2016-07-08 16:09:04]

nick: Since jam is OOO could you take a look at content/browser/renderer_host/,
content/test/, content/public, and content/shell?

[ncarter (slow), 2016-07-08 18:10:36]

lgtm. This looks like better layering.

[ortuno, 2016-07-08 18:58:01]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2016-07-08 18:58:48]

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

[commit-bot: I haz the power, 2016-07-08 20:07:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-08 20:07:10]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 20:08:56]

Description was changed from

==========
bluetooth: Move FactoryWrapper to device and expose a function for testing in
chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528
==========

to

==========
bluetooth: Move FactoryWrapper to device and expose a function for testing in
chooser controller

Moving BluetoothAdapterFactoryWrapper avoids the need to access a member from
RenderProcessHostImpl, which avoids casting from RenderProcessHost to
RenderProcessHostImpl.

The scan duration is a web bluetooth concept and therefore should not be part of
the FactoryWrapper in device. To that extent we expose a static function in
BluetoothDeviceChooserController that allows tests to set a scan duration of 0.

BUG=621528

Committed: https://crrev.com/189e95876f948145773b4a1151716eadb58ca1a0
Cr-Commit-Position: refs/heads/master@{#404472}
==========

[commit-bot: I haz the power, 2016-07-08 20:08:58]

Patchset 9 (id:??) landed as
https://crrev.com/189e95876f948145773b4a1151716eadb58ca1a0
Cr-Commit-Position: refs/heads/master@{#404472}