Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862093002:
bluetooth: replace GattServices D-Bus property with ServicesResolved (Closed)
Descriptionbluetooth: replace GattServices D-Bus property with ServicesResolved
GattServices has been removed from BlueZ so ServicesResolved shall be used instead.
BUG=599472
Depends on: https://chromium-review.googlesource.com/#/c/337482/
Committed: https://crrev.com/f52a0e19fdb56e0ea7a511e0560afa03ed098150
Cr-Commit-Position: refs/heads/master@{#392339}
Patch Set 1 #
Total comments: 3
Patch Set 2 : bluetooth: replace GattServices D-Bus property with ServicesResolved #
Total comments: 2
Patch Set 3 : rebased #
Total comments: 2
Patch Set 4 : Fix calling SetGattServicesDiscoveryComplete #Patch Set 5 : Fix unit tests #
Messages
Total messages: 40 (14 generated)
Description was changed from ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved so shall be used instead. BUG=599472 ========== to ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved so shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ==========
Description was changed from ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved so shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ========== to ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ==========
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862093002/1
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_adapter_bluez.cc:531: device_bluez->SetGattServicesDiscoveryComplete(true); I don't think we need to keep track of the value ourselves anymore. I would recommend making IsGattServicesDiscoveryComplete in bluetooth_device.h virtual and implement IsGattServicesDiscoveryComplete in bluetooth_device_bluez.h/cc. The implementation would just access the property directly. Also override SetGattServicesDiscoveryComplete in bluetooth_device_bluez and add: // Bluez keeps track of the discovery state. NOTIMPLEMENTED()
https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_adapter_bluez.cc:531: device_bluez->SetGattServicesDiscoveryComplete(true); On 2016/04/07 15:34:59, ortuno wrote: > I don't think we need to keep track of the value ourselves anymore. I would > recommend making IsGattServicesDiscoveryComplete in bluetooth_device.h virtual > and implement IsGattServicesDiscoveryComplete in bluetooth_device_bluez.h/cc. > The implementation would just access the property directly. > > Also override SetGattServicesDiscoveryComplete in bluetooth_device_bluez and > add: > > // Bluez keeps track of the discovery state. > NOTIMPLEMENTED() I was wondering myself why there was two very similar method being called one after the other, anyway what you are suggesting doesn't sound backward compatible since GattServices doesn't track service discovery properly.
https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_adapter_bluez.cc:531: device_bluez->SetGattServicesDiscoveryComplete(true); On 2016/04/07 at 17:59:33, vudentz wrote: > On 2016/04/07 15:34:59, ortuno wrote: > > I don't think we need to keep track of the value ourselves anymore. I would > > recommend making IsGattServicesDiscoveryComplete in bluetooth_device.h virtual > > and implement IsGattServicesDiscoveryComplete in bluetooth_device_bluez.h/cc. > > The implementation would just access the property directly. > > > > Also override SetGattServicesDiscoveryComplete in bluetooth_device_bluez and > > add: > > > > // Bluez keeps track of the discovery state. > > NOTIMPLEMENTED() > > I was wondering myself why there was two very similar method being called one after the other, anyway what you are suggesting doesn't sound backward compatible since GattServices doesn't track service discovery properly. I think we should wait until we roll bluez again and then just land without the backwards compatible code. Once we launch Web Bluetooth on Linux we will need to keep old version in mind but until then I think it's best to keep the codebase lean.
On 2016/04/08 19:24:17, ortuno wrote: > https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... > File device/bluetooth/bluetooth_adapter_bluez.cc (right): > > https://codereview.chromium.org/1862093002/diff/1/device/bluetooth/bluetooth_... > device/bluetooth/bluetooth_adapter_bluez.cc:531: > device_bluez->SetGattServicesDiscoveryComplete(true); > On 2016/04/07 at 17:59:33, vudentz wrote: > > On 2016/04/07 15:34:59, ortuno wrote: > > > I don't think we need to keep track of the value ourselves anymore. I would > > > recommend making IsGattServicesDiscoveryComplete in bluetooth_device.h > virtual > > > and implement IsGattServicesDiscoveryComplete in > bluetooth_device_bluez.h/cc. > > > The implementation would just access the property directly. > > > > > > Also override SetGattServicesDiscoveryComplete in bluetooth_device_bluez and > > > add: > > > > > > // Bluez keeps track of the discovery state. > > > NOTIMPLEMENTED() > > > > I was wondering myself why there was two very similar method being called one > after the other, anyway what you are suggesting doesn't sound backward > compatible since GattServices doesn't track service discovery properly. > > I think we should wait until we roll bluez again and then just land without the > backwards compatible code. Once we launch Web Bluetooth on Linux we will need to > keep old version in mind but until then I think it's best to keep the codebase > lean. That means chromium does not considered Web Bluetooth on Linux as supported already? Well if that is the case then Im fine to drop GattServices property entirely. Btw, we could perhaps plan in advance to drop the experimental flag in BlueZ 5.40 which should then enable Web Bluetooth by default in Linux.
https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:530: device_bluez->SetGattServicesDiscoveryComplete(true); We no longer need to call this. https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:446: virtual void SetGattServicesDiscoveryComplete(bool complete); I had to fix some tests that required this to be virtual so you no longer need to do this.
On 2016/04/20 20:19:14, ortuno wrote: > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_adapter_bluez.cc (right): > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_adapter_bluez.cc:530: > device_bluez->SetGattServicesDiscoveryComplete(true); > We no longer need to call this. > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_device.h (right): > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_device.h:446: virtual void > SetGattServicesDiscoveryComplete(bool complete); > I had to fix some tests that required this to be virtual so you no longer need > to do this. vudentz with https://codereview.chromium.org/1908523002 landed, you can simply revert your changes in device/bluetooth/bluetooth_device.h and proceed to a $ git rebase-update
On 2016/04/21 07:22:06, François Beaufort wrote: > On 2016/04/20 20:19:14, ortuno wrote: > > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > > File device/bluetooth/bluetooth_adapter_bluez.cc (right): > > > > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > > device/bluetooth/bluetooth_adapter_bluez.cc:530: > > device_bluez->SetGattServicesDiscoveryComplete(true); > > We no longer need to call this. > > > > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > > File device/bluetooth/bluetooth_device.h (right): > > > > > https://codereview.chromium.org/1862093002/diff/20001/device/bluetooth/blueto... > > device/bluetooth/bluetooth_device.h:446: virtual void > > SetGattServicesDiscoveryComplete(bool complete); > > I had to fix some tests that required this to be virtual so you no longer need > > to do this. > > vudentz with https://codereview.chromium.org/1908523002 landed, you can simply > revert your changes in device/bluetooth/bluetooth_device.h and proceed to a $ > git rebase-update @ortuno Can you have a look?
https://codereview.chromium.org/1862093002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:529: device_bluez->SetGattServicesDiscoveryComplete(true); You no longer need to set this value.
https://codereview.chromium.org/1862093002/diff/40001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1862093002/diff/40001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_adapter_bluez.cc:529: device_bluez->SetGattServicesDiscoveryComplete(true); On 2016/04/26 14:12:14, ortuno wrote: > You no longer need to set this value. Done.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862093002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862093002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
You also need to fix the test in: https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... To run that specific test I use: ninja -C out/Default -j 1024 device_unittests && out/Default/device_unittests --gtest_filter="BluetoothGattBlueZTest.ServicesDiscovered" You can change gtest_filter to "Bluetooth*: to run all bluetooth tests.
The CQ bit was checked by fbeaufort@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862093002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862093002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/02 13:40:11, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I believe we can commit this patch since BlueZ 5.39 is now default in Chrome OS Canary.
On 2016/05/09 at 06:34:31, beaufort.francois wrote: > On 2016/05/02 13:40:11, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > I believe we can commit this patch since BlueZ 5.39 is now default in Chrome OS Canary. I think we rolled back BlueZ 5.39 because we haven't fixed this issue yet: https://bugs.chromium.org/p/chromium/issues/detail?id=606011#c8
On 2016/05/09 14:42:55, ortuno wrote: > On 2016/05/09 at 06:34:31, beaufort.francois wrote: > > On 2016/05/02 13:40:11, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > I believe we can commit this patch since BlueZ 5.39 is now default in Chrome > OS Canary. > > I think we rolled back BlueZ 5.39 because we haven't fixed this issue yet: > https://bugs.chromium.org/p/chromium/issues/detail?id=606011#c8 Perhaps that is because of: https://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=6aa1c4488ee96fd6a2... At least Im able to use the demos on Linux with BlueZ latest and these changes applied.
On 2016/05/09 at 15:16:04, luiz.von.dentz wrote: > On 2016/05/09 14:42:55, ortuno wrote: > > On 2016/05/09 at 06:34:31, beaufort.francois wrote: > > > On 2016/05/02 13:40:11, commit-bot: I haz the power wrote: > > > > Dry run: This issue passed the CQ dry run. > > > > > > I believe we can commit this patch since BlueZ 5.39 is now default in Chrome > > OS Canary. > > > > I think we rolled back BlueZ 5.39 because we haven't fixed this issue yet: > > https://bugs.chromium.org/p/chromium/issues/detail?id=606011#c8 > > Perhaps that is because of: > > https://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=6aa1c4488ee96fd6a2... > > At least Im able to use the demos on Linux with BlueZ latest and these changes applied. Web Bluetooth should work but other clients of the bluetooth code relied on the 'Characteristics' property which was removed.
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862093002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862093002/80001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm! Thanks for doing this!
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862093002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862093002/80001
Message was sent while issue was closed.
Description was changed from ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ========== to ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ ========== to ========== bluetooth: replace GattServices D-Bus property with ServicesResolved GattServices has been removed from BlueZ so ServicesResolved shall be used instead. BUG=599472 Depends on: https://chromium-review.googlesource.com/#/c/337482/ Committed: https://crrev.com/f52a0e19fdb56e0ea7a511e0560afa03ed098150 Cr-Commit-Position: refs/heads/master@{#392339} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f52a0e19fdb56e0ea7a511e0560afa03ed098150 Cr-Commit-Position: refs/heads/master@{#392339} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
