Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767813002:
Bluetooth: macOS: Implementing read/write for descriptors (Closed)
DescriptionBluetooth: macOS: Implementing read/write for descriptors
Implementation and unit tests.
BUG=624017
Review-Url: https://codereview.chromium.org/2767813002
Cr-Commit-Position: refs/heads/master@{#463740}
Committed: https://chromium.googlesource.com/chromium/src/+/1cc94fe3b88dc9f048d4fce8c8814fd334d120d1
Patch Set 1 #Patch Set 2 : Adding NSError logs (and merge) #
Total comments: 20
Patch Set 3 : NSString/NSNumber type for descriptor value #
Total comments: 28
Patch Set 4 : . #
Total comments: 6
Patch Set 5 : Removing CFNumberGetByteSize() #
Total comments: 3
Patch Set 6 : . #Messages
Total messages: 39 (23 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Bluetooth: macOS: Implementing read/write for descriptors BUG=624017 ========== to ========== Bluetooth: macOS: Implementing read/write for descriptors Implementation and unit tests. BUG=624017 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
jlebel@chromium.org changed reviewers: + ortuno@chromium.org
Hello Giovanni, Can you review this patch related to implement descriptors on macOS? The tricky part is to convert the received descriptor value to a vector of uint8. According to Apple documentation, we can receive NSNumber. But according to the test we've done with François, we receive only NSString and NSData, but no NSNumber. I would be interested if you can test that on your side. And if you have suggestion about converting values. Thanks,
Looking good! re: "value", wow that's a really bad API choice. Left some more comments in the patch. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h:73: bool value_read_or_write_in_progress_; Don't forget to initialize this, otherwise we will get random IN_PROGRESS errors. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:18: std::vector<uint8_t> VectorValueFromNSValue(id nsvalue) { This name is pretty confusing since nsvalue is not an actual NSValue. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:20: // https://developer.apple.com/reference/corebluetooth/cbdescriptor some of We should handle all documented cases. It's certainly possible that Apple notices the bug, fixes it and breaks our code. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:34: } Could we log the class if it's not one of the three expected classes? https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:97: // Sends a write request to a remote characteristic descriptor, to modify the Why do we need this comment? https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:134: << BluetoothAdapterMac::String(error) Didn't we have a patch to make it easier to print errors? https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:139: VLOG(1) << *this << ": Value updated."; nit: Value read. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:162: VLOG(1) << *this << ": Value updated."; nit: Value written. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc:721: #endif // defined(OS_ANDROID) || defined(OS_MACOSX) Could you add test to make sure we can handle the relevant types of NSValue. You will probably have to introduce some macOS only test functions: - SimulateGattDescriptorReadNSData - SimulateGattDescriptorReadNSString - SimulateGattDescriptorReadNSNumber You can then implement SimulateGattDescriptorRead() on top of SimulateGattDescriptorReadNSData.
https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:68: BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {} We need to call pending error callbacks when the object gets destroyed. The necessary code and tests should be very similar to what we have for characteristics.
Thanks for your input. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h:73: bool value_read_or_write_in_progress_; On 2017/04/03 01:27:50, ortuno wrote: > Don't forget to initialize this, otherwise we will get random IN_PROGRESS > errors. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:18: std::vector<uint8_t> VectorValueFromNSValue(id nsvalue) { On 2017/04/03 01:27:50, ortuno wrote: > This name is pretty confusing since nsvalue is not an actual NSValue. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:20: // https://developer.apple.com/reference/corebluetooth/cbdescriptor some of On 2017/04/03 01:27:50, ortuno wrote: > We should handle all documented cases. It's certainly possible that Apple > notices the bug, fixes it and breaks our code. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:34: } On 2017/04/03 01:27:50, ortuno wrote: > Could we log the class if it's not one of the three expected classes? Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:68: BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {} On 2017/04/03 04:51:46, ortuno wrote: > We need to call pending error callbacks when the object gets destroyed. The > necessary code and tests should be very similar to what we have for > characteristics. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:97: // Sends a write request to a remote characteristic descriptor, to modify the On 2017/04/03 01:27:50, ortuno wrote: > Why do we need this comment? Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:134: << BluetoothAdapterMac::String(error) On 2017/04/03 01:27:50, ortuno wrote: > Didn't we have a patch to make it easier to print errors? I did that: crrev.com/2745323002 crrev.com/2784373003 So that's the best I came up with. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:139: VLOG(1) << *this << ": Value updated."; On 2017/04/03 01:27:50, ortuno wrote: > nit: Value read. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:162: VLOG(1) << *this << ": Value updated."; On 2017/04/03 01:27:50, ortuno wrote: > nit: Value written. Done. https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc (right): https://codereview.chromium.org/2767813002/diff/140002/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc:721: #endif // defined(OS_ANDROID) || defined(OS_MACOSX) On 2017/04/03 01:27:50, ortuno wrote: > Could you add test to make sure we can handle the relevant types of NSValue. You > will probably have to introduce some macOS only test functions: > > - SimulateGattDescriptorReadNSData > - SimulateGattDescriptorReadNSString > - SimulateGattDescriptorReadNSNumber > > You can then implement SimulateGattDescriptorRead() on top of > SimulateGattDescriptorReadNSData. I'm concerned about endianess. I'm not sure what is the endianess for the result of NSNumber.
Patchset #3 (id:170001) has been deleted
https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:433: return gatt_characteristic->GetBluetoothRemoteGattDescriptorMac( optional: Is GetBluetoothRemoteGattDescriptorMac always expected to return a descriptor? If so then maybe we should return a reference so that users of the function don't have to check if the descriptor is there. I recognize that we do this for all GetBluetoothRemote*Mac() functions already so maybe we should just fix it for all in a separate patch. Not high priority though. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:79: if (!read_value_callbacks_.first.is_null()) { We need tests like [1] to test that this is getting called. Branch point is close so no need to it in this patch. [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:118: // Sends a write request to a remote characteristic descriptor. |callback| is I think its weird to have these comments in the .mm file of a subclass. There is one already in the base class' .h file. Just remove it.
ortuno@chromium.org changed reviewers: + tapted@chromium.org
+tapted for the function that converts from value to vector.
https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:18: @class CBCharacteristic; note @class isn't a valid keyword when __OBJC__ is not defined, so I don't think this case is ever compiled https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:8: #include "base/mac/foundation_util.h" nit: #import https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:11: #include "device/bluetooth/bluetooth_adapter_mac.h" #import https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:12: #include "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h" #import as well https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:18: std::vector<uint8_t> VectorValueFromObjC(id objcValue) { objcValue -> objc_value (or just value) - we only declare new things camelCase between @foo/@end https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:24: NSString* string = ObjCCast<NSString>(objcValue); ObjCCast also does isKindOfClass, so there's a bit or repetition here. The malloc and other stuff is all a bit scary. Really I think The neatest and safest way to do this is to convert all types to NSData. I'd even suggest converting numbers to strings first to remove questions around byte order and things like that and just keep things simpler, but maybe that will break stuff. NSData data = ObjCCast<NSData>(objc_value); NSString* as_string = ObjCCast<NSString>(objc_value); NSNumber* as_number = ObjCCast<NSNumber>(objc_value); if (!data && !as_string && as_number) { // Simpler, but might not work: as_string = [as_number stringValue]; long descriptor = [as_number longValue]; data = [NSData dataWithBytes:&descriptor length:sizeof(descriptor)]; } if (!data && as_string) data = [as_string dataUsingEncoding:NSUTF8StringEncoding]; DCHECK(data) << "Unexpected .." if (data) { value.resize([data length]); [data getBytes:value.data() length:value.size()]; } return value; The NS/CFNumber APIs are free to change data sizes when they're initialized, so I'd verify that you really need stuff like CFNumberGetByteSize before using it -- it's probably not useful. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:25: const uint8_t* buffer = (const uint8_t*)(string.UTF8String); note style guide prohibits C-style casts (use reinterpret_cast instead) https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:31: uint8_t* buffer = (uint8_t*)malloc(byteSize); memory leak? - nothing frees |buffer| https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:37: const uint8_t* buffer = (const uint8_t*)(data.bytes); data.bytes may return nil - this is really dangerous code. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:159: value_ = VectorValueFromObjC(cb_descriptor_.get().value); we don't use ObjC dot notation for NSObjects in Chrome (only for structs like NSSize). Instead of `cb_descriptor_.get().value` it should be `[cb_descriptor_ value]` so it's clear this is an ObjC message send. (all the other places in this file that call ".get()" on scoped_nsobject should be updated too)
Thanks Trent and Giovanni for your inputs. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:433: return gatt_characteristic->GetBluetoothRemoteGattDescriptorMac( On 2017/04/04 23:39:43, ortuno wrote: > optional: Is GetBluetoothRemoteGattDescriptorMac always expected to return a > descriptor? If so then maybe we should return a reference so that users of the > function don't have to check if the descriptor is there. I recognize that we do > this for all GetBluetoothRemote*Mac() functions already so maybe we should just > fix it for all in a separate patch. Not high priority though. crbug.com/709100 https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:18: @class CBCharacteristic; On 2017/04/05 00:38:37, tapted wrote: > note @class isn't a valid keyword when __OBJC__ is not defined, so I don't think > this case is ever compiled Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:8: #include "base/mac/foundation_util.h" On 2017/04/05 00:38:37, tapted wrote: > nit: #import Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:11: #include "device/bluetooth/bluetooth_adapter_mac.h" On 2017/04/05 00:38:37, tapted wrote: > #import Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:12: #include "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h" On 2017/04/05 00:38:37, tapted wrote: > #import as well Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:18: std::vector<uint8_t> VectorValueFromObjC(id objcValue) { On 2017/04/05 00:38:37, tapted wrote: > objcValue -> objc_value (or just value) - we only declare new things camelCase > between @foo/@end Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:24: NSString* string = ObjCCast<NSString>(objcValue); On 2017/04/05 00:38:37, tapted wrote: > ObjCCast also does isKindOfClass, so there's a bit or repetition here. The > malloc and other stuff is all a bit scary. Really I think The neatest and safest > way to do this is to convert all types to NSData. I'd even suggest converting > numbers to strings first to remove questions around byte order and things like > that and just keep things simpler, but maybe that will break stuff. > > NSData data = ObjCCast<NSData>(objc_value); > NSString* as_string = ObjCCast<NSString>(objc_value); > NSNumber* as_number = ObjCCast<NSNumber>(objc_value); > > if (!data && !as_string && as_number) { > // Simpler, but might not work: as_string = [as_number stringValue]; > long descriptor = [as_number longValue]; > data = [NSData dataWithBytes:&descriptor length:sizeof(descriptor)]; > } > > if (!data && as_string) > data = [as_string dataUsingEncoding:NSUTF8StringEncoding]; > > DCHECK(data) << "Unexpected .." > if (data) { > value.resize([data length]); > [data getBytes:value.data() length:value.size()]; > } > return value; > > > The NS/CFNumber APIs are free to change data sizes when they're initialized, so > I'd verify that you really need stuff like CFNumberGetByteSize before using it > -- it's probably not useful. about the first "if": I don't understand why we need to test data, as_string and as_number. I can't imagine to have data and as_number set. about DCHECK followed by "if (data)" I've been told I should not test a value after a DCHECK(): either the DCHECK can fail, so I need to remove the DCHECK(), or the DCHECK should never fails, so there is no need to test right after. I think it is really important to use CFNumberGetByteSize(). It would not make sense for the developer to receive 8 bytes when the device sends only 2 bytes. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:25: const uint8_t* buffer = (const uint8_t*)(string.UTF8String); On 2017/04/05 00:38:37, tapted wrote: > note style guide prohibits C-style casts (use reinterpret_cast instead) Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:31: uint8_t* buffer = (uint8_t*)malloc(byteSize); On 2017/04/05 00:38:37, tapted wrote: > memory leak? - nothing frees |buffer| Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:37: const uint8_t* buffer = (const uint8_t*)(data.bytes); On 2017/04/05 00:38:37, tapted wrote: > data.bytes may return nil - this is really dangerous code. According to the documentation, -[NSData bytes] can return nil when there is no data. So -[NSData length] should return 0. I think therefore there should be no crash, since no bytes should be read from nil. Am I wrong? https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:79: if (!read_value_callbacks_.first.is_null()) { On 2017/04/04 23:39:43, ortuno wrote: > We need tests like [1] to test that this is getting called. Branch point is > close so no need to it in this patch. > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... crbug.com/709066 https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:118: // Sends a write request to a remote characteristic descriptor. |callback| is On 2017/04/04 23:39:43, ortuno wrote: > I think its weird to have these comments in the .mm file of a subclass. There is > one already in the base class' .h file. Just remove it. Done. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:159: value_ = VectorValueFromObjC(cb_descriptor_.get().value); On 2017/04/05 00:38:37, tapted wrote: > we don't use ObjC dot notation for NSObjects in Chrome (only for structs like > NSSize). Instead of `cb_descriptor_.get().value` it should be `[cb_descriptor_ > value]` so it's clear this is an ObjC message send. > > (all the other places in this file that call ".get()" on scoped_nsobject should > be updated too) Done. For the code I wrote in other files: crbug.com/709104
https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:24: NSString* string = ObjCCast<NSString>(objcValue); On 2017/04/06 17:48:49, jlebel wrote: > On 2017/04/05 00:38:37, tapted wrote: > > ObjCCast also does isKindOfClass, so there's a bit or repetition here. The > > malloc and other stuff is all a bit scary. Really I think The neatest and > safest > > way to do this is to convert all types to NSData. I'd even suggest converting > > numbers to strings first to remove questions around byte order and things like > > that and just keep things simpler, but maybe that will break stuff. > > > > NSData data = ObjCCast<NSData>(objc_value); > > NSString* as_string = ObjCCast<NSString>(objc_value); > > NSNumber* as_number = ObjCCast<NSNumber>(objc_value); > > > > if (!data && !as_string && as_number) { > > // Simpler, but might not work: as_string = [as_number stringValue]; > > long descriptor = [as_number longValue]; > > data = [NSData dataWithBytes:&descriptor length:sizeof(descriptor)]; > > } > > > > if (!data && as_string) > > data = [as_string dataUsingEncoding:NSUTF8StringEncoding]; > > > > DCHECK(data) << "Unexpected .." > > if (data) { > > value.resize([data length]); > > [data getBytes:value.data() length:value.size()]; > > } > > return value; > > > > > > The NS/CFNumber APIs are free to change data sizes when they're initialized, > so > > I'd verify that you really need stuff like CFNumberGetByteSize before using it > > -- it's probably not useful. > > about the first "if": > I don't understand why we need to test data, as_string and as_number. I can't > imagine to have data and as_number set. Yep - it would be an error if more than one is set. This was just about organising the logic nicely. > > about DCHECK followed by "if (data)" > I've been told I should not test a value after a DCHECK(): either the DCHECK can > fail, so I need to remove the DCHECK(), or the DCHECK should never fails, so > there is no need to test right after. Dealing with external APIs gives some leeway on this IMO. The code currently captures all the documented possibilities, but we don't have a guarantee that Apple introduce a new data type in the future. We want to detect that that should be handled, but we don't want to introduce a security bug or crash users machines. > > I think it is really important to use CFNumberGetByteSize(). It would not make > sense for the developer to receive 8 bytes when the device sends only 2 bytes. See later comment. https://codereview.chromium.org/2767813002/diff/190001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:37: const uint8_t* buffer = (const uint8_t*)(data.bytes); On 2017/04/06 17:48:49, jlebel wrote: > On 2017/04/05 00:38:37, tapted wrote: > > data.bytes may return nil - this is really dangerous code. > > According to the documentation, -[NSData bytes] can return nil when there is no > data. So -[NSData length] should return 0. I think therefore there should be no > crash, since no bytes should be read from nil. Am I wrong? Hm - maybe you're safe with C++ vectors. It scares me still though - passing null to memcpy is undefined behaviour, even if the length is zero. https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:29: CFIndex byteSize = CFNumberGetByteSize(number); https://developer.apple.com/reference/corefoundation/1542080-cfnumbergetbytes... says "Because a CFNumber object might store a value using a type different from that of the original value with which it was created, this function may return a size different from the size of the original value's type." It also says "In order to improve performance, some commonly-used numbers (such as 0 and 1) are uniqued. You should not expect that allocating multiple CFNumber instances will necessarily result in distinct objects." So whenever a device reports `0` or `1` it's going to point to some shared NSNumber whose size may not match the bytesize of the number used to create it. https://developer.apple.com/reference/foundation/nsnumber?language=objc also says "(Note that number objects do not necessarily preserve the type they are created with.)" (I'd hazard a guess that these descriptors are always `int` anyway..) So I really think `CFNumberGetByteSize` is giving a false sense of usefulness. If you still think otherwise - I don't think we should use malloc -- it doesn't belong in modern C++. For this case I think it would make sense to skip NSData, do the value.resize() and assign into it rather than malloc. But that doesn't override my next concern. (also byteSize -> byte_size) https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:30: CFNumberType type = CFNumberGetType(number); Maybe I'm overthinking it, but I can't tell from reading the API whether CFNumberGetByteSize guarantees that it returns sizeof(primitive-type-returned-by-CFNumberGetType()). That is, it doesn't seem guarantee that the buffer will be big enough for CFNumberGetValue to write to. The way CFNumberGetValue is meant to be used is something like int value; if (!CFNumberGetValue(number, kCFNumberIntType, &value)) NOTREACHED(); e.g. you could template <class T> std::vector<uint8_t> CopyBitsAs(CFNumberRef number, CFNumberType cf_type) { T value; if (!CFNumberGetValue(number, cf_type, &value)) NOTREACHED(); uint8_t* as_bytes = reinterpret_cast<uint8_t*>(&value); return std::vector<uint8_t>(as_bytes, as_bytes + sizeof(value)); } switch (CFNumberGetType(number)) { case kCFNumberIntType: return CopyBitsAs<int>(number, kCFNumberIntType); etc. Note this approach doesn't use CFNumberGetByteSize at all. But also I don't think this is useful. https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:31: uint8_t* buffer = (uint8_t*)malloc(byteSize); static_cast<> (style guide prohibits C-style casts.)
Hello Trent, I guess it is simpler to not use CFNumberGetByteSize(). If you don't mind, I prefer to use -[NSNumber shortValue] instead of -[NSNumber longValue]. According to Apple documentation, descriptors that are supposed to return NSNumber, are all 2 bytes size. If I don't do that the behavior on macOS will be different that other platform. Anyway, when I tested that I received NSData instead of NSNumber. So we are talking about things that don't work according to the documentation. Thanks, https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:29: CFIndex byteSize = CFNumberGetByteSize(number); On 2017/04/07 04:40:43, tapted wrote: > https://developer.apple.com/reference/corefoundation/1542080-cfnumbergetbytes... > says > > "Because a CFNumber object might store a value using a type different from that > of the original value with which it was created, this function may return a size > different from the size of the original value's type." > > It also says > > "In order to improve performance, some commonly-used numbers (such as 0 and 1) > are uniqued. You should not expect that allocating multiple CFNumber instances > will necessarily result in distinct objects." > > > So whenever a device reports `0` or `1` it's going to point to some shared > NSNumber whose size may not match the bytesize of the number used to create it. > > > https://developer.apple.com/reference/foundation/nsnumber?language=objc also > says > > "(Note that number objects do not necessarily preserve the type they are created > with.)" > > (I'd hazard a guess that these descriptors are always `int` anyway..) > > > So I really think `CFNumberGetByteSize` is giving a false sense of usefulness. > > > If you still think otherwise - I don't think we should use malloc -- it doesn't > belong in modern C++. For this case I think it would make sense to skip NSData, > do the value.resize() and assign into it rather than malloc. But that doesn't > override my next concern. > > > (also byteSize -> byte_size) This documentation means that if we do +[NSNumber numberWithUnsignedShort:], the internal storage might be 4 bytes, and then CFNumberGetByteSize() will not match the original type. And CFNumberGetType() returns the internal type, not the original type. https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:30: CFNumberType type = CFNumberGetType(number); On 2017/04/07 04:40:43, tapted wrote: > Maybe I'm overthinking it, but I can't tell from reading the API whether > CFNumberGetByteSize guarantees that it returns > sizeof(primitive-type-returned-by-CFNumberGetType()). That is, it doesn't seem > guarantee that the buffer will be big enough for CFNumberGetValue to write to. > The way CFNumberGetValue is meant to be used is something like > > int value; > if (!CFNumberGetValue(number, kCFNumberIntType, &value)) > NOTREACHED(); > > > e.g. you could > > template <class T> > std::vector<uint8_t> CopyBitsAs(CFNumberRef number, > CFNumberType cf_type) { > T value; > if (!CFNumberGetValue(number, cf_type, &value)) > NOTREACHED(); > uint8_t* as_bytes = reinterpret_cast<uint8_t*>(&value); > return std::vector<uint8_t>(as_bytes, as_bytes + sizeof(value)); > } > > > switch (CFNumberGetType(number)) { > case kCFNumberIntType: > return CopyBitsAs<int>(number, kCFNumberIntType); > etc. > > > Note this approach doesn't use CFNumberGetByteSize at all. > > But also I don't think this is useful. Ok, I'm going for the simple version, but at least, I will limit to 2 bytes. According to the documentation all descriptors with NSNumber should return 2 bytes only. With this, the behavior should the same on macOS and on other platforms. https://codereview.chromium.org/2767813002/diff/210001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:31: uint8_t* buffer = (uint8_t*)malloc(byteSize); On 2017/04/07 04:40:43, tapted wrote: > static_cast<> (style guide prohibits C-style casts.) Done.
VectorValueFromObjC lgtm - I skimmed the other ObjC stuff in the diff - looks good too. The device/bluetooth/test/mock_* files are not following the ObjC style guide ( http://go/objcguide ), but that's orthogonal to this CL.
lgtm bar small logging request. https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:35: DCHECK(data) << "Unexpected value: " Can we log this?
https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:35: DCHECK(data) << "Unexpected value: " On 2017/04/10 at 01:52:11, ortuno wrote: > Can we log this? DCHECK usually signals that that case will never happen. So readers don't have to worry about that case. Having the "if" right after the DCHECK is really confusing to me. I would remove the DCHECK and add an else statement to "if (data)": if (data) { // ... } else { // In case of new unhandled types. LOG(WARNING) << "Unexpected value: " << base::SysNSStringToUTF8(...); }
Thanks, https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right): https://codereview.chromium.org/2767813002/diff/230001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:35: DCHECK(data) << "Unexpected value: " On 2017/04/10 23:36:39, ortuno wrote: > On 2017/04/10 at 01:52:11, ortuno wrote: > > Can we log this? > > DCHECK usually signals that that case will never happen. So readers don't have > to worry about that case. Having the "if" right after the DCHECK is really > confusing to me. > > I would remove the DCHECK and add an else statement to "if (data)": > > if (data) { > // ... > } else { > // In case of new unhandled types. > LOG(WARNING) << "Unexpected value: " << base::SysNSStringToUTF8(...); > } Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2767813002/#ps250001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 250001, "attempt_start_ts": 1491936577914260,
"parent_rev": "856f9774c5070251468d106f751acb81c5693d16", "commit_rev":
"1cc94fe3b88dc9f048d4fce8c8814fd334d120d1"}
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: macOS: Implementing read/write for descriptors Implementation and unit tests. BUG=624017 ========== to ========== Bluetooth: macOS: Implementing read/write for descriptors Implementation and unit tests. BUG=624017 Review-Url: https://codereview.chromium.org/2767813002 Cr-Commit-Position: refs/heads/master@{#463740} Committed: https://chromium.googlesource.com/chromium/src/+/1cc94fe3b88dc9f048d4fce8c881... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:250001) as https://chromium.googlesource.com/chromium/src/+/1cc94fe3b88dc9f048d4fce8c881... |
