Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests

Committed: https://crrev.com/da9047813fef5c79e536420281860c62606e8201
Cr-Commit-Position: refs/heads/master@{#394345}

[Miao, 2016-05-16 15:51:56]

Description was changed from

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests
==========

to

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests
==========

[Miao, 2016-05-16 15:51:56]

mcchou@chromium.org changed reviewers:
+ rkc@chromium.org, scheib@chromium.org

[rkc, 2016-05-16 16:25:23]

lgtm

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:603: DCHECK(adapter());
Nit: Use either adapter_ or adapter() at all places?

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:644: VLOG(1) << "Initialize the
list of GATT services associated with device "
Normal operation, use VLOG(3).
Also,
nit: s/Initialize/Initializing

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:218: // This vector temporarily
cached the newly added services until
nit: s/cached/caches

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_remote_gatt_service_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_remote_gatt_service_bluez.h:9: 
Nit: Remove extra line.

[scheib, 2016-05-16 20:22:26]

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:632: if
(!gatt_services_.empty()) {
If InitializeGattServiceMap is only called from constructor:
This should be a DCHECK, as it is an assertion.

If InitializeGattServiceMap is to be called also from DevicePropertyChanged then
the code here must change to accommodate that: remove this block.

It seems there are 2 things here really, add all services at construction only.

Then, at construction or at DevicePropertiesChanged(services_resolved) call
NotifyGattDiscoveryComplete as needed.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:136: // Called conditionally by
the constructor and |DevicePropertyChanged()| to
Currently this is only called by constructor. See comments at the definition and
then adjust the comment to reflect what you decide to go with.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:271:
TEST_F(BluetoothGattBlueZTest, GattServiceAddedAndRemoved) {
Lots of test code here. Please point out code, or make new code, that has
services discovered before a BluetoothDeviceBlueZ is constructed -- testing the
BluetoothDeviceBlueZ::BluetoothDeviceBlueZ call to InitializeGattServiceMap
after services exist and services_resolved==true.

[Miao, 2016-05-17 00:22:47]

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:603: DCHECK(adapter());
On 2016/05/16 16:25:22, Rahul Chaturvedi wrote:
> Nit: Use either adapter_ or adapter() at all places?

Done.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:644: VLOG(1) << "Initialize the
list of GATT services associated with device "
On 2016/05/16 16:25:22, Rahul Chaturvedi wrote:
> Normal operation, use VLOG(3).
> Also,
> nit: s/Initialize/Initializing

Done.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:218: // This vector temporarily
cached the newly added services until
On 2016/05/16 16:25:23, Rahul Chaturvedi wrote:
> nit: s/cached/caches

Done.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_remote_gatt_service_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_remote_gatt_service_bluez.h:9: 
On 2016/05/16 16:25:23, Rahul Chaturvedi wrote:
> Nit: Remove extra line.

This extra line is for separating the C and C++ libraries. :)

[Miao, 2016-05-17 18:08:27]

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:632: if
(!gatt_services_.empty()) {
On 2016/05/16 20:22:26, scheib wrote:
> If InitializeGattServiceMap is only called from constructor:
> This should be a DCHECK, as it is an assertion.
> 
> If InitializeGattServiceMap is to be called also from DevicePropertyChanged
then
> the code here must change to accommodate that: remove this block.
> 
> It seems there are 2 things here really, add all services at construction
only.
> 
> Then, at construction or at DevicePropertiesChanged(services_resolved) call
> NotifyGattDiscoveryComplete as needed.

Done. Exactly. This function addresses the case where services have been
discovered and cached by BlueZ. All other incoming GATT services will go through
GattServiceAdded and will be handled later by DevicePropertiesChanged.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:136: // Called conditionally by
the constructor and |DevicePropertyChanged()| to
On 2016/05/16 20:22:26, scheib wrote:
> Currently this is only called by constructor. See comments at the definition
and
> then adjust the comment to reflect what you decide to go with.

Done.

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:271:
TEST_F(BluetoothGattBlueZTest, GattServiceAddedAndRemoved) {
On 2016/05/16 20:22:26, scheib wrote:
> Lots of test code here. Please point out code, or make new code, that has
> services discovered before a BluetoothDeviceBlueZ is constructed -- testing
the
> BluetoothDeviceBlueZ::BluetoothDeviceBlueZ call to InitializeGattServiceMap
> after services exist and services_resolved==true.

Removed the added lines in other tests and created two separated two tests for
two path to NotifyGattDiscoveryComplete.

[rkc, 2016-05-17 18:38:50]

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.h:136: // Called by the
constructor and |DevicePropertyChanged()| to  initialize the
Fix comment. We don't call this from DevicePropertyChanged.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:363:
TEST_F(BluetoothGattBlueZTest, DiscoveryCompleteForCashedGattService) {
s/Cashed/Cached

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/dbus/...
File device/bluetooth/dbus/fake_bluetooth_gatt_service_client.cc (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/dbus/...
device/bluetooth/dbus/fake_bluetooth_gatt_service_client.cc:117:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Why does this need to be delayed at all? Can PostTask suffice?

In tests we try not to use actual timing for anything since they tend to
introduce flakiness. The idea is that a 100ms delay should give the same result
as a 10s delay as the same as a 0ms delay.

[scheib, 2016-05-17 19:24:48]

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.h:136: // Called by the
constructor and |DevicePropertyChanged()| to  initialize the
Still, this is only called by the constructor, not DevicePropertyChanged.

Single space after 'to'.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:396:
TestBluetoothAdapterObserver observer(adapter_);
Move the observer up to before CreateDevice and verify here:
EXPECT_EQ(0, device->GetGattServices().size());
EXPECT_EQ(0, observer.gatt_discovery_complete_count());

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:494: EXPECT_EQ(0,
observer.gatt_service_changed_count());
Does this line need to be removed? Did behavior change?

[Miao, 2016-05-18 03:26:58]

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_device_bluez.h:136: // Called by the
constructor and |DevicePropertyChanged()| to  initialize the
On 2016/05/17 19:24:47, scheib wrote:
> Still, this is only called by the constructor, not DevicePropertyChanged.
> 
> Single space after 'to'.

Done.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:363:
TEST_F(BluetoothGattBlueZTest, DiscoveryCompleteForCashedGattService) {
On 2016/05/17 18:38:50, Rahul Chaturvedi wrote:
> s/Cashed/Cached

Done.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:396:
TestBluetoothAdapterObserver observer(adapter_);
On 2016/05/17 19:24:47, scheib wrote:
> Move the observer up to before CreateDevice and verify here:
> EXPECT_EQ(0, device->GetGattServices().size());
> EXPECT_EQ(0, observer.gatt_discovery_complete_count());

Done.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:494: EXPECT_EQ(0,
observer.gatt_service_changed_count());
On 2016/05/17 19:24:47, scheib wrote:
> Does this line need to be removed? Did behavior change?

The Behavior doesn't not change. It is unclear to me that why
observer.gatt_discovery_complete_count() is checked here but it is not checked
below.

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/dbus/...
File device/bluetooth/dbus/fake_bluetooth_gatt_service_client.cc (right):

https://codereview.chromium.org/1979633004/diff/120001/device/bluetooth/dbus/...
device/bluetooth/dbus/fake_bluetooth_gatt_service_client.cc:117:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2016/05/17 18:38:50, Rahul Chaturvedi wrote:
> Why does this need to be delayed at all? Can PostTask suffice?
> 
> In tests we try not to use actual timing for anything since they tend to
> introduce flakiness. The idea is that a 100ms delay should give the same
result
> as a 10s delay as the same as a 0ms delay.

I think PostTask is sufficient. The reason is the same as the above code, I was
trying to emulate the delay here. Considering the flakines, the PostDelayedTask
of exposing characteristics should be changed to PostTask as well.

[rkc, 2016-05-18 03:32:32]

lgtm

[scheib, 2016-05-18 03:34:50]

https://codereview.chromium.org/1979633004/diff/140001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/140001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:497: EXPECT_EQ(0,
observer.gatt_characteristic_added_count());
Is the change to gatt_characteristic_added_count intentional?

[Miao, 2016-05-18 03:43:48]

https://codereview.chromium.org/1979633004/diff/140001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/1979633004/diff/140001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:497: EXPECT_EQ(0,
observer.gatt_characteristic_added_count());
On 2016/05/18 03:34:50, scheib wrote:
> Is the change to gatt_characteristic_added_count intentional?

It is not intentional, I think it was a mistake that I made during the rebase.
Removed.

[scheib, 2016-05-18 04:00:06]

LGTM

[Miao, 2016-05-18 04:01:50]

The CQ bit was checked by mcchou@chromium.org

[Miao, 2016-05-18 04:01:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/1979633004/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-05-18 04:02:23]

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

[commit-bot: I haz the power, 2016-05-18 07:00:46]

Description was changed from

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests
==========

to

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests
==========

[commit-bot: I haz the power, 2016-05-18 07:00:47]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-18 07:01:55]

Description was changed from

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests
==========

to

==========
Invoke GattDiscoveryCompleteForService by observing ServicesResolved property

This changes how GattDiscoveryCompleteForService is invoked by observing the
toggle of ServicesResolved property, which is added in BlueZ5.39, of interface
org.bluez.Device1 instead of depending GattServiceAdded. Although BlueZ toggles
this property not only on the first fully-discovered event but also on the
following addition/removal events of service. GattDiscoveryCompleteForService
should be called only once after a remote device is created.

BUG=606011
TEST=build and run tests

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

[commit-bot: I haz the power, 2016-05-18 07:01:56]

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