Implement BluetoothRemoteGattServiceWin and related unit tests

Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

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

[gogerald1, 2016-02-13 04:09:05]

Description was changed from

==========
implement GATT service


implement GATT service

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

to

==========
Implement BLE GATT service

This CL implements BLE GATT service and related unit tests.

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

[gogerald1, 2016-02-13 04:09:16]

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

[commit-bot: I haz the power, 2016-02-13 04:09:21]

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

[commit-bot: I haz the power, 2016-02-13 04:44:00]

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

[commit-bot: I haz the power, 2016-02-13 04:44:01]

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

[gogerald1, 2016-02-13 19:37:42]

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

[commit-bot: I haz the power, 2016-02-13 19:37:52]

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

[gogerald1, 2016-02-13 20:33:54]

Description was changed from

==========
Implement BLE GATT service

This CL implements BLE GATT service and related unit tests.

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

to

==========
Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

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

[gogerald1, 2016-02-13 20:33:54]

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

[commit-bot: I haz the power, 2016-02-13 20:57:54]

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

[commit-bot: I haz the power, 2016-02-13 20:57:55]

Dry run: This issue passed the CQ dry run.

[gogerald1, 2016-02-13 21:07:10]

Hi Vincent, PTAL.

[scheib, 2016-02-13 22:57:19]

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

[scheib, 2016-02-13 22:57:20]

ortuno, would you please review while I'm OOO and I will do an owners pass
approximately the 23rd.

[ortuno, 2016-02-19 17:33:06]

Got to bluetooth_remote_gatt_characteristic_win.cc

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.cc:290: if
(force_update_device_for_test_)
Could you add a comment explaining why tests need this?

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
This only returns true for windows 8 and above. The win8 bot doesn't run
device_unittests so basically no bot would be able to run these tests.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:742:
(*out_included_services).reset(new BTH_LE_GATT_SERVICE[required_length]);
You do this in a couple of places: why don't use arrow operator?
out_included_services->reset(...);

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:43: return
parent_service_->GetIdentifier() + "_" +
I don't think you need to construct this every single time someone calls
GetIdentifier. You should initialize the identifier in the constructor.

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:84: 
What about the other two properties, ReliableWrite and WriteAuxiliaries? If you
can't get those yet add a todo linking to: crbug.com/548449

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:52: void Update();
This function name is too vague. Please make the name more explicit and add a
description.

[gogerald1, 2016-02-19 22:39:29]

Patchset #4 (id:60001) has been deleted

[gogerald1, 2016-02-19 22:45:03]

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.cc:290: if
(force_update_device_for_test_)
On 2016/02/19 17:33:05, ortuno wrote:
> Could you add a comment explaining why tests need this?

Done.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
On 2016/02/19 17:33:05, ortuno wrote:
> This only returns true for windows 8 and above. The win8 bot doesn't run
> device_unittests so basically no bot would be able to run these tests.

Done.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:742:
(*out_included_services).reset(new BTH_LE_GATT_SERVICE[required_length]);
On 2016/02/19 17:33:05, ortuno wrote:
> You do this in a couple of places: why don't use arrow operator?
> out_included_services->reset(...);

Done.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:43: return
parent_service_->GetIdentifier() + "_" +
On 2016/02/19 17:33:06, ortuno wrote:
> I don't think you need to construct this every single time someone calls
> GetIdentifier. You should initialize the identifier in the constructor.

Done.

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:84: 
On 2016/02/19 17:33:06, ortuno wrote:
> What about the other two properties, ReliableWrite and WriteAuxiliaries? If
you
> can't get those yet add a todo linking to: crbug.com/548449

Done.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/02/19 17:33:06, ortuno wrote:
> 2016

Done.

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:52: void Update();
On 2016/02/19 17:33:06, ortuno wrote:
> This function name is too vague. Please make the name more explicit and add a
> description.

Done. add a description should be enough.

[scheib, 2016-02-24 01:38:28]

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

[scheib, 2016-02-24 01:39:10]

Please run try jobs when posting code for review.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
On 2016/02/19 22:45:02, gogerald1 wrote:
> On 2016/02/19 17:33:05, ortuno wrote:
> > This only returns true for windows 8 and above. The win8 bot doesn't run
> > device_unittests so basically no bot would be able to run these tests.
> 
> Done.

It would have helped when replying 'Done' here to explain what was done to fix
it - since the code at this comment location didn't change and a reviewer won't
see in the patch diff what happened here.

