Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12929003:
Implemented BluetoothAdapterMac::AddDevices(). (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 BluetoothAdapterMac::AddDevices() and BluetoothAdapterMac::RemoveUnpairedDevices().
This method will update the paired devices and make this information accessible from Bluetooth API.
BUG=135472
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190560
Patch Set 1 #
Total comments: 30
Patch Set 2 : DeviceRemoved obsolete in OSX. #
Total comments: 12
Patch Set 3 : used pairedDevices instead of recentDevices #
Total comments: 6
Patch Set 4 : Changed TestIOBluetoothDevice properties. #Patch Set 5 : Replicate specific 10.7 SDK declarations. #Patch Set 6 : Added Bluetooth.h. #Patch Set 7 : Changed the format type to unsigned long. #
Total comments: 10
Patch Set 8 : Remove SetVisible() logic in BluetoothDeviceMac. #Patch Set 9 : Reverted BluetoothDevice interface. #
Total comments: 1
Messages
Total messages: 26 (0 generated)
Mark, could you review this CL for object-c language usage?
https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.h:69: // |DeviceChanged|, and |DeviceRemoved| observer calls. This comment should explain what |devices| is, because the declaration just says “NSArray.” NSArray of what? https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:7: #import <IObluetooth/objc/IOBluetoothDevice.h> Fix capitalization. This only works because you’re on a case-insensitive filesystem. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:174: // Remove devices that are no longer known by the system. How does something become unknown by the system? The documentation for +[IOBluetoothDevice recentDevices:] says > The resulting array contains IOBluetoothDevice objects sorted in reverse > chronological order. The most recently accessed devices are first. If the > numDevices parameter is 0, all devices accessed by the system are returned. > If numDevices is non-zero, only the most recent devices are returned. All devices accessed by the system? Since the beginning of time? https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:177: DevicesMap::iterator temp = device_iter; This isn’t really temporary at all, you use it throughout the entire loop. It needs a better name (which may also mean changing the name of device_iter). https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:184: BluetoothDeviceMac* device_mac = Pull this out of the enclosing conditional, so that the conditional and lines 192-193 can use the more meaningful device_mac instead of the meaningless temp->second. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:188: DeviceChanged(this, device_mac)); …but it’s not in “devices,” so will it ever be cleaned up after it disconnects or unpairs? Something should explain what’s happening. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_unittest.mm:38: mName = name; This (and your access of mAddress) is really pretty heinous. Do you NEED to recycle the superclass’ fields, or can you get away with having your own fields (and, if necessary, overriding the superclass’ accessors)? https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.h:23: BluetoothDeviceMac(const IOBluetoothDevice* device); This constructor must be explicit now that you’re adding an argument. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.h:26: void SetVisible(bool visible); Shouldn’t this be private? You only need to call this from BluetoothAdapterMac, which is a friend, and I don’t think you want this to be part of the public interface. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:7: #import <IObluetooth/objc/IOBluetoothDevice.h> Fix capitalization. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:24: name_ = base::SysNSStringToUTF8([device name]); You can list all of these in the initializer list. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:129: std::string device_string = base::StringPrintf("%s%s%u%s%s", For fields without fixed width, can you have the different fields delimited by some character that can’t appear in the input, to reduce collision and ambiguity potential? https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:133: [device isConnected] ? "true" : "false", You don’t need to turn these into strings, you can just include them in the printf with %d. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:141: [[record sortedAttributes] count]); If you’re just getting a count, you don’t care if the attributes are sorted or not.
https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.h:69: // |DeviceChanged|, and |DeviceRemoved| observer calls. On 2013/03/19 16:04:35, Mark Mentovai wrote: > This comment should explain what |devices| is, because the declaration just says > “NSArray.” NSArray of what? Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:7: #import <IObluetooth/objc/IOBluetoothDevice.h> On 2013/03/19 16:04:35, Mark Mentovai wrote: > Fix capitalization. This only works because you’re on a case-insensitive > filesystem. Sorry about this; done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:174: // Remove devices that are no longer known by the system. On 2013/03/19 16:04:35, Mark Mentovai wrote: > How does something become unknown by the system? The documentation for > +[IOBluetoothDevice recentDevices:] says > > > The resulting array contains IOBluetoothDevice objects sorted in reverse > > chronological order. The most recently accessed devices are first. If the > > numDevices parameter is 0, all devices accessed by the system are returned. > > If numDevices is non-zero, only the most recent devices are returned. > > All devices accessed by the system? Since the beginning of time? So.. the documentation didn't say that since when all devices accessed by the system. But I think I should follow the documentation and assume that the devices won't be unknown by the system as long as the chromium browser lives. Then the devices will never be unknown by the chromium browser once they are added. I removed the second part of this method implementation. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:177: DevicesMap::iterator temp = device_iter; On 2013/03/19 16:04:35, Mark Mentovai wrote: > This isn’t really temporary at all, you use it throughout the entire loop. It > needs a better name (which may also mean changing the name of device_iter). I removed the code because this is no longer necessary. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:184: BluetoothDeviceMac* device_mac = On 2013/03/19 16:04:35, Mark Mentovai wrote: > Pull this out of the enclosing conditional, so that the conditional and lines > 192-193 can use the more meaningful device_mac instead of the meaningless > temp->second. I removed the code because this is no longer necessary. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:188: DeviceChanged(this, device_mac)); On 2013/03/19 16:04:35, Mark Mentovai wrote: > …but it’s not in “devices,” so will it ever be cleaned up after it disconnects > or unpairs? Something should explain what’s happening. I removed the code because this is no longer necessary. Actually this case won't work because there is no device that is connected but is unknown by the system.. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_unittest.mm:38: mName = name; On 2013/03/19 16:04:35, Mark Mentovai wrote: > This (and your access of mAddress) is really pretty heinous. Do you NEED to > recycle the superclass’ fields, or can you get away with having your own fields > (and, if necessary, overriding the superclass’ accessors)? Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.h:23: BluetoothDeviceMac(const IOBluetoothDevice* device); On 2013/03/19 16:04:35, Mark Mentovai wrote: > This constructor must be explicit now that you’re adding an argument. Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.h:26: void SetVisible(bool visible); On 2013/03/19 16:04:35, Mark Mentovai wrote: > Shouldn’t this be private? You only need to call this from BluetoothAdapterMac, > which is a friend, and I don’t think you want this to be part of the public > interface. Removed this function since it is no longer necessary. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:7: #import <IObluetooth/objc/IOBluetoothDevice.h> On 2013/03/19 16:04:35, Mark Mentovai wrote: > Fix capitalization. Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:24: name_ = base::SysNSStringToUTF8([device name]); On 2013/03/19 16:04:35, Mark Mentovai wrote: > You can list all of these in the initializer list. Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:129: std::string device_string = base::StringPrintf("%s%s%u%s%s", On 2013/03/19 16:04:35, Mark Mentovai wrote: > For fields without fixed width, can you have the different fields delimited by > some character that can’t appear in the input, to reduce collision and ambiguity > potential? Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:133: [device isConnected] ? "true" : "false", On 2013/03/19 16:04:35, Mark Mentovai wrote: > You don’t need to turn these into strings, you can just include them in the > printf with %d. Done. https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_device_mac.mm:141: [[record sortedAttributes] count]); On 2013/03/19 16:04:35, Mark Mentovai wrote: > If you’re just getting a count, you don’t care if the attributes are sorted or > not. Done.
https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:174: // Remove devices that are no longer known by the system. On 2013/03/19 19:04:55, youngki wrote: > On 2013/03/19 16:04:35, Mark Mentovai wrote: > > How does something become unknown by the system? The documentation for > > +[IOBluetoothDevice recentDevices:] says > > > > > The resulting array contains IOBluetoothDevice objects sorted in reverse > > > chronological order. The most recently accessed devices are first. If the > > > numDevices parameter is 0, all devices accessed by the system are returned. > > > If numDevices is non-zero, only the most recent devices are returned. > > > > All devices accessed by the system? Since the beginning of time? > > So.. the documentation didn't say that since when all devices accessed by the > system. But I think I should follow the documentation and assume that the > devices won't be unknown by the system as long as the chromium browser lives. > Then the devices will never be unknown by the chromium browser once they are > added. > > I removed the second part of this method implementation. I don’t know if it’s right to remove it. I wasn’t really expecting you to remove it, I was just looking for an explanation of how it all works. You can look beyond the documentation for answers, like by playing with the system from a user’s perspective and trying to figure out the lifetime based on where it’s storing things. For example, if you remove devices from the Bluetooth system preference pane, do they disappear from the +recentDevices: list? https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:16: NSString* _name; This doesn’t follow the Chromium convention of using a trailing underscore. (Permission to use a leading underscore is new in the Objective-C guide, but when working with existing projects like Chromium that have a long-established tradition of using a trailing underscore, you should follow the local convention.) https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:24: @property (copy) NSString* name; You don’t need either property to be atomic, right? You can declare them both nonatomic. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_chromeos.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_chromeos.cc:51: : BluetoothDevice("", "", 0, false, false), Fix indentation. The : should line up with the B from the preceding line. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:124: std::string device_string = base::StringPrintf("|%s|%s|%u|%d|%d|", The first and last | characters aren’t necessary. Tiny data size savings. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:134: "|%s|%u|", The last | character isn’t necessary.
https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:174: // Remove devices that are no longer known by the system. On 2013/03/19 19:14:20, Mark Mentovai wrote: > On 2013/03/19 19:04:55, youngki wrote: > > On 2013/03/19 16:04:35, Mark Mentovai wrote: > > > How does something become unknown by the system? The documentation for > > > +[IOBluetoothDevice recentDevices:] says > > > > > > > The resulting array contains IOBluetoothDevice objects sorted in reverse > > > > chronological order. The most recently accessed devices are first. If the > > > > numDevices parameter is 0, all devices accessed by the system are > returned. > > > > If numDevices is non-zero, only the most recent devices are returned. > > > > > > All devices accessed by the system? Since the beginning of time? > > > > So.. the documentation didn't say that since when all devices accessed by the > > system. But I think I should follow the documentation and assume that the > > devices won't be unknown by the system as long as the chromium browser lives. > > Then the devices will never be unknown by the chromium browser once they are > > added. > > > > I removed the second part of this method implementation. > > I don’t know if it’s right to remove it. I wasn’t really expecting you to remove > it, I was just looking for an explanation of how it all works. You can look > beyond the documentation for answers, like by playing with the system from a > user’s perspective and trying to figure out the lifetime based on where it’s > storing things. > > For example, if you remove devices from the Bluetooth system preference pane, do > they disappear from the +recentDevices: list? I actually reviewed the documentations about BluetoothDevice::DeviceAdded(), BluetoothDevice::DeviceChanged(), and BluetoothDevice::DeviceRemoved(). And I should call those functions only for discovered devices and paired devices. So, instead of using recentDevices, I used pairedDevices to update the paired devices. (discovered devices will be implemented in later CLs). Please take a look. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:16: NSString* _name; On 2013/03/19 19:14:20, Mark Mentovai wrote: > This doesn’t follow the Chromium convention of using a trailing underscore. > > (Permission to use a leading underscore is new in the Objective-C guide, but > when working with existing projects like Chromium that have a long-established > tradition of using a trailing underscore, you should follow the local > convention.) Done. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:24: @property (copy) NSString* name; On 2013/03/19 19:14:20, Mark Mentovai wrote: > You don’t need either property to be atomic, right? You can declare them both > nonatomic. Putting nonatomic here throws an error: error: 'atomic' attribute on property 'name' does not match the property inherited from 'IOBluetoothDevice' This property in IOBluetoothDevice is defined as: @property(readonly, copy) NSString *name; I think I should keep this atomic since that's what the parent class defined it.. But I put nonatomic to address. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_chromeos.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_chromeos.cc:51: : BluetoothDevice("", "", 0, false, false), On 2013/03/19 19:14:20, Mark Mentovai wrote: > Fix indentation. The : should line up with the B from the preceding line. Done. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:124: std::string device_string = base::StringPrintf("|%s|%s|%u|%d|%d|", On 2013/03/19 19:14:20, Mark Mentovai wrote: > The first and last | characters aren’t necessary. Tiny data size savings. Done. https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_device_mac.mm:134: "|%s|%u|", On 2013/03/19 19:14:20, Mark Mentovai wrote: > The last | character isn’t necessary. Done.
https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:24: @property (copy) NSString* name; youngki wrote: > On 2013/03/19 19:14:20, Mark Mentovai wrote: > > You don’t need either property to be atomic, right? You can declare them both > > nonatomic. > > Putting nonatomic here throws an error: > > error: 'atomic' attribute on property 'name' does not match the property > inherited from 'IOBluetoothDevice' > > This property in IOBluetoothDevice is defined as: > > @property(readonly, copy) NSString *name; > > I think I should keep this atomic since that's what the parent class defined > it.. But I put nonatomic to address. Well, since you’re just a test class here, you could either - not inherit from IOBluetoothDevice (do you need to? maybe if you need some of IOBluetoothDevice’s functionality, you could have an IOBluetoothDevice instance variable) or - choose different names, like testName and testAddress. https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.h:71: void UpdateDevices(NSArray* devices); I like keeping the naming consistent. The new method uses the name paired_devices. Can you use the same name for this method too, here and in the implementation? https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.mm:182: for (IOBluetoothDevice* device in paired_devices) Use {braces} when you’re controlling something that takes more than one line. https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.mm:198: delete device_iter->second; What if IsPaired was false?
https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/9001/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_mac_unittest.mm:24: @property (copy) NSString* name; On 2013/03/20 19:01:16, Mark Mentovai wrote: > youngki wrote: > > On 2013/03/19 19:14:20, Mark Mentovai wrote: > > > You don’t need either property to be atomic, right? You can declare them > both > > > nonatomic. > > > > Putting nonatomic here throws an error: > > > > error: 'atomic' attribute on property 'name' does not match the property > > inherited from 'IOBluetoothDevice' > > > > This property in IOBluetoothDevice is defined as: > > > > @property(readonly, copy) NSString *name; > > > > I think I should keep this atomic since that's what the parent class defined > > it.. But I put nonatomic to address. > > Well, since you’re just a test class here, you could either > > - not inherit from IOBluetoothDevice (do you need to? maybe if you need some of > IOBluetoothDevice’s functionality, you could have an IOBluetoothDevice instance > variable) > > or > > - choose different names, like testName and testAddress. Done. This has to be IOBluetoothDevice instance, so I chose different names. https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_mac.h (right): https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.h:71: void UpdateDevices(NSArray* devices); On 2013/03/20 19:01:16, Mark Mentovai wrote: > I like keeping the naming consistent. The new method uses the name > paired_devices. Can you use the same name for this method too, here and in the > implementation? Actually I am going to use this method to add discovered devices into |devices| in the later CLs, so I think I should keep the name as devices. Speaking of that, I think AddDevices is probably a better name than UpdateDevices, so I just changed the method name; also I revised its comments to say that devices could be either discovered or paired devices. https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.mm:182: for (IOBluetoothDevice* device in paired_devices) On 2013/03/20 19:01:16, Mark Mentovai wrote: > Use {braces} when you’re controlling something that takes more than one line. Done. https://chromiumcodereview.appspot.com/12929003/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_mac.mm:198: delete device_iter->second; On 2013/03/20 19:01:16, Mark Mentovai wrote: > What if IsPaired was false? We do nothing. I was going to reuse BluetoothDeviceMac::UpdateDevice() when we discover devices that are not necessarily paired, so I did not want to assume here that |devices_| only contains paired devices. The method removes devices that used to be paired but are unpaired by the system. I just revised the comments in bluetooth_adapter_mac.h since the comment was a bit confusing..
OK. It’s hard to tell how things fit together if you have ideas in mind for future changes but they’re not reflected in the code yet. LGTM now.
Thanks for the review Mark. I will send out the other CL that uses AddDevices soon. Scott, could you review this CL for ownership?
mac tests are failing because some methods are specific to OSX 10.7 SDK. So I replicated some specific declarations. Please take a look.
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { What's the reason for adding these methods to the BluetoothDevice constructor? That applies to all implementations https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.h (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.h:247: I was planning to remove this method from Chrome OS (it's not relevant in LE) ... any reason for prompting it?
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { On 2013/03/21 18:03:34, keybuk wrote: > What's the reason for adding these methods to the BluetoothDevice constructor? > That applies to all implementations I think the member variables to BluetoothDevice should be initialized by BluetoothDevice itself and not the subclasses. So far BluetoothDeviceWin and BluetoothDeviceMac initialize the member variables in their constructors, so now I think I should have BluetoothDevice constructor to initialize them instead of having subclass constructors to do so. https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.h (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.h:247: On 2013/03/21 18:03:34, keybuk wrote: > I was planning to remove this method from Chrome OS (it's not relevant in LE) > ... any reason for prompting it? Okay I removed SetVisible. I wasn't aware of your plan to remove it. :) I thought since visible_ belongs to BluetoothDevice and all the subclasses are setting it, I thought SetVisible() should rather belong to this class instead.
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { On 2013/03/21 18:43:28, youngki wrote: > On 2013/03/21 18:03:34, keybuk wrote: > > What's the reason for adding these methods to the BluetoothDevice constructor? > > That applies to all implementations > > I think the member variables to BluetoothDevice should be initialized by > BluetoothDevice itself and not the subclasses. So far BluetoothDeviceWin and > BluetoothDeviceMac initialize the member variables in their constructors, so now > I think I should have BluetoothDevice constructor to initialize them instead of > having subclass constructors to do so. My concern is that these don't need to be member variables at all - a sub class should chose to implement this differently. In fact, the LE ChromeOS implementation just initializes BluetoothDeviceChromeOS using a BluetoothDeviceClient::Properties* pointer, which it stores in the class - and address() looks up data through that pointer, rather than using a member variable. https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.h (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.h:247: On 2013/03/21 18:43:28, youngki wrote: > On 2013/03/21 18:03:34, keybuk wrote: > > I was planning to remove this method from Chrome OS (it's not relevant in LE) > > ... any reason for prompting it? > > Okay I removed SetVisible. I wasn't aware of your plan to remove it. :) I > thought since visible_ belongs to BluetoothDevice and all the subclasses are > setting it, I thought SetVisible() should rather belong to this class instead. Basically Visible is meaningless ;) it's used in ChromeOS to avoid accidentally deleting paired devices found during discovery It's an API that shouldn't be part of BluetoothDevice at all - since to the UI it's just the opposite of "paired"
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { On 2013/03/21 18:46:25, keybuk wrote: > On 2013/03/21 18:43:28, youngki wrote: > > On 2013/03/21 18:03:34, keybuk wrote: > > > What's the reason for adding these methods to the BluetoothDevice > constructor? > > > That applies to all implementations > > > > I think the member variables to BluetoothDevice should be initialized by > > BluetoothDevice itself and not the subclasses. So far BluetoothDeviceWin and > > BluetoothDeviceMac initialize the member variables in their constructors, so > now > > I think I should have BluetoothDevice constructor to initialize them instead > of > > having subclass constructors to do so. > > My concern is that these don't need to be member variables at all - a sub class > should chose to implement this differently. > > In fact, the LE ChromeOS implementation just initializes BluetoothDeviceChromeOS > using a BluetoothDeviceClient::Properties* pointer, which it stores in the class > - and address() looks up data through that pointer, rather than using a member > variable. Currently BluetoothDevice::address() is returning const std::string&. Do you want this to be just const std::string instead of const std::string&? Also if I follow what you are planning to do, I should make the following methods pure virtual: BluetoothDevice::address() BluetoothDevice::GetName() BluetoothDevice::IsBonded() BluetoothDevice::IsConnected() Let me know if this is what you want. https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.h (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.h:247: On 2013/03/21 18:46:25, keybuk wrote: > On 2013/03/21 18:43:28, youngki wrote: > > On 2013/03/21 18:03:34, keybuk wrote: > > > I was planning to remove this method from Chrome OS (it's not relevant in > LE) > > > ... any reason for prompting it? > > > > Okay I removed SetVisible. I wasn't aware of your plan to remove it. :) I > > thought since visible_ belongs to BluetoothDevice and all the subclasses are > > setting it, I thought SetVisible() should rather belong to this class instead. > > Basically Visible is meaningless ;) it's used in ChromeOS to avoid accidentally > deleting paired devices found during discovery > > It's an API that shouldn't be part of BluetoothDevice at all - since to the UI > it's just the opposite of "paired" Agreed.
ping
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { It should probably just return std::string; and yes the others should be pure virtual in this function (I'm also ok with you not making that change btw, but if you're changing to pass in all the property values in the constructor, you just "owned" the change ;p)
https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... File device/bluetooth/bluetooth_device.cc (right): https://chromiumcodereview.appspot.com/12929003/diff/48001/device/bluetooth/b... device/bluetooth/bluetooth_device.cc:33: connecting_(false) { On 2013/03/25 16:49:47, keybuk wrote: > It should probably just return std::string; and yes the others should be pure > virtual in this function > > (I'm also ok with you not making that change btw, but if you're changing to pass > in all the property values in the constructor, you just "owned" the change ;p) So according to the new design doc it's likely we should change the interface of BluetoothAdapter and BluetoothDevice, so we probably have to change this file again. In this case I am more inclined to just submit this change as is now, then we can discuss the new interface of BluetoothDevice. What do you think? :)
Well you're changing the interface of BluetoothDevice here ;-) That was my point; I'm ok with you *not* changing the interface of BluetoothDevice - I just don't see the point of changing it now in an opposite direction to changes later
I’ll chime in by saying that it’s highly abnormal in Chrome code for an interface class to have protected data members that its subclasses are expected to use. Where you have an interface class, it’s typically only got pure virtual methods and no data of its own. For the use here, where the interface is really only used to dispatch to a per-OS implementation class, and there will never be more than one concerete implementation in use within a given process, it’s also acceptable (and in some cases preferable) to have a single class with different implementations on each platform. This means that the header needs to have #ifdefs for any OS-specific data or methods, but it’s generally the option with the smallest object code size and fastest run time. This change LGTM either way. I agree that if you want to refactor things in here to adapt them to a different model, it’s best addressed in a follow-up change.
I reverted BluetoothDevice interface change. Since our design doc implies we have to redesign this interface anyways, I don't think it's worth changing the interface now. Please take a look.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/12929003/66001
Retried try job too often on win_rel for step(s) printing_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/12929003/66001
Message was sent while issue was closed.
Change committed as 190560
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12929003/diff/66001/device/bluetooth/b... File device/bluetooth/bluetooth_device_mac.mm (right): https://chromiumcodereview.appspot.com/12929003/diff/66001/device/bluetooth/b... device/bluetooth/bluetooth_device_mac.mm:152: [[record attributes] count]); This breaks the 64-bit build. NSUInteger shouldn't be printf-ed. http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.8%20x64... Please in the future use %lu and cast to unsigned long. |
