bluetooth: Update the map of GATT services when services resolve

bluetooth: Update the map of GATT services when services resolve

Before we updated the map two times:

1. When constructing the device object *iff* the services had been resolved.
2. When a new service was added.

This meant that if there was a cached service before services resolved,
the service would never be added to the map.

There are a couple of changes:

1. No longer add BluetoothDeviceBluez as an observer of BluetoothDeviceClient.
   This follows the previous pattern of exposing a function that
   BluetoothAdapterClient will use to update the device.
2. When services are resolved we add all previously unknown services.
3. Modified 2 tests and added two extra ones to test all the following cases:

   1. Cached services exist and discovery hasn't completed.
   2. Cached services exist and discovery has completed.
   3. New service is exposed with no previous cached services.
   4. New service is exposed with previous cached service.

   Case 1 should cover this bug.

BUG=624400
Committed: https://crrev.com/214ea3e270584ca53cebe4e3412bf7585fbd4807
Cr-Commit-Position: refs/heads/master@{#404468}

[ortuno, 2016-06-30 21:34:37]

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

[ortuno, 2016-06-30 21:34:38]

scheib: PTAL

[scheib, 2016-07-01 00:40:53]

LGTM with nit and thoughts on helping the test organization.

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.h:143: /// Called once all
services have been discovered. Invokes
s|///|//|

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:657:
TEST_F(BluetoothGattBlueZTest, CachedServices) {
I like this test coverage, but after this change it will be hard to see that the
4 tests are related. Especially because ServicesDiscovered is seperated from
these, but also because names are not similar. Let's fix by some combination of:
+ Move these tests to just after ServicesDiscovered
+ Comment at top of tests the highly related tests and how they differ?
+ Name these more consistently? Discover{New|Cached}Services?

[vudentz, 2016-07-01 11:16:08]

luiz.von.dentz@intel.com changed reviewers:
+ luiz.von.dentz@intel.com

[vudentz, 2016-07-01 11:16:08]

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:667: if
(!IsGattServicesDiscoveryComplete()) {
InitializeGattServicesMap is only called by the constructor so this probably
always evaluate to false.

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:687: for (const auto& iter :
gatt_services_) {
What is the purpose of iterating to gatt_services twice? Can't this be done with
the first iteration?

[vudentz, 2016-07-01 11:25:15]

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:684: // The device
should exist but contain no cached services while the services
Should this be the other way around, we are suppose to test a device with cached
attributes.

[ortuno, 2016-07-01 15:47:40]

Description was changed from

==========
bluetooth: Update the map of GATT services when services resolve

Before we updated the map two times:

1. When constructing the device object *iff* the services had been resolved.
2. When a new service was added.

This meant that if there was a cached service before services resolved,
the service would never be added to the map.

There are a couple of changes:

1. No longer add BluetoothDeviceBluez as an observer of BluetoothDeviceClient.
   This follows the previous pattern of exposing a function that
   BluetoothAdapterClient will use to update the device.
2. When services are resolved we add all previously unknown services.
3. Modified 2 tests and added an extra one to test all the following cases:

   1. Cached services exist and discovery hasn't completed.
   2. Cached services exist and discovery has completed.
   3. New service is exposed with no previous cached services.
   4. New service is exposed with previous cached service.

   Cases 1 & 2 should cover this bug.

BUG=624400
==========

to

==========
bluetooth: Update the map of GATT services when services resolve

Before we updated the map two times:

1. When constructing the device object *iff* the services had been resolved.
2. When a new service was added.

This meant that if there was a cached service before services resolved,
the service would never be added to the map.

There are a couple of changes:

1. No longer add BluetoothDeviceBluez as an observer of BluetoothDeviceClient.
   This follows the previous pattern of exposing a function that
   BluetoothAdapterClient will use to update the device.
2. When services are resolved we add all previously unknown services.
3. Modified 2 tests and added two extra ones to test all the following cases:

   1. Cached services exist and discovery hasn't completed.
   2. Cached services exist and discovery has completed.
   3. New service is exposed with no previous cached services.
   4. New service is exposed with previous cached service.

   Case 1 should cover this bug.

BUG=624400
==========

[ortuno, 2016-07-01 15:51:48]

Thanks for the comments Luiz!

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:667: if
(!IsGattServicesDiscoveryComplete()) {
On 2016/07/01 at 11:16:08, vudentz wrote:
> InitializeGattServicesMap is only called by the constructor so this probably
always evaluate to false.

What if a device is connected and all it's attributes have been discovered
already? Wouldn't this be true in that case?

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:687: for (const auto& iter :
gatt_services_) {
On 2016/07/01 at 11:16:08, vudentz wrote:
> What is the purpose of iterating to gatt_services twice? Can't this be done
with the first iteration?

I combined the two for loops.

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.h:143: /// Called once all
services have been discovered. Invokes
On 2016/07/01 at 00:40:53, scheib wrote:
> s|///|//|

Done.

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:657:
TEST_F(BluetoothGattBlueZTest, CachedServices) {
On 2016/07/01 at 00:40:53, scheib wrote:
> I like this test coverage, but after this change it will be hard to see that
the 4 tests are related. Especially because ServicesDiscovered is seperated from
these, but also because names are not similar. Let's fix by some combination of:
> + Move these tests to just after ServicesDiscovered
> + Comment at top of tests the highly related tests and how they differ?
> + Name these more consistently? Discover{New|Cached}Services?

I moved them after services discovered and changed the names. I think that makes
it obvious enough that these tests are related.

https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:684: // The device
should exist but contain no cached services while the services
On 2016/07/01 at 11:25:15, vudentz wrote:
> Should this be the other way around, we are suppose to test a device with
cached attributes.

We have no guarantee that the cached services are valid so we can't create
objects for them before all services are resolved. This test makes sure that
while services are being resolved we don't create objects for cached services,
but once services are resolved we create objects for them.

[vudentz, 2016-07-04 08:50:24]

On 2016/07/01 15:51:48, ortuno wrote:
> Thanks for the comments Luiz!
> 
>
https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
> File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):
> 
>
https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
> device/bluetooth/bluez/bluetooth_device_bluez.cc:667: if
> (!IsGattServicesDiscoveryComplete()) {
> On 2016/07/01 at 11:16:08, vudentz wrote:
> > InitializeGattServicesMap is only called by the constructor so this probably
> always evaluate to false.
> 
> What if a device is connected and all it's attributes have been discovered
> already? Wouldn't this be true in that case?

Yes, but that is not 'cached' since we had rediscovered them during the
connection.

>
https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/blue...
> device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:684: // The device
> should exist but contain no cached services while the services
> On 2016/07/01 at 11:25:15, vudentz wrote:
> > Should this be the other way around, we are suppose to test a device with
> cached attributes.
> 
> We have no guarantee that the cached services are valid so we can't create
> objects for them before all services are resolved. This test makes sure that
> while services are being resolved we don't create objects for cached services,
> but once services are resolved we create objects for them.

You are talking about a test that is supposed to test enumeration of cached
service but you don't want to create the objects for cached service? Anyway I
can see where this is different, since you are starting of with a connected
device, but in the other hand I don't know where else the GetServices() would be
called to enumerate the cached services since the other part of the code that
calls GattServiceAdded seems to be only ObjectAdded. Or ObjectAdded is already
enumerating existing objects? If that is the case what is purpose of
InitializeGattServicesMap? Btw, if you are expecting BlueZ to issue
InterfacesAdded for cached attributes that will not going to happen since that
is only done for new objects, for existing objects this has to be done with use
of GetManagedObjets.

[Miao, 2016-07-04 10:00:28]

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

[vudentz, 2016-07-04 11:04:03]

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

https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:147: void
UpdateGattServices(const dbus::ObjectPath& object_path);
Looks like I missed the introduction of this method, well if that is doing most
part of what InitializeGattServicesMap g perhaps we should call it directly
instead of having duplicated code.

[ortuno, 2016-07-06 21:31:46]

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

https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.h:147: void
UpdateGattServices(const dbus::ObjectPath& object_path);
On 2016/07/04 at 11:04:03, vudentz wrote:
> Looks like I missed the introduction of this method, well if that is doing
most part of what InitializeGattServicesMap g perhaps we should call it directly
instead of having duplicated code.

Good catch. Done.

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

https://codereview.chromium.org/2105423003/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:162: if
(IsGattServicesDiscoveryComplete()) {
luiz: You mentioned that this will always be false during construction. The case
I was thinking about was the following:

1. Chrome is closed.
2. Use bluetoothctl to connect to a device and wait for all its services to be
discovered.
3. Open chrome.

At this point the device is connected all its services are discovered so
IsGattServicesDiscoveryComplete should be true, right?

[ortuno, 2016-07-06 22:31:36]

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

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

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

[commit-bot: I haz the power, 2016-07-06 23:43:43]

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

[commit-bot: I haz the power, 2016-07-06 23:43:44]

Dry run: This issue passed the CQ dry run.

[vudentz, 2016-07-07 09:45:09]

lgtm

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

https://codereview.chromium.org/2105423003/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_device_bluez.cc:162: if
(IsGattServicesDiscoveryComplete()) {
On 2016/07/06 21:31:46, ortuno wrote:
> luiz: You mentioned that this will always be false during construction. The
case
> I was thinking about was the following:
> 
> 1. Chrome is closed.
> 2. Use bluetoothctl to connect to a device and wait for all its services to be
> discovered.
> 3. Open chrome.
> 
> At this point the device is connected all its services are discovered so
> IsGattServicesDiscoveryComplete should be true, right?

Yep, I was assuming this object to be responsible of tracking
IsGattServicesDiscoveryComplete but it looks like it is reading from D-Bus
directly.

[ortuno, 2016-07-07 15:54:24]

mcchou: PTAL

[ortuno, 2016-07-08 17:42:35]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-07-08 17:42:36]

mcchou: I'll land this so that the fix can go in the next dev build and we can
ask for a merge if everything looks good there. I'll address any comments you
have in a follow up CL.

[ortuno, 2016-07-08 17:42:37]

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/2105423003/#ps40001
(title: "Address luiz's comments")

[commit-bot: I haz the power, 2016-07-08 17:43:11]

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-08 19:45:35]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 19:49:32]

Description was changed from

==========
bluetooth: Update the map of GATT services when services resolve

Before we updated the map two times:

1. When constructing the device object *iff* the services had been resolved.
2. When a new service was added.

This meant that if there was a cached service before services resolved,
the service would never be added to the map.

There are a couple of changes:

1. No longer add BluetoothDeviceBluez as an observer of BluetoothDeviceClient.
   This follows the previous pattern of exposing a function that
   BluetoothAdapterClient will use to update the device.
2. When services are resolved we add all previously unknown services.
3. Modified 2 tests and added two extra ones to test all the following cases:

   1. Cached services exist and discovery hasn't completed.
   2. Cached services exist and discovery has completed.
   3. New service is exposed with no previous cached services.
   4. New service is exposed with previous cached service.

   Case 1 should cover this bug.

BUG=624400
==========

to

==========
bluetooth: Update the map of GATT services when services resolve

Before we updated the map two times:

1. When constructing the device object *iff* the services had been resolved.
2. When a new service was added.

This meant that if there was a cached service before services resolved,
the service would never be added to the map.

There are a couple of changes:

1. No longer add BluetoothDeviceBluez as an observer of BluetoothDeviceClient.
   This follows the previous pattern of exposing a function that
   BluetoothAdapterClient will use to update the device.
2. When services are resolved we add all previously unknown services.
3. Modified 2 tests and added two extra ones to test all the following cases:

   1. Cached services exist and discovery hasn't completed.
   2. Cached services exist and discovery has completed.
   3. New service is exposed with no previous cached services.
   4. New service is exposed with previous cached service.

   Case 1 should cover this bug.

BUG=624400

Committed: https://crrev.com/214ea3e270584ca53cebe4e3412bf7585fbd4807
Cr-Commit-Position: refs/heads/master@{#404468}
==========

[commit-bot: I haz the power, 2016-07-08 19:49:33]

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