Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202
Committed: https://crrev.com/cab3123dd3d86927cbc5e310100ec498f130ea80
Cr-Commit-Position: refs/heads/master@{#373341}

[gogerald1, 2016-01-28 20:08:11]

Description was changed from

==========
refactor BLE

BUG=
==========

to

==========
refactor BLE

BUG=
==========

[gogerald1, 2016-01-29 00:07:12]

Patchset #1 (id:1) has been deleted

[gogerald1, 2016-01-29 00:11:05]

Description was changed from

==========
refactor BLE

BUG=
==========

to

==========
Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake
Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202
==========

[gogerald1, 2016-01-29 00:11:05]

gogerald@chromium.org changed reviewers:
+ scheib@chromium.org

[gogerald1, 2016-01-29 00:17:45]

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

[commit-bot: I haz the power, 2016-01-29 00:18:06]

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

[gogerald1, 2016-01-29 00:22:39]

Hey Vincent, PTAL, Refactor bluetooth_low_energy_win to prepare for simulating
fake Bluetooth low energy device.

[commit-bot: I haz the power, 2016-01-29 00:29:17]

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

[commit-bot: I haz the power, 2016-01-29 00:29:18]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[scheib, 2016-01-29 04:35:51]

I've reflected on the appropriate testing layer. At first considering if
wrapping/fakes should be done at an even lower level, e.g. the SetupDi* API
calls. That would provide the greatest test coverage, though the windows APIs do
look tedious to fake when we have a high level goal.

There is non trivial code that you intend to add to the bluetooth_low_energy_win
file:
https://codereview.chromium.org/1606013002/diff/100001/device/bluetooth/bluet...
so that should be tested in some way.

So a reasonable solution is the pattern started in this patch, and testing the
bluetooth_low_energy_win.cc code by using bluetooth_low_energy_win_unittest.cc.

However, in those unittests there will still need to be a way to isolate the
logic being added from the windows API calls. So, in the long run if that
separation needs to be done is there much savings in creating this testing layer
and having that lower level testing layer instead of only the lowest level?

And I think yes, because the amount of Windows specific errors being tested at
that very low level would be tedious to expose testing controls for at the cross
platform testing layer.

The same argument can be used to move the cross platform testing layer Fakes up
to the Task Manager level if it helped -- but with the expectation that Task
Manager and lower level was still covered by tests. I'll leave it to you to
decide what is the least effort, so long as:

+ BluetoothTest fixture tests can work for high level API features added.
+ bluetooth_low_energy_unittest (and perhaps
bluetooth_task_manager_win_unittest.cc) have tests added for any new
implementation at that level not covered by BluetoothTest tests.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win.cc (right):

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:621: static BluetoothLowEnergyHub*
instance_ = nullptr;
I think you'll want a base/lazy_instance.h as you will be leaking this
allocation. Or, you'll want to have a DeleteInstance() method.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:632: delete instance_;
The if isn't needed. just delete, it works on nullptr.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win.h (right):

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.h:107: // Wrap Windows Apis to
enumerate Bluetooth low energy devices and services.
Wraps Windows APIs used to access Bluetooth Low Energy devices, providing an
interface that can be replaced with fakes in tests.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.h:108: class BluetoothLowEnergyHub {
I think these names would be more descriptive:
BluetoothLowEnergyWrapper
BluetoothLowEnergyFake

[gogerald1, 2016-01-30 18:58:15]

Patchset #3 (id:60001) has been deleted

[gogerald1, 2016-01-30 19:05:49]

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

[commit-bot: I haz the power, 2016-01-30 19:06:04]

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

[commit-bot: I haz the power, 2016-01-30 19:15:41]

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

[commit-bot: I haz the power, 2016-01-30 19:15:42]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gogerald1, 2016-02-02 20:01:24]

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

[gogerald1, 2016-02-02 20:02:50]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-02-02 20:02:58]

Patchset #5 (id:120001) has been deleted

[gogerald1, 2016-02-02 20:06:02]

Patchset #5 (id:140001) has been deleted

[gogerald1, 2016-02-02 20:11:09]

Patchset #4 (id:100001) has been deleted

[gogerald1, 2016-02-02 20:17:57]

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

[gogerald1, 2016-02-02 20:18:14]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-02-02 21:36:19]

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

[commit-bot: I haz the power, 2016-02-02 21:43:22]

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

[gogerald1, 2016-02-02 21:43:53]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-02-02 21:44:00]

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

[commit-bot: I haz the power, 2016-02-02 21:48:16]

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

