Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming CL will implement BluetoothRemoteGattServiceWin.

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

[gogerald1, 2016-02-09 18:58:28]

Description was changed from

==========
enable tests


remove unnecessary head file


compilable

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

to

==========
Add remote GATT service to BluetoothDeviceWin

This CL adds remote GATT service to BluetoothDeviceWin. The upcoming CL will
implement remote GATT service for Windows.

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

[gogerald1, 2016-02-09 18:59:37]

Description was changed from

==========
Add remote GATT service to BluetoothDeviceWin

This CL adds remote GATT service to BluetoothDeviceWin. The upcoming CL will
implement remote GATT service for Windows.

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

to

==========
Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming
CL will implement BluetoothRemoteGattServiceWin.

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

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

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

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

Patchset #1 (id:1) has been deleted

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

Patchset #1 (id:20001) has been deleted

[gogerald1, 2016-02-09 20:51:03]

Patchset #1 (id:40001) has been deleted

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

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-02-09 20:52:13]

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

[gogerald1, 2016-02-09 20:57:14]

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

[gogerald1, 2016-02-09 20:57:14]

Hi Vincent, PTAL.

[commit-bot: I haz the power, 2016-02-09 22:08:19]

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

[commit-bot: I haz the power, 2016-02-09 22:08:20]

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...)

[gogerald1, 2016-02-09 23:14:38]

Patchset #2 (id:100001) has been deleted