Remove the skipping logic "if (!PlatformSupportsLowEnergy())" from the tests.
These InitWithFakeAdapter() tests should always run, even on OS versions that
are earlier than when the APIs are supported. The fakes should always work. I
made a mistake in earlier review not catching this -- please remove the skipping
logic from all tests using fake adapter.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.cc:294: if
(force_update_device_for_test_)
Consider moving force_update_device_for_test_ above as:
if (!device_win->IsEqual(*device_state) || force_update_device_for_test_) {

Keep the comment, though delete "Above ".

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.h:160: // Flag indicates to update
device, used by BluetootTestWin.
Flag indicating a device update must be forced in DevicesPolled.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:746: out_included_services->get(),
&required_length, BLUETOOTH_GATT_FLAG_NONE);
actual_length and required_length are in wrong argument positions.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:786: file.GetPlatformFile(),
service, actual_length,
ditto

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:97: // Note: |parent_service|
== nullptr if simulated service is a primary
|parent_service| may be nullptr to indicate a primary service.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:103: // Note: |parent_service|
== nullptr if removed service is a primary
ditto

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:105: void
SimulateBLEGattServiceRemove(BLEDevice* device,
SimulateBLEGattServiceRemoved

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:115: BLEGattCharacteristic*
SimulateCharacterisc(
SimulateCharacteristic. Note this doesn't follow the above names with 'BLEGatt'
in them. I think the 'BLEGatt' is redundant, you may want to refactor that out
in a later patch.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:89: //
characteristic_info_ (BTH_LE_GATT_CHARACTERISTIC). See crbug.com/588274.
Thanks for the comment. 
Change to: TODO(crbug.com/########) details.
When filing TODO issues they should be Type-Feature and always block a parent
issue to gather related work. I've created https://crbug.com/589304 for platform
limitations. If later e.g. refactoring issues are identified use
https://crbug.com/580406 (create a Windows refactoring issue for windows
specific refactoring issues).

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:116: // Simualtes success of discovering
included services of |service|. |uuids| is
Simulates

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:135: virtual void
SimulateGattCharacteristicRemove(
SimulateGattCharacteristicRemoved

[commit-bot: I haz the power, 2016-02-24 01:39:54]

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

[scheib, 2016-02-24 01:45:43]

I'm stopping now about half way through this patch, at 
device/bluetooth/bluetooth_remote_gatt_characteristic_win.h
I'll resume tomorrow.

Please keep in mind that the smaller the patches are the faster they can be
reviewed - as there's less code to consider at a given time, search, and
understand. 1 patch twice the size of 2 smaller patches takes more time.

[commit-bot: I haz the power, 2016-02-24 05:38:12]

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

[commit-bot: I haz the power, 2016-02-24 05:38:13]

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

[gogerald1, 2016-02-24 16:05:37]

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

[commit-bot: I haz the power, 2016-02-24 16:07:31]

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

[gogerald1, 2016-02-24 16:09:07]

Managed to implement one small feature in one CL, will try to keep my next CL
smaller.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.cc:294: if
(force_update_device_for_test_)
On 2016/02/24 01:39:09, scheib wrote:
> Consider moving force_update_device_for_test_ above as:
> if (!device_win->IsEqual(*device_state) || force_update_device_for_test_) {
> 
> Keep the comment, though delete "Above ".

The reason I keep it here is because I don't want to send device change
notification as normal device change.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_win.h:160: // Flag indicates to update
device, used by BluetootTestWin.
On 2016/02/24 01:39:09, scheib wrote:
> Flag indicating a device update must be forced in DevicesPolled.

Done.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:746: out_included_services->get(),
&required_length, BLUETOOTH_GATT_FLAG_NONE);
On 2016/02/24 01:39:10, scheib wrote:
> actual_length and required_length are in wrong argument positions.

Here actual_length means actual buffer size offered to OS. required_length means
OS required_length. As above (line 735), required_length used to let OS indicate
required buffer size.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:786: file.GetPlatformFile(),
service, actual_length,
On 2016/02/24 01:39:09, scheib wrote:
> ditto

Acknowledged.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:97: // Note: |parent_service|
== nullptr if simulated service is a primary
On 2016/02/24 01:39:10, scheib wrote:
> |parent_service| may be nullptr to indicate a primary service.

Done.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:103: // Note: |parent_service|
== nullptr if removed service is a primary
On 2016/02/24 01:39:10, scheib wrote:
> ditto

Done.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:105: void
SimulateBLEGattServiceRemove(BLEDevice* device,
On 2016/02/24 01:39:10, scheib wrote:
> SimulateBLEGattServiceRemoved

Done.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win_fake.h:115: BLEGattCharacteristic*
SimulateCharacterisc(
On 2016/02/24 01:39:10, scheib wrote:
> SimulateCharacteristic. Note this doesn't follow the above names with
'BLEGatt'
> in them. I think the 'BLEGatt' is redundant, you may want to refactor that out
> in a later patch.

Follow above names "BLEGatt" for now since all the other data structures and
interfaces followed. Refactor them out in a separate path at the end.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:89: //
characteristic_info_ (BTH_LE_GATT_CHARACTERISTIC). See crbug.com/588274.
On 2016/02/24 01:39:10, scheib wrote:
> Thanks for the comment. 
> Change to: TODO(crbug.com/########) details.
> When filing TODO issues they should be Type-Feature and always block a parent
> issue to gather related work. I've created https://crbug.com/589304 for
platform
> limitations. If later e.g. refactoring issues are identified use
> https://crbug.com/580406 (create a Windows refactoring issue for windows
> specific refactoring issues).

Done.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:116: // Simualtes success of discovering
included services of |service|. |uuids| is
On 2016/02/24 01:39:10, scheib wrote:
> Simulates

Done.

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:135: virtual void
SimulateGattCharacteristicRemove(
On 2016/02/24 01:39:10, scheib wrote:
> SimulateGattCharacteristicRemoved

Done.

[commit-bot: I haz the power, 2016-02-24 19:56:36]

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

[commit-bot: I haz the power, 2016-02-24 19:56:38]

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

[scheib, 2016-02-25 06:22:44]

Still not all the way through, will resume at bluetooth_task_manager_win.h

FYI this and previous rounds were a bit over 2 hours each and need to be
balanced with other work tasks.

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

https://codereview.chromium.org/1690133002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_win.cc:746: out_included_services->get(),
&required_length, BLUETOOTH_GATT_FLAG_NONE);
On 2016/02/24 16:09:06, gogerald1 wrote:
> On 2016/02/24 01:39:10, scheib wrote:
> > actual_length and required_length are in wrong argument positions.
> 
> Here actual_length means actual buffer size offered to OS. required_length
means
> OS required_length. As above (line 735), required_length used to let OS
indicate
> required buffer size.

When others are confused by your code and suspect a logic error it is a good
indication that the code isn't clear.  When that happens, change the code, or
add comments, so that all future readers / maintainers will understand. After
this review no one will read the review comments, so explaining only in review
comments isn't sufficient.

In this case a 'actual' and 'required' are a bit ambiguous. Required in
BluetoothGATTGetIncludedServices(,,,NULL) makes sense as the OS is informing us
what size is required to be allocated. Using the same variable again becomes
confusing, as the same name used It's used to then allocate, and that makes
sense. It doesn't make The array is constructed using required, but in this code
review response you use actual to mean the allocated size of the array.
'required' 

I suggest 'allocated_length' and 'out_counts' 

First call to BluetoothGATTGetIncludedServices(,,0,NULL,allocated_length)

new [allocated_length]

then BluetoothGATTGetIncludedServices(,, allocated_length,
out_included_services->get(), out_counts)

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_service_unittest.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
On 2016/02/24 01:39:09, scheib wrote:
> On 2016/02/19 22:45:02, gogerald1 wrote:
> > On 2016/02/19 17:33:05, ortuno wrote:
> > > This only returns true for windows 8 and above. The win8 bot doesn't run
> > > device_unittests so basically no bot would be able to run these tests.
> > 
> > Done.
> 
> It would have helped when replying 'Done' here to explain what was done to fix
> it - since the code at this comment location didn't change and a reviewer
won't
> see in the patch diff what happened here.
> 
> Remove the skipping logic "if (!PlatformSupportsLowEnergy())" from the tests.
> These InitWithFakeAdapter() tests should always run, even on OS versions that
> are earlier than when the APIs are supported. The fakes should always work. I
> made a mistake in earlier review not catching this -- please remove the
skipping
> logic from all tests using fake adapter. 

Ping.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:158:
DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
I see next patch adds logic here, just FYI if a method doesn't do anything just
wait to add it in the patch that needs it. Same for members such as
ui_task_runner_.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_win.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:15: void
BluetoothRemoteGattServiceWin::NotifyServiceDiscComplIfNecessary() {
Please keep member definitions in the same order as their declarations.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:35: for (const auto& cha :
included_characteristic_) {
Not 'cha'. Perhaps 'entry', 'kv' for 'key / value', or even just 'c', but not a
non-common partial truncation 'cha'.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:79: // First, remove no
longer existed characteristics.
remove characteristics that no longer exist.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:81: for (const auto& cha :
included_characteristic_) {
'cha' again

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:85: for (auto id :
to_be_removed) {
const auto&

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:125: for (const auto& ser
: included_service_) {
not 'ser'

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:168: // First, remove no
longer exist included service.
remove services that no longer exist.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:170: for (const auto& ser
: included_service_) {
not 'ser'

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:269: // Only notify
adapter for primary service.
explain in comment why.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:275: std::string
identifier =
The characteristic code that computes the ID once and caches it makes me think
that in a follow up patch you should do the same for service -- as clients such
as Web Bluetooth may iterate over services and cause this string work to be done
many times.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_win.h (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:57: void
GattServiceDiscoverComplete(BluetoothRemoteGattServiceWin* service);
Discovery

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:61: void
GattCharacteristicDiscoverComplete(
Discovery

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:72: void
GetIncludedCharacteristicsCallback(
OnGetIncludedCharacteristics. The previous name can also read as an accessor
method that gets the callback.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:76: void
GetIncludedServicesCallback(scoped_ptr<BTH_LE_GATT_SERVICE> services,
OnGetIncludedServices

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:85: void
NotifyServiceDiscComplIfNecessary();
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules "do not
abbreviate by deleting letters within a word".

NotifyGattDiscoveryCompleteForServiceNecessary is long but OK.
NotifyDiscoveryCompleteIfNecessary if you feel a need for shorter one?

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:91: // Checks if
|characteristic| is still exist in this service according to
"| still exists"

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:93: bool
DoesCharacteristicExist(
static

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:132: GattCharacteristicsMap
included_characteristic_;
plural as it is a container, included_characteristics_

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:143: GattServicesMap
included_service_;
included_services_

[gogerald1, 2016-02-25 22:35:43]

Patchset #6 (id:120001) has been deleted

[gogerald1, 2016-02-25 23:05:00]

Patchset #6 (id:140001) has been deleted

[gogerald1, 2016-02-25 23:16:10]

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

[commit-bot: I haz the power, 2016-02-25 23:19:02]

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

[gogerald1, 2016-02-25 23:19:18]

Split out of included service. Will break the next CL into three smaller CLs to
make it easier for review.

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

https://codereview.chromium.org/1690133002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
On 2016/02/24 01:39:09, scheib wrote:
> On 2016/02/19 22:45:02, gogerald1 wrote:
> > On 2016/02/19 17:33:05, ortuno wrote:
> > > This only returns true for windows 8 and above. The win8 bot doesn't run
> > > device_unittests so basically no bot would be able to run these tests.
> > 
> > Done.
> 
> It would have helped when replying 'Done' here to explain what was done to fix
> it - since the code at this comment location didn't change and a reviewer
won't
> see in the patch diff what happened here.
> 
> Remove the skipping logic "if (!PlatformSupportsLowEnergy())" from the tests.
> These InitWithFakeAdapter() tests should always run, even on OS versions that
> are earlier than when the APIs are supported. The fakes should always work. I
> made a mistake in earlier review not catching this -- please remove the
skipping
> logic from all tests using fake adapter. 

I've made PlatformSupportsLowEnergy always return true for test. Remove them
from Gatt service unit tests

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_service_unittest.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_service_unittest.cc:27: if
(!PlatformSupportsLowEnergy()) {
On 2016/02/25 06:22:43, scheib wrote:
> On 2016/02/24 01:39:09, scheib wrote:
> > On 2016/02/19 22:45:02, gogerald1 wrote:
> > > On 2016/02/19 17:33:05, ortuno wrote:
> > > > This only returns true for windows 8 and above. The win8 bot doesn't run
> > > > device_unittests so basically no bot would be able to run these tests.
> > > 
> > > Done.
> > 
> > It would have helped when replying 'Done' here to explain what was done to
fix
> > it - since the code at this comment location didn't change and a reviewer
> won't
> > see in the patch diff what happened here.
> > 
> > Remove the skipping logic "if (!PlatformSupportsLowEnergy())" from the
tests.
> > These InitWithFakeAdapter() tests should always run, even on OS versions
that
> > are earlier than when the APIs are supported. The fakes should always work.
I
> > made a mistake in earlier review not catching this -- please remove the
> skipping
> > logic from all tests using fake adapter. 
> 
> Ping.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:158:
DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
On 2016/02/25 06:22:43, scheib wrote:
> I see next patch adds logic here, just FYI if a method doesn't do anything
just
> wait to add it in the patch that needs it. Same for members such as
> ui_task_runner_.

This method is used by BluetoothRemoteGattServiceWin but without detail
implementation. Put a NOTIMPLEMENTED() here.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_win.cc (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:15: void
BluetoothRemoteGattServiceWin::NotifyServiceDiscComplIfNecessary() {
On 2016/02/25 06:22:43, scheib wrote:
> Please keep member definitions in the same order as their declarations.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:35: for (const auto& cha :
included_characteristic_) {
On 2016/02/25 06:22:43, scheib wrote:
> Not 'cha'. Perhaps 'entry', 'kv' for 'key / value', or even just 'c', but not
a
> non-common partial truncation 'cha'.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:79: // First, remove no
longer existed characteristics.
On 2016/02/25 06:22:43, scheib wrote:
> remove characteristics that no longer exist.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:81: for (const auto& cha :
included_characteristic_) {
On 2016/02/25 06:22:44, scheib wrote:
> 'cha' again

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:85: for (auto id :
to_be_removed) {
On 2016/02/25 06:22:44, scheib wrote:
> const auto&

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:125: for (const auto& ser
: included_service_) {
On 2016/02/25 06:22:44, scheib wrote:
> not 'ser'

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:168: // First, remove no
longer exist included service.
On 2016/02/25 06:22:44, scheib wrote:
> remove services that no longer exist.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:170: for (const auto& ser
: included_service_) {
On 2016/02/25 06:22:43, scheib wrote:
> not 'ser'

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:269: // Only notify
adapter for primary service.
On 2016/02/25 06:22:44, scheib wrote:
> explain in comment why.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:275: std::string
identifier =
On 2016/02/25 06:22:44, scheib wrote:
> The characteristic code that computes the ID once and caches it makes me think
> that in a follow up patch you should do the same for service -- as clients
such
> as Web Bluetooth may iterate over services and cause this string work to be
done
> many times.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_win.h (right):

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:57: void
GattServiceDiscoverComplete(BluetoothRemoteGattServiceWin* service);
On 2016/02/25 06:22:44, scheib wrote:
> Discovery

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:61: void
GattCharacteristicDiscoverComplete(
On 2016/02/25 06:22:44, scheib wrote:
> Discovery

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:72: void
GetIncludedCharacteristicsCallback(
On 2016/02/25 06:22:44, scheib wrote:
> OnGetIncludedCharacteristics. The previous name can also read as an accessor
> method that gets the callback.

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:76: void
GetIncludedServicesCallback(scoped_ptr<BTH_LE_GATT_SERVICE> services,
On 2016/02/25 06:22:44, scheib wrote:
> OnGetIncludedServices

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:85: void
NotifyServiceDiscComplIfNecessary();
On 2016/02/25 06:22:44, scheib wrote:
> https://google.github.io/styleguide/cppguide.html#General_Naming_Rules "do not
> abbreviate by deleting letters within a word".
> 
> NotifyGattDiscoveryCompleteForServiceNecessary is long but OK.
> NotifyDiscoveryCompleteIfNecessary if you feel a need for shorter one?

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:91: // Checks if
|characteristic| is still exist in this service according to
On 2016/02/25 06:22:44, scheib wrote:
> "| still exists"

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:91: // Checks if
|characteristic| is still exist in this service according to
On 2016/02/25 06:22:44, scheib wrote:
> "| still exists"

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:93: bool
DoesCharacteristicExist(
On 2016/02/25 06:22:44, scheib wrote:
> static

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:132: GattCharacteristicsMap
included_characteristic_;
On 2016/02/25 06:22:44, scheib wrote:
> plural as it is a container, included_characteristics_

Done.

https://codereview.chromium.org/1690133002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.h:143: GattServicesMap
included_service_;
On 2016/02/25 06:22:44, scheib wrote:
> included_services_

Done.

[commit-bot: I haz the power, 2016-02-26 01:18:20]

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

[commit-bot: I haz the power, 2016-02-26 01:18:20]

Dry run: This issue passed the CQ dry run.

[scheib, 2016-02-26 04:56:17]

Thanks for splitting up, and addressing feedback.
LGTM

https://codereview.chromium.org/1690133002/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_win.cc (right):

https://codereview.chromium.org/1690133002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_win.cc:87: //
TODO(crbug.com/590008): Needs implementation.
Thanks for issue. Please always have issues block some parent issue.

[gogerald1, 2016-02-26 15:21:33]

The CQ bit was checked by gogerald@chromium.org

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

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

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

Description was changed from

==========
Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

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

to

==========
Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

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

[commit-bot: I haz the power, 2016-02-26 15:36:16]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-02-26 15:37:28]

Description was changed from

==========
Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

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

to

==========
Implement BluetoothRemoteGattServiceWin and related unit tests

This CL implements BluetoothRemoteGattServiceWin and related unit tests.

BUG=579202

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

[commit-bot: I haz the power, 2016-02-26 15:37:29]

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