[gogerald1, 2016-02-02 21:49:07]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-02-02 21:50:33]

Patchset #4 (id:160001) has been deleted

[gogerald1, 2016-02-02 21:50:47]

Patchset #3 (id:80001) has been deleted

[gogerald1, 2016-02-02 21:54:12]

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

[commit-bot: I haz the power, 2016-02-02 21:56:43]

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

[gogerald1, 2016-02-02 22:00:21]

Hey Vincent, PTAL.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win.cc (right):

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:621: static BluetoothLowEnergyHub*
instance_ = nullptr;
On 2016/01/29 04:35:50, scheib wrote:
> I think you'll want a base/lazy_instance.h as you will be leaking this
> allocation. Or, you'll want to have a DeleteInstance() method.

Done.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:632: delete instance_;
On 2016/01/29 04:35:50, scheib wrote:
> The if isn't needed. just delete, it works on nullptr.

Done.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win.h (right):

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.h:107: // Wrap Windows Apis to
enumerate Bluetooth low energy devices and services.
On 2016/01/29 04:35:50, scheib wrote:
> Wraps Windows APIs used to access Bluetooth Low Energy devices, providing an
> interface that can be replaced with fakes in tests.

Done.

https://codereview.chromium.org/1646023002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.h:108: class BluetoothLowEnergyHub {
On 2016/01/29 04:35:50, scheib wrote:
> I think these names would be more descriptive:
> BluetoothLowEnergyWrapper
> BluetoothLowEnergyFake

Done.

[commit-bot: I haz the power, 2016-02-02 22:16:41]

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

[commit-bot: I haz the power, 2016-02-02 22:16:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[scheib, 2016-02-02 22:50:06]

LGTM

[gogerald1, 2016-02-02 23:31:55]

Patchset #4 (id:200001) has been deleted

[gogerald1, 2016-02-02 23:32:21]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2016-02-02 23:32:37]

The CQ bit was unchecked by gogerald@chromium.org

[gogerald1, 2016-02-02 23:35:54]

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

[commit-bot: I haz the power, 2016-02-02 23:39:45]

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

[commit-bot: I haz the power, 2016-02-02 23:51:16]

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

[commit-bot: I haz the power, 2016-02-02 23:51:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[gogerald1, 2016-02-03 15:13:02]

Patchset #4 (id:220001) has been deleted

[gogerald1, 2016-02-03 15:25:24]

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

[commit-bot: I haz the power, 2016-02-03 15:26:17]

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

[commit-bot: I haz the power, 2016-02-03 15:30:47]

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

[commit-bot: I haz the power, 2016-02-03 15:30:47]

Dry run: 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/...)

[gogerald1, 2016-02-03 17:10:24]

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

[gogerald1, 2016-02-03 17:10:47]

Patchset #4 (id:240001) has been deleted

[commit-bot: I haz the power, 2016-02-03 17:10:55]

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

[commit-bot: I haz the power, 2016-02-03 17:57:32]

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

[commit-bot: I haz the power, 2016-02-03 17:57:33]

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

[scheib, 2016-02-03 18:12:46]

Still LGTM

[scheib, 2016-02-03 18:13:08]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-02-03 18:14:14]

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

[gogerald1, 2016-02-03 20:07:11]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2016-02-03 20:07:11]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1646023002/#ps280001
(title: "export class BluetoothLowEnergyWrapper for test")

[commit-bot: I haz the power, 2016-02-03 20:07:51]

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

[commit-bot: I haz the power, 2016-02-03 21:20:39]

Description was changed from

==========
Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake
Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202
==========

to

==========
Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake
Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202
==========

[commit-bot: I haz the power, 2016-02-03 21:20:40]

Committed patchset #5 (id:280001)

[commit-bot: I haz the power, 2016-02-03 21:21:47]

Description was changed from

==========
Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake
Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202
==========

to

==========
Refactor bluetooth_low_energy_win to prepare for new Bluetooth test fixture

This CL refactors bluetooth_low_energy_win to prepare for simulating fake
Bluetooth low energy device for the new Bluetooth test fixture.

BUG=579202

Committed: https://crrev.com/cab3123dd3d86927cbc5e310100ec498f130ea80
Cr-Commit-Position: refs/heads/master@{#373341}
==========

[commit-bot: I haz the power, 2016-02-03 21:21:48]

Patchset 5 (id:??) landed as
https://crrev.com/cab3123dd3d86927cbc5e310100ec498f130ea80
Cr-Commit-Position: refs/heads/master@{#373341}