Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(43)

Issue 2105423003: bluetooth: Update the map of GATT services when services resolve (Closed)

Created:
3 years, 5 months ago by ortuno
Modified:
3 years, 5 months ago
Reviewers:
Miao, scheib, vudentz
CC:
chromium-reviews, Miao, ortuno+watch_chromium.org, scheib+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@my-origin
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Address Luiz's and scheib's comments #

Total comments: 2

Patch Set 3 : Address luiz's comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+342 lines, -261 lines) Patch
M device/bluetooth/bluez/bluetooth_adapter_bluez.cc View 1 chunk +1 line, -0 lines 0 comments Download
M device/bluetooth/bluez/bluetooth_device_bluez.h View 1 2 4 chunks +10 lines, -16 lines 0 comments Download
M device/bluetooth/bluez/bluetooth_device_bluez.cc View 1 2 5 chunks +33 lines, -47 lines 2 comments Download
M device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc View 1 5 chunks +243 lines, -116 lines 0 comments Download
M device/bluetooth/dbus/fake_bluetooth_device_client.h View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M device/bluetooth/dbus/fake_bluetooth_device_client.cc View 1 2 5 chunks +2 lines, -28 lines 0 comments Download
M device/bluetooth/dbus/fake_bluetooth_gatt_service_client.h View 3 chunks +20 lines, -9 lines 0 comments Download
M device/bluetooth/dbus/fake_bluetooth_gatt_service_client.cc View 6 chunks +33 lines, -40 lines 0 comments Download

Messages

Total messages: 27 (9 generated)
ortuno
scheib: PTAL
3 years, 5 months ago (2016-06-30 21:34:38 UTC) #2
scheib
LGTM with nit and thoughts on helping the test organization. https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.h File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.h#newcode143 ...
3 years, 5 months ago (2016-07-01 00:40:53 UTC) #3
vudentz
https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.cc File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.cc#newcode667 device/bluetooth/bluez/bluetooth_device_bluez.cc:667: if (!IsGattServicesDiscoveryComplete()) { InitializeGattServicesMap is only called by the ...
3 years, 5 months ago (2016-07-01 11:16:08 UTC) #5
vudentz
https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right): https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc#newcode684 device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:684: // The device should exist but contain no cached ...
3 years, 5 months ago (2016-07-01 11:25:15 UTC) #6
ortuno
Thanks for the comments Luiz! https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.cc File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.cc#newcode667 device/bluetooth/bluez/bluetooth_device_bluez.cc:667: if (!IsGattServicesDiscoveryComplete()) { On ...
3 years, 5 months ago (2016-07-01 15:51:48 UTC) #8
vudentz
On 2016/07/01 15:51:48, ortuno wrote: > Thanks for the comments Luiz! > > https://codereview.chromium.org/2105423003/diff/1/device/bluetooth/bluez/bluetooth_device_bluez.cc > ...
3 years, 5 months ago (2016-07-04 08:50:24 UTC) #9
Miao
3 years, 5 months ago (2016-07-04 10:00:29 UTC) #11
vudentz
https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/bluetooth_device_bluez.h File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/bluetooth_device_bluez.h#newcode147 device/bluetooth/bluez/bluetooth_device_bluez.h:147: void UpdateGattServices(const dbus::ObjectPath& object_path); Looks like I missed the ...
3 years, 5 months ago (2016-07-04 11:04:03 UTC) #12
ortuno
https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/bluetooth_device_bluez.h File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2105423003/diff/20001/device/bluetooth/bluez/bluetooth_device_bluez.h#newcode147 device/bluetooth/bluez/bluetooth_device_bluez.h:147: void UpdateGattServices(const dbus::ObjectPath& object_path); On 2016/07/04 at 11:04:03, vudentz ...
3 years, 5 months ago (2016-07-06 21:31:46 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2105423003/40001
3 years, 5 months ago (2016-07-06 22:33:07 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 5 months ago (2016-07-06 23:43:44 UTC) #17
vudentz
lgtm https://codereview.chromium.org/2105423003/diff/40001/device/bluetooth/bluez/bluetooth_device_bluez.cc File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2105423003/diff/40001/device/bluetooth/bluez/bluetooth_device_bluez.cc#newcode162 device/bluetooth/bluez/bluetooth_device_bluez.cc:162: if (IsGattServicesDiscoveryComplete()) { On 2016/07/06 21:31:46, ortuno wrote: ...
3 years, 5 months ago (2016-07-07 09:45:09 UTC) #18
ortuno
mcchou: PTAL
3 years, 5 months ago (2016-07-07 15:54:24 UTC) #19
ortuno
mcchou: I'll land this so that the fix can go in the next dev build ...
3 years, 5 months ago (2016-07-08 17:42:36 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2105423003/40001
3 years, 5 months ago (2016-07-08 17:43:11 UTC) #23
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 years, 5 months ago (2016-07-08 19:45:10 UTC) #24
commit-bot: I haz the power
CQ bit was unchecked.
3 years, 5 months ago (2016-07-08 19:45:35 UTC) #25
commit-bot: I haz the power
3 years, 5 months ago (2016-07-08 19:49:33 UTC) #27
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/214ea3e270584ca53cebe4e3412bf7585fbd4807
Cr-Commit-Position: refs/heads/master@{#404468}

Powered by Google App Engine
This is Rietveld 408576698