Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(427)

Issue 1950033002: bluetooth: mac: Initial BluetoothRemoteGattCharacteristicMac implementation (Closed)

Created:
4 years, 7 months ago by jlebel
Modified:
4 years, 6 months ago
CC:
blink-reviews, chromium-reviews, haraken, ortuno+watch_chromium.org, scheib+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@servicescan_cleanup
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

bluetooth: mac: Initial BluetoothRemoteGattCharacteristicMac implementation Adding initial remote characteristic implementation on OS X, with only the basic methods (e.g. GetUUID) implemented. Characteristic discovery also implemented in BluetoothRemoteGattServiceMac. BUG=609064 Committed: https://crrev.com/6af3e124c3e3745f280d3b0c5c9491a5e603340f Committed: https://crrev.com/027333819f3fb880a7e433157dfc4c55d3bf8268 Cr-Original-Commit-Position: refs/heads/master@{#400127} Cr-Commit-Position: refs/heads/master@{#400266}

Patch Set 1 #

Patch Set 2 : Working #

Patch Set 3 : Adding more tests #

Patch Set 4 : Don't run tests if BLE is not supported #

Patch Set 5 : Adding comments, and not implemented #

Patch Set 6 : Last fix for unit tests #

Patch Set 7 : Updating the characteristic property conversion #

Total comments: 16

Patch Set 8 : Addressing comments #

Total comments: 23

Patch Set 9 : ortuno fixes #

Patch Set 10 : msarda fixes #

Patch Set 11 : Merge from top of tree #

Total comments: 13

Patch Set 12 : Fixes for ortuno #

Patch Set 13 : Adding check for unit test vs 10.10 #

Patch Set 14 : Adding test for 10.10 for unit tests #

