Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12539004:
Implemented BluetoothServiceRecordMac with unittest. (Closed)
|
Created:
7 years, 9 months ago by youngki Modified:
7 years, 9 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplemented BluetoothServiceRecordMac with unittest.
BUG=135472
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187783
Patch Set 1 #
Total comments: 30
Patch Set 2 : Remove space after colon #
Total comments: 14
Patch Set 3 : Few more nits. #
Total comments: 2
Patch Set 4 : added BluetoothServiceRecordMac::~BluetoothServiceRecordMac() #
Messages
Total messages: 13 (0 generated)
Hi Mark, I am new to OSX development and this is pretty much my first non-trivial CL in Objective C++. Could you review my CL for Objective-C++ review? Once I get a LGTM from you I will assign this to the reviewer for the OWNERS. Thanks!
https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac.h (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.h:22: BluetoothServiceRecordMac(IOBluetoothSDPServiceRecord* record); explicit. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic... https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:7: #import <IOBluetooth/Bluetooth.h> You #include C[++] headers and #import Objective-C headers. This one is a C header. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:28: for (NSUInteger i = 0; i < [inner_elements count]; i++) { You can use fast enumeration: for (IOBluetoothSDPDataElement* inner_element in inner_elements) { https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:30: [inner_elements objectAtIndex: i]; Continuation lines should be indented by 4, but you’re going to be removing this statement anyway so it’s moot. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:32: kBluetoothSDPDataElementTypeUUID) { I believe this will fit on a single line. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:33: sdp_uuid = [[inner_element getUUIDValue] getUUIDWithLength: 16]; There is no space between the : and its argument. This occurs throughout the change. Consult the examples throughout http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml . https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:54: name_ = base::SysNSStringToUTF8([record getServiceName]); Please use initializer list format instead of initializing in the constructor body. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:64: if (service_class_data != nil && You don’t need to check for nil. See http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=nil_C... and http://developer.apple.com/library/ios/documentation/cocoa/conceptual/Program... . https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:14: #include "device/bluetooth/bluetooth_service_record_mac.h" The test corresponding to this header is allowed to #include its own header first, before even C system includes. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... . https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:21: = 0x0004; I think it’s more usual to see the = at the end of the preceding line. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:27: const char kRfcommUuid[] = "0123456789abcdef0123456789abcdef"; kRfcommUuid, kUpperCaseUuid, kShortUuid, and kMediumUuid do not need to be file-scoped constants in this anonymous namespace. Each is only used in a single function. Please put the constants down at the point of use. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:28: const int kRfcommUuidSize = sizeof(kRfcommUuid) / 2; These are size_t, not int. But I don’t think you need constants for these. I think the code below reads more clearly if you write your sizeof / 2 expressions below in the code. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:40: Extra blank line? https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:44: } } // namespace https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:58: return [IOBluetoothSDPUUID uuidWithBytes: uuid_buffer length: uuid_size]; No space after the : for an argument in an Objective-C message. Throughout.
Thanks for your review Mark..! I just read the objective C style guide and fixed issues according to the style guide and your comments. Please take another look. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac.h (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.h:22: BluetoothServiceRecordMac(IOBluetoothSDPServiceRecord* record); On 2013/03/11 22:38:48, Mark Mentovai wrote: > explicit. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic... Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:7: #import <IOBluetooth/Bluetooth.h> On 2013/03/11 22:38:48, Mark Mentovai wrote: > You #include C[++] headers and #import Objective-C headers. This one is a C > header. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:28: for (NSUInteger i = 0; i < [inner_elements count]; i++) { On 2013/03/11 22:38:48, Mark Mentovai wrote: > You can use fast enumeration: > > for (IOBluetoothSDPDataElement* inner_element in inner_elements) { Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:30: [inner_elements objectAtIndex: i]; On 2013/03/11 22:38:48, Mark Mentovai wrote: > Continuation lines should be indented by 4, but you’re going to be removing this > statement anyway so it’s moot. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:32: kBluetoothSDPDataElementTypeUUID) { On 2013/03/11 22:38:48, Mark Mentovai wrote: > I believe this will fit on a single line. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:33: sdp_uuid = [[inner_element getUUIDValue] getUUIDWithLength: 16]; On 2013/03/11 22:38:48, Mark Mentovai wrote: > There is no space between the : and its argument. This occurs throughout the > change. Consult the examples throughout > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml . Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:54: name_ = base::SysNSStringToUTF8([record getServiceName]); On 2013/03/11 22:38:48, Mark Mentovai wrote: > Please use initializer list format instead of initializing in the constructor > body. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac.mm:64: if (service_class_data != nil && On 2013/03/11 22:38:48, Mark Mentovai wrote: > You don’t need to check for nil. See > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=nil_C... > and > http://developer.apple.com/library/ios/documentation/cocoa/conceptual/Program... > . Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_service_record_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:14: #include "device/bluetooth/bluetooth_service_record_mac.h" On 2013/03/11 22:38:48, Mark Mentovai wrote: > The test corresponding to this header is allowed to #include its own header > first, before even C system includes. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > . Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:21: = 0x0004; On 2013/03/11 22:38:48, Mark Mentovai wrote: > I think it’s more usual to see the = at the end of the preceding line. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:27: const char kRfcommUuid[] = "0123456789abcdef0123456789abcdef"; On 2013/03/11 22:38:48, Mark Mentovai wrote: > kRfcommUuid, kUpperCaseUuid, kShortUuid, and kMediumUuid do not need to be > file-scoped constants in this anonymous namespace. Each is only used in a single > function. Please put the constants down at the point of use. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:28: const int kRfcommUuidSize = sizeof(kRfcommUuid) / 2; On 2013/03/11 22:38:48, Mark Mentovai wrote: > These are size_t, not int. But I don’t think you need constants for these. I > think the code below reads more clearly if you write your sizeof / 2 expressions > below in the code. Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:40: On 2013/03/11 22:38:48, Mark Mentovai wrote: > Extra blank line? Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:44: } On 2013/03/11 22:38:48, Mark Mentovai wrote: > } // namespace Done. https://chromiumcodereview.appspot.com/12539004/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_service_record_mac_unittest.mm:58: return [IOBluetoothSDPUUID uuidWithBytes: uuid_buffer length: uuid_size]; On 2013/03/11 22:38:48, Mark Mentovai wrote: > No space after the : for an argument in an Objective-C message. Throughout. Done.
https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:8: #include <IOBluetooth/IOBluetoothUtilities.h> If you look at this header, you’ll see that this one actually is an Objective-C header when included by __OBJC__, so #import’s good here. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:22: const BluetoothSDPServiceAttributeID kServiceClassId = 1; This doesn’t need to be file-scoped, you can move it down to the function where it’s used. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:28: for (IOBluetoothSDPDataElement* inner_element in inner_elements) { Assuming -getArrayValue did return an array (as opposed to nil), might the inner_elements array contain objects of other types than IOBluetoothSDPDataElement? https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:50: IOBluetoothSDPServiceRecord* record) : device_([record device]) { Since you already had to break the line for the first argument, you should put the initializer list on a new line too, like: BSRM::BSRM( IOBSSR* record) : device_([record device]) { https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:51: name_ = base::SysNSStringToUTF8([record getServiceName]); There are fields in BluetoothServiceRecord that aren’t initialized by this constructor or the BluetoothServiceRecord constructor. Is it OK to leave those values undefined? https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:51: name_ = base::SysNSStringToUTF8([record getServiceName]); Why don’t you initialize the rest of the fields in the initializer list too? That’s what I had in mind when I suggested it… BSRM::BSRM( IOBSSR* record) : device_([record device]), name_(base::SysNSStringToUTF8(…)), address_(…), supports_rfcomm_(…) { OK, I see that this won’t work because some of the fields are protected members of the superclass. I got it. Since you can’t initialize some fields in this manner, then it’s probably better to be consistent and not initialize any of them. Sorry for the runaround. However, to be explicit about the inheritance relationship, you should call the superclass’ constructor from the initializer list: BSRM::BSRM( IOBSSR* record) : BluetoothServiceRecord() { https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... File device/bluetooth/bluetooth_service_record_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac_unittest.mm:40: IOBluetoothSDPUUID* ConvertUuid(const char* uuid_hex_char, int uuid_size) { This is more of a size_t than an int.
https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:8: #include <IOBluetooth/IOBluetoothUtilities.h> On 2013/03/12 16:02:54, Mark Mentovai wrote: > If you look at this header, you’ll see that this one actually is an Objective-C > header when included by __OBJC__, so #import’s good here. Done. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:22: const BluetoothSDPServiceAttributeID kServiceClassId = 1; On 2013/03/12 16:02:54, Mark Mentovai wrote: > This doesn’t need to be file-scoped, you can move it down to the function where > it’s used. Done. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:28: for (IOBluetoothSDPDataElement* inner_element in inner_elements) { I think it's safe to assume the type is only IOBluetoothSDPDataElement according to the specification: http://developer.apple.com/library/mac/#documentation/IOBluetooth/Reference/I... Basically I am checking if |service_class_data| type descriptor is a sequence type before calling ExtractUuid(), which means that getArrayValue() will return an array of IOBluetoothSDPDataElement's. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:50: IOBluetoothSDPServiceRecord* record) : device_([record device]) { On 2013/03/12 16:02:54, Mark Mentovai wrote: > Since you already had to break the line for the first argument, you should put > the initializer list on a new line too, like: > > BSRM::BSRM( > IOBSSR* record) > : device_([record device]) { Done. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:51: name_ = base::SysNSStringToUTF8([record getServiceName]); On 2013/03/12 16:02:54, Mark Mentovai wrote: > There are fields in BluetoothServiceRecord that aren’t initialized by this > constructor or the BluetoothServiceRecord constructor. Is it OK to leave those > values undefined? Done. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac.mm:51: name_ = base::SysNSStringToUTF8([record getServiceName]); On 2013/03/12 16:02:54, Mark Mentovai wrote: > There are fields in BluetoothServiceRecord that aren’t initialized by this > constructor or the BluetoothServiceRecord constructor. Is it OK to leave those > values undefined? Done. https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... File device/bluetooth/bluetooth_service_record_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/8001/device/bluetooth/bl... device/bluetooth/bluetooth_service_record_mac_unittest.mm:40: IOBluetoothSDPUUID* ConvertUuid(const char* uuid_hex_char, int uuid_size) { On 2013/03/12 16:02:54, Mark Mentovai wrote: > This is more of a size_t than an int. Done.
LGTM
Thanks Mark for the language review...! Scott, could you review the CL?
lgtm
https://chromiumcodereview.appspot.com/12539004/diff/13001/device/bluetooth/b... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/13001/device/bluetooth/b... device/bluetooth/bluetooth_service_record_mac.mm:51: device_ = [record device]; Whoops, sorry, one more. You should retain device_ here, since you want for this object to own it. device_ = [[record device] retain]; You will then also need to release it in the destructor.
https://chromiumcodereview.appspot.com/12539004/diff/13001/device/bluetooth/b... File device/bluetooth/bluetooth_service_record_mac.mm (right): https://chromiumcodereview.appspot.com/12539004/diff/13001/device/bluetooth/b... device/bluetooth/bluetooth_service_record_mac.mm:51: device_ = [record device]; On 2013/03/12 20:03:04, Mark Mentovai wrote: > Whoops, sorry, one more. > > You should retain device_ here, since you want for this object to own it. > > device_ = [[record device] retain]; > > You will then also need to release it in the destructor. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/12539004/23001
Message was sent while issue was closed.
Change committed as 187783 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