[scheib, 2016-02-09 23:24:57]

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_win.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.h:96: // The follwing methods are used to
send various GATT observer events to
These are copied from the bluez code. Instead of duplicating, please refactor
them into the base class BluetoothAdapter. The DCHECKs are fine, but will also
need GetAdapter to be added to BluetoothGattService. The subclasses can override
GetAdapter to provide the static_cast if needed.

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

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:336: break;
return true;
instead of break.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:338: // it == service_state.end() means
|service| no longer exist.
comment and if aren't needed, just return false.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:347: // Remove no longer exist GATT
service first.
First, remove GATT services that no longer exist.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:348: if (gatt_services_.size() != 0) {
The if is redundant with the for() predicates. 
Remove the if.
Fine to keep the block scope with {
}
so that to_be_removed is scoped.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:350: for (auto gatt_service :
gatt_services_) {
const auto&

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:355: for (auto service :
to_be_removed_services) {
const auto&

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:380:
static_cast<BluetoothAdapterWin*>(adapter_)->NotifyGattServiceAdded(
instead of static_cast everywhere make a BluetoothDeviceWin::GetAdapter similar
to
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_win.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:110: // Check whether GATT service with
|uuid| and |attribute_handle| has already
Descriptive, not imperative.
https://google.github.io/styleguide/cppguide.html#Function_Comments
I suggest "Checks if the GATT service with ..."

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:114: // Check whether |service| still
exist on device according to newly discovered
ditto

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:116: bool IsGattServiceExist(
DoesGattServiceExist

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win_fake.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:86: BLEDevice*
GetSimulatedBLEDevice(std::string device_address);
optional based on your plan, but seems reasonable to put this in the above block
just under SimulateBLEDevice? So e.g. Simulate__ and GetSimulated__ pairs are
adjacent.

[gogerald1, 2016-02-10 17:21:33]

Patchset #2 (id:120001) has been deleted

[gogerald1, 2016-02-10 17:22:58]

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

[commit-bot: I haz the power, 2016-02-10 17:24:42]

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

[commit-bot: I haz the power, 2016-02-10 17:45:01]

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

[commit-bot: I haz the power, 2016-02-10 17:45:01]

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_...)
  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...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gogerald1, 2016-02-10 17:53:37]

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

[gogerald1, 2016-02-10 17:53:43]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-02-10 17:56:01]

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

[gogerald1, 2016-02-10 18:00:56]

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_win.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.h:96: // The follwing methods are used to
send various GATT observer events to
On 2016/02/09 23:24:57, scheib wrote:
> These are copied from the bluez code. Instead of duplicating, please refactor
> them into the base class BluetoothAdapter. The DCHECKs are fine, but will also
> need GetAdapter to be added to BluetoothGattService. The subclasses can
override
> GetAdapter to provide the static_cast if needed.

Done.

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

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:336: break;
On 2016/02/09 23:24:57, scheib wrote:
> return true;
> instead of break.

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:338: // it == service_state.end() means
|service| no longer exist.
On 2016/02/09 23:24:57, scheib wrote:
> comment and if aren't needed, just return false. 

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:347: // Remove no longer exist GATT
service first.
On 2016/02/09 23:24:57, scheib wrote:
> First, remove GATT services that no longer exist.

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:348: if (gatt_services_.size() != 0) {
On 2016/02/09 23:24:57, scheib wrote:
> The if is redundant with the for() predicates. 
> Remove the if.
> Fine to keep the block scope with {
> }
> so that to_be_removed is scoped.

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:350: for (auto gatt_service :
gatt_services_) {
On 2016/02/09 23:24:57, scheib wrote:
> const auto&

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:355: for (auto service :
to_be_removed_services) {
On 2016/02/09 23:24:57, scheib wrote:
> const auto&

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.cc:380:
static_cast<BluetoothAdapterWin*>(adapter_)->NotifyGattServiceAdded(
On 2016/02/09 23:24:57, scheib wrote:
> instead of static_cast everywhere make a BluetoothDeviceWin::GetAdapter
similar
> to
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_win.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:110: // Check whether GATT service with
|uuid| and |attribute_handle| has already
On 2016/02/09 23:24:57, scheib wrote:
> Descriptive, not imperative.
> https://google.github.io/styleguide/cppguide.html#Function_Comments
> I suggest "Checks if the GATT service with ..."

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:114: // Check whether |service| still
exist on device according to newly discovered
On 2016/02/09 23:24:57, scheib wrote:
> ditto

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_win.h:116: bool IsGattServiceExist(
On 2016/02/09 23:24:57, scheib wrote:
> DoesGattServiceExist

Done.

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_win_fake.h (right):

https://codereview.chromium.org/1681853003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:86: BLEDevice*
GetSimulatedBLEDevice(std::string device_address);
On 2016/02/09 23:24:57, scheib wrote:
> optional based on your plan, but seems reasonable to put this in the above
block
> just under SimulateBLEDevice? So e.g. Simulate__ and GetSimulated__ pairs are
> adjacent.

Done.

[scheib, 2016-02-10 18:12:17]

LGTM

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

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

[commit-bot: I haz the power, 2016-02-10 19:17:52]

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_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gogerald1, 2016-02-10 20:14:45]

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

[commit-bot: I haz the power, 2016-02-10 20:17:28]

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

[commit-bot: I haz the power, 2016-02-10 22:09:51]

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

[commit-bot: I haz the power, 2016-02-10 22:09:52]

Dry run: This issue passed the CQ dry run.

[gogerald1, 2016-02-10 22:10:27]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2016-02-10 22:10:28]

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/1681853003/#ps180001
(title: "add PlatformSupportsLowEnergy check in the unittests")

[commit-bot: I haz the power, 2016-02-10 22:14:25]

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

[commit-bot: I haz the power, 2016-02-10 22:26:10]

Description was changed from

==========
Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming
CL will implement BluetoothRemoteGattServiceWin.

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

to

==========
Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming
CL will implement BluetoothRemoteGattServiceWin.

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

[commit-bot: I haz the power, 2016-02-10 22:26:11]

Committed patchset #3 (id:180001)

[Nico, 2016-02-11 02:29:53]

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

[Nico, 2016-02-11 02:29:54]

https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/6088/ste...

..\..\device\bluetooth\bluetooth_remote_gatt_service_win.cc(16,7) :  error:
field 'service_path_' will be initialized after field 'device_'
[-Werror,-Wreorder]
    : service_path_(service_path),
      ^

You seem to break that bot often. Consider using clang/win locally, see
https://chromium.googlesource.com/chromium/src/+/master/docs/clang.md

[chromium-reviews, 2016-02-11 15:28:49]

Thanks, will try it.

On Wed, Feb 10, 2016 at 9:29 PM, <thakis@chromium.org> wrote:

>
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/6088/ste...
>
> ..\..\device\bluetooth\bluetooth_remote_gatt_service_win.cc(16,7) :  error:
> field 'service_path_' will be initialized after field 'device_'
> [-Werror,-Wreorder]
>     : service_path_(service_path),
>       ^
>
> You seem to break that bot often. Consider using clang/win locally,
seehttps://chromium.googlesource.com/chromium/src/+/master/docs/clang.md
> https://codereview.chromium.org/1681853003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

Description was changed from

==========
Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming
CL will implement BluetoothRemoteGattServiceWin.

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

to

==========
Add BluetoothRemoteGattServiceWin to BluetoothDeviceWin

This CL adds BluetoothRemoteGattServiceWin to BluetoothDeviceWin. The upcoming
CL will implement BluetoothRemoteGattServiceWin.

BUG=579202

Committed: https://crrev.com/43eba589ad78fbae63cef04ad67e8468e60b1165
Cr-Commit-Position: refs/heads/master@{#374751}
==========

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

Patchset 3 (id:??) landed as
https://crrev.com/43eba589ad78fbae63cef04ad67e8468e60b1165
Cr-Commit-Position: refs/heads/master@{#374751}