Patch Set 15 : Fixing the build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+732 lines, -75 lines) Patch
M device/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/BUILD.gn View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_low_energy_device_mac.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_low_energy_device_mac.mm View 1 2 3 4 5 6 7 8 9 2 chunks +39 lines, -5 lines 0 comments Download
M device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm View 1 2 chunks +10 lines, -0 lines 0 comments Download
A device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h View 1 2 3 4 5 6 7 8 9 1 chunk +72 lines, -0 lines 0 comments Download
A device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm View 1 2 3 4 5 6 7 8 1 chunk +151 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc View 1 2 3 4 chunks +24 lines, -8 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_service_mac.h View 1 2 3 4 5 6 7 8 9 2 chunks +26 lines, -1 line 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_service_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +89 lines, -5 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +25 lines, -10 lines 0 comments Download
M device/bluetooth/test/bluetooth_test_mac.h View 1 2 3 4 5 6 7 8 9 3 chunks +16 lines, -0 lines 0 comments Download
M device/bluetooth/test/bluetooth_test_mac.mm View 1 2 3 4 5 6 7 8 9 5 chunks +63 lines, -2 lines 0 comments Download
A + device/bluetooth/test/mock_bluetooth_cbcharacteristic_mac.h View 1 2 1 chunk +23 lines, -0 lines 0 comments Download
A device/bluetooth/test/mock_bluetooth_cbcharacteristic_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +120 lines, -0 lines 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm View 1 2 3 4 5 6 7 4 chunks +22 lines, -3 lines 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbservice_mac.h View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -1 line 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbservice_mac.mm View 1 2 3 4 5 6 7 8 3 chunks +28 lines, -1 line 0 comments Download
M device/device_tests.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp View 2 chunks +0 lines, -14 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 81 (36 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950033002/40001
4 years, 6 months ago (2016-06-01 03:29:13 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/119983)
4 years, 6 months ago (2016-06-01 04:16:37 UTC) #4
jlebel
Hello Giovanni, Can you review this big patch to add characteristic scanning on OS X? ...
4 years, 6 months ago (2016-06-07 15:56:09 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/100001
4 years, 6 months ago (2016-06-07 16:02:04 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/159491) win_chromium_rel_ng on ...
4 years, 6 months ago (2016-06-07 16:06:25 UTC) #11
jlebel
Hello Vincent, Can you review this big patch to add characteristic scanning on OS X? ...
4 years, 6 months ago (2016-06-07 18:57:19 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/120001
4 years, 6 months ago (2016-06-08 22:17:02 UTC) #15
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/241415)
4 years, 6 months ago (2016-06-08 23:51:43 UTC) #17
jlebel
Hello Mihai, Would you have time to review the Objective-C of this patch? Thanks,
4 years, 6 months ago (2016-06-10 12:17:00 UTC) #19
scheib
https://codereview.chromium.org/1950033002/diff/120001/device/bluetooth/bluetooth_low_energy_device_mac.mm File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1950033002/diff/120001/device/bluetooth/bluetooth_low_energy_device_mac.mm#newcode287 device/bluetooth/bluetooth_low_energy_device_mac.mm:287: // Does all services have been discovered? blank line, ...
4 years, 6 months ago (2016-06-10 21:59:31 UTC) #20
scheib
Please add more detail to the change description as well.
4 years, 6 months ago (2016-06-10 22:00:45 UTC) #21
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/140001
4 years, 6 months ago (2016-06-10 23:00:30 UTC) #24
jlebel
Hello Vincent, Thanks for your input. I'm not sure what I'm supposed put the description. ...
4 years, 6 months ago (2016-06-10 23:02:46 UTC) #25
scheib
LGTM > Thanks for your input. I'm not sure what I'm supposed put the description. ...
4 years, 6 months ago (2016-06-10 23:51:34 UTC) #28
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/86224)
4 years, 6 months ago (2016-06-11 00:34:40 UTC) #30
ortuno
There are a couple of tests that also need to be enabled in bluetooth_remote_gatt_service_unittest.cc: - ...
4 years, 6 months ago (2016-06-13 20:51:20 UTC) #31
msarda
Nits. https://codereview.chromium.org/1950033002/diff/140001/device/bluetooth/bluetooth_low_energy_device_mac.mm File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1950033002/diff/140001/device/bluetooth/bluetooth_low_energy_device_mac.mm#newcode275 device/bluetooth/bluetooth_low_energy_device_mac.mm:275: // TODO(http://crbug.com/609844): Decide what to do if we ...
4 years, 6 months ago (2016-06-14 09:15:06 UTC) #32
jlebel
Hello Mihai and Giovanni, I've addressed your comments. Jérôme, https://codereview.chromium.org/1950033002/diff/140001/device/bluetooth/bluetooth_low_energy_device_mac.mm File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1950033002/diff/140001/device/bluetooth/bluetooth_low_energy_device_mac.mm#newcode275 device/bluetooth/bluetooth_low_energy_device_mac.mm:275: ...
4 years, 6 months ago (2016-06-15 16:16:37 UTC) #33
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/180001
4 years, 6 months ago (2016-06-15 16:37:49 UTC) #35
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/21396) ios-device-gn on ...
4 years, 6 months ago (2016-06-15 16:39:59 UTC) #37
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/200001
4 years, 6 months ago (2016-06-15 17:22:25 UTC) #39
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/127744)
4 years, 6 months ago (2016-06-15 18:15:57 UTC) #41
ortuno
https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm#newcode85 device/bluetooth/bluetooth_remote_gatt_service_mac.mm:85: void BluetoothRemoteGattServiceMac::DidDiscoverCharacteristics() { When would you call this method ...
4 years, 6 months ago (2016-06-15 19:28:51 UTC) #42
jlebel
https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm#newcode85 device/bluetooth/bluetooth_remote_gatt_service_mac.mm:85: void BluetoothRemoteGattServiceMac::DidDiscoverCharacteristics() { On 2016/06/15 19:28:51, ortuno wrote: > ...
4 years, 6 months ago (2016-06-15 20:28:47 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/220001
4 years, 6 months ago (2016-06-15 20:52:24 UTC) #45
ortuno
lgtm https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1950033002/diff/200001/device/bluetooth/bluetooth_remote_gatt_service_mac.mm#newcode117 device/bluetooth/bluetooth_remote_gatt_service_mac.mm:117: GetMacAdapter()->NotifyGattServiceChanged(this); On 2016/06/15 at 20:28:47, jlebel wrote: > ...
4 years, 6 months ago (2016-06-15 20:57:47 UTC) #46
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/127929)
4 years, 6 months ago (2016-06-15 22:07:00 UTC) #48
msarda
lgtm
4 years, 6 months ago (2016-06-16 08:24:30 UTC) #49
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/240001
4 years, 6 months ago (2016-06-16 08:38:33 UTC) #51
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/260001
4 years, 6 months ago (2016-06-16 08:41:23 UTC) #53
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-16 10:03:03 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/260001
4 years, 6 months ago (2016-06-16 10:47:07 UTC) #58
commit-bot: I haz the power
Committed patchset #14 (id:260001)
4 years, 6 months ago (2016-06-16 10:51:18 UTC) #60
commit-bot: I haz the power
CQ bit was unchecked
4 years, 6 months ago (2016-06-16 10:51:28 UTC) #61
commit-bot: I haz the power
Patchset 14 (id:??) landed as https://crrev.com/6af3e124c3e3745f280d3b0c5c9491a5e603340f Cr-Commit-Position: refs/heads/master@{#400127}
4 years, 6 months ago (2016-06-16 10:53:07 UTC) #63
Henrik Grunell
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2071853002/ by grunell@chromium.org. ...
4 years, 6 months ago (2016-06-16 13:47:15 UTC) #64
ortuno
On 2016/06/16 at 13:47:15, grunell wrote: > A revert of this CL (patchset #14 id:260001) ...
4 years, 6 months ago (2016-06-16 14:46:56 UTC) #65
blink-reviews
Same question I've asked Jerome ;) On Thu, Jun 16, 2016 at 4:46 PM <ortuno@chromium.org> ...
4 years, 6 months ago (2016-06-16 15:46:50 UTC) #66
chromium-reviews
Same question I've asked Jerome ;) On Thu, Jun 16, 2016 at 4:46 PM <ortuno@chromium.org> ...
4 years, 6 months ago (2016-06-16 15:46:51 UTC) #67
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/280001
4 years, 6 months ago (2016-06-16 16:12:26 UTC) #71
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-16 19:10:16 UTC) #73
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950033002/280001
4 years, 6 months ago (2016-06-16 21:29:50 UTC) #76
commit-bot: I haz the power
Committed patchset #15 (id:280001)
4 years, 6 months ago (2016-06-16 21:41:29 UTC) #78
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/027333819f3fb880a7e433157dfc4c55d3bf8268 Cr-Commit-Position: refs/heads/master@{#400266}
4 years, 6 months ago (2016-06-16 21:43:45 UTC) #80
ortuno
4 years, 6 months ago (2016-06-17 00:31:25 UTC) #81
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2078773002/ by ortuno@chromium.org.

The reason for reverting is: Breaks mac bot:

https://bugs.chromium.org/p/chromium/issues/detail?id=620917#c1.

Powered by Google App Engine
This is Rietveld 408576698