Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
469613002:
Add bluetooth host and controller delegates for pairing. (Closed)
|
Created:
6 years, 4 months ago by Zachary Kuznia Modified:
6 years, 3 months ago Reviewers:
achuithb CC:
dzhioev (left Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd bluetooth host and controller delegates for pairing.
BUG=380961, 380962
Committed: https://crrev.com/de5908f77c572532a0d76a72b02c49a9165f582e
Cr-Commit-Position: refs/heads/master@{#292005}
Patch Set 1 #
Total comments: 90
Patch Set 2 : Code review fixes #
Total comments: 8
Patch Set 3 : Code review fixes #
Total comments: 2
Patch Set 4 : Fix nit #
Messages
Total messages: 12 (0 generated)
Please take a look.
First pass. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:31: } Is this expected to be created and used on the UI Thread? Maybe we should add DCHECKs, especially for the ctor/dtor and callbacks. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:158: ChangeStage(STAGE_DEVICE_NOT_FOUND); Shouldn't we be logging something here? Why STAGE_DEVICE_NOT_FOUND? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:164: Reset(); Is this the best thing to do? What happens if we reset? Does the system recover? Should we be retrying instead? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:169: if (device_->IsPaired()) { Could you add some comments here - why is connect error normal? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:184: // ControllerPairingController: Remove this comment https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:196: BluetoothControllerPairingController::GetCurrentStage() { This should be current_stage() and defined in the header https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:205: bool bluetooth_available = This variable seems unnecessary - why not inline it. If you want to keep it, it should be const. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:207: // TODO(zork): Add a stage for no bluetooth. Let's add bug-ids for all TODOs. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:226: CHECK(discovered_devices_.count(device_id)); Why CHECK? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:227: // TODO: Consider using stop instead of reset(). TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:248: CHECK(current_stage_ == STAGE_DEVICE_NOT_FOUND || Why CHECK and not DCHECK? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:255: std::string BluetoothControllerPairingController::GetConfirmationCode() { Return const std::string& https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:282: // TODO: Get proper credentials. TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:307: // ProtoDecoder::Observer: Remove this comment https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:312: if (message.parameters().domain() == kFakeEnrollmentDomain) { Where is this domain supposed to come from? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:315: // TODO: Get AddAnother from UI TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:327: &BluetoothControllerPairingController::OnErrorWithMessage, Should we be retrying? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:341: // TODO: Handling updating stages. TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:368: // device::BluetoothAdapter::Observer: REmove this comment https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:383: // device::BluetoothDevice::PairingDelegate: REmove this comment https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_controller_pairing_controller.h (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:102: base::WeakPtrFactory<BluetoothControllerPairingController> ptr_factory_; I believe this should be the last member of this list. You want the ptr factory to be destructed first. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:103: ObserverList<ControllerPairingController::Observer> observers_; I would also move this further down the list, maybe second last, since it's more of an implementation detail. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:109: device::BluetoothDevice* device_; What is the expected lifetime of device_? This naked pointer could be a source of crashes and security issues https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:45: // TODO: Get these values from the UI. TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:51: // TODO: Get a list of other paired controllers. TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:104: CHECK(!adapter_); Why CHECK? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:107: // TODO(zork): Make the device name prettier. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:177: // TODO: Update Host. TODO(zork) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:214: // TODO(zork): Add a stage for bluetooth error. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:220: // TODO(zork): Add a stage for bluetooth error. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:249: // TODO(zork): Add event to API to handle this case. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:261: // TODO: Enroll device, send error on error TODO(zork/achuith) + bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:267: // TODO: Check that domain matches. If not, send error and abort. TODO(zork/achuith) + bug-id Did we decide to do this on the server side instead? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:279: // TODO(zork): Handle adding another controller. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:288: // HostPairingFlow: REmove comment https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:297: HostPairingController::Stage BluetoothHostPairingController::GetCurrentStage() { This should be current_stage() and defined in the header. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:302: CHECK(current_stage_ == STAGE_NONE); Why CHECK? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:305: // TODO(zork): Add a stage for no bluetooth. bug-id https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:314: std::string BluetoothHostPairingController::GetDeviceName() { const std::string& device_name() const https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:318: std::string BluetoothHostPairingController::GetConfirmationCode() { const std::string& GetConfirmationCode() const https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:319: CHECK(current_stage_ == STAGE_WAITING_FOR_CODE_CONFIRMATION); Why CHECK? https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:323: std::string BluetoothHostPairingController::GetEnrollmentDomain() { const std::string& enrollment_domain() const https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_host_pairing_controller.h (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:60: // HostPairingFlow: HostPairingController https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:93: base::WeakPtrFactory<BluetoothHostPairingController> ptr_factory_; Move to the end. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:94: ObserverList<Observer> observers_; second from end
https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:31: } On 2014/08/20 21:52:32, achuithb wrote: > Is this expected to be created and used on the UI Thread? Maybe we should add > DCHECKs, especially for the ctor/dtor and callbacks. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:158: ChangeStage(STAGE_DEVICE_NOT_FOUND); On 2014/08/20 21:52:32, achuithb wrote: > Shouldn't we be logging something here? Why STAGE_DEVICE_NOT_FOUND? Added TODO and explanation. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:164: Reset(); On 2014/08/20 21:52:31, achuithb wrote: > Is this the best thing to do? What happens if we reset? Does the system recover? > Should we be retrying instead? Added TODO and explanation. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:169: if (device_->IsPaired()) { On 2014/08/20 21:52:32, achuithb wrote: > Could you add some comments here - why is connect error normal? Added comment. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:184: // ControllerPairingController: On 2014/08/20 21:52:32, achuithb wrote: > Remove this comment Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:196: BluetoothControllerPairingController::GetCurrentStage() { On 2014/08/20 21:52:31, achuithb wrote: > This should be current_stage() and defined in the header This is virtual, as discussed. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:205: bool bluetooth_available = On 2014/08/20 21:52:32, achuithb wrote: > This variable seems unnecessary - why not inline it. > > If you want to keep it, it should be const. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:207: // TODO(zork): Add a stage for no bluetooth. On 2014/08/20 21:52:32, achuithb wrote: > Let's add bug-ids for all TODOs. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:226: CHECK(discovered_devices_.count(device_id)); On 2014/08/20 21:52:31, achuithb wrote: > Why CHECK? DCHECK now. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:227: // TODO: Consider using stop instead of reset(). On 2014/08/20 21:52:33, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:248: CHECK(current_stage_ == STAGE_DEVICE_NOT_FOUND || On 2014/08/20 21:52:33, achuithb wrote: > Why CHECK and not DCHECK? Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:255: std::string BluetoothControllerPairingController::GetConfirmationCode() { On 2014/08/20 21:52:32, achuithb wrote: > Return const std::string& This is virtual, as discussed. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:282: // TODO: Get proper credentials. On 2014/08/20 21:52:33, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:307: // ProtoDecoder::Observer: On 2014/08/20 21:52:32, achuithb wrote: > Remove this comment Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:312: if (message.parameters().domain() == kFakeEnrollmentDomain) { On 2014/08/20 21:52:31, achuithb wrote: > Where is this domain supposed to come from? Added a comment to explain. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:315: // TODO: Get AddAnother from UI On 2014/08/20 21:52:32, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:327: &BluetoothControllerPairingController::OnErrorWithMessage, On 2014/08/20 21:52:31, achuithb wrote: > Should we be retrying? No; if send fails, we'll have to restart the whole communication process, and it's safest to restart the pairing. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:341: // TODO: Handling updating stages. On 2014/08/20 21:52:32, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:368: // device::BluetoothAdapter::Observer: On 2014/08/20 21:52:32, achuithb wrote: > REmove this comment Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.cc:383: // device::BluetoothDevice::PairingDelegate: On 2014/08/20 21:52:32, achuithb wrote: > REmove this comment Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_controller_pairing_controller.h (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:102: base::WeakPtrFactory<BluetoothControllerPairingController> ptr_factory_; On 2014/08/20 21:52:33, achuithb wrote: > I believe this should be the last member of this list. You want the ptr factory > to be destructed first. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:103: ObserverList<ControllerPairingController::Observer> observers_; On 2014/08/20 21:52:33, achuithb wrote: > I would also move this further down the list, maybe second last, since it's more > of an implementation detail. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_controller_pairing_controller.h:109: device::BluetoothDevice* device_; On 2014/08/20 21:52:33, achuithb wrote: > What is the expected lifetime of device_? This naked pointer could be a source > of crashes and security issues Storing the device id instead of the device pointer now. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:45: // TODO: Get these values from the UI. On 2014/08/20 21:52:33, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:51: // TODO: Get a list of other paired controllers. On 2014/08/20 21:52:34, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:104: CHECK(!adapter_); On 2014/08/20 21:52:34, achuithb wrote: > Why CHECK? Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:107: // TODO(zork): Make the device name prettier. On 2014/08/20 21:52:33, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:177: // TODO: Update Host. On 2014/08/20 21:52:34, achuithb wrote: > TODO(zork) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:214: // TODO(zork): Add a stage for bluetooth error. On 2014/08/20 21:52:34, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:220: // TODO(zork): Add a stage for bluetooth error. On 2014/08/20 21:52:33, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:249: // TODO(zork): Add event to API to handle this case. On 2014/08/20 21:52:33, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:261: // TODO: Enroll device, send error on error On 2014/08/20 21:52:33, achuithb wrote: > TODO(zork/achuith) + bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:267: // TODO: Check that domain matches. If not, send error and abort. On 2014/08/20 21:52:33, achuithb wrote: > TODO(zork/achuith) + bug-id > > Did we decide to do this on the server side instead? Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:279: // TODO(zork): Handle adding another controller. On 2014/08/20 21:52:34, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:288: // HostPairingFlow: On 2014/08/20 21:52:33, achuithb wrote: > REmove comment Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:297: HostPairingController::Stage BluetoothHostPairingController::GetCurrentStage() { On 2014/08/20 21:52:34, achuithb wrote: > This should be current_stage() and defined in the header. Acknowledged. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:302: CHECK(current_stage_ == STAGE_NONE); On 2014/08/20 21:52:34, achuithb wrote: > Why CHECK? Acknowledged. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:305: // TODO(zork): Add a stage for no bluetooth. On 2014/08/20 21:52:33, achuithb wrote: > bug-id Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:314: std::string BluetoothHostPairingController::GetDeviceName() { On 2014/08/20 21:52:33, achuithb wrote: > const std::string& device_name() const Acknowledged. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:318: std::string BluetoothHostPairingController::GetConfirmationCode() { On 2014/08/20 21:52:34, achuithb wrote: > const std::string& GetConfirmationCode() const Acknowledged. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:319: CHECK(current_stage_ == STAGE_WAITING_FOR_CODE_CONFIRMATION); On 2014/08/20 21:52:34, achuithb wrote: > Why CHECK? Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.cc:323: std::string BluetoothHostPairingController::GetEnrollmentDomain() { On 2014/08/20 21:52:34, achuithb wrote: > const std::string& enrollment_domain() const This is a virtual function, as we discussed. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... File components/pairing/bluetooth_host_pairing_controller.h (right): https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:60: // HostPairingFlow: On 2014/08/20 21:52:34, achuithb wrote: > HostPairingController Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:93: base::WeakPtrFactory<BluetoothHostPairingController> ptr_factory_; On 2014/08/20 21:52:34, achuithb wrote: > Move to the end. Done. https://codereview.chromium.org/469613002/diff/1/components/pairing/bluetooth... components/pairing/bluetooth_host_pairing_controller.h:94: ObserverList<Observer> observers_; On 2014/08/20 21:52:34, achuithb wrote: > second from end Done.
https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:46: controller_device_id_ = ""; nit: I prefer clear() to this assignment. https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:128: device::BluetoothDevice* device = adapter_->GetDevice(controller_device_id_); You're using this pattern often enough that you should probably write a helper function to do this, like device::BluetoothDevice* BluetoothControllerPairingController::GetDevice() { device::BluetoothDevice* device = adapter_->GetDevice(controller_device_id_); if (!device) ChangeStage(STAGE_ESTABLISHING_CONNECTION_ERROR); return device; } And the calling sites can do a NULL check https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:130: ChangeStage(STAGE_ESTABLISHING_CONNECTION_ERROR); Should this log an error? https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:299: controller_device_id_ = ""; nit: clear()
https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:46: controller_device_id_ = ""; On 2014/08/21 07:08:58, achuithb wrote: > nit: I prefer clear() to this assignment. Done. https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:128: device::BluetoothDevice* device = adapter_->GetDevice(controller_device_id_); On 2014/08/21 07:08:58, achuithb wrote: > You're using this pattern often enough that you should probably write a helper > function to do this, like > device::BluetoothDevice* BluetoothControllerPairingController::GetDevice() { > device::BluetoothDevice* device = adapter_->GetDevice(controller_device_id_); > if (!device) > ChangeStage(STAGE_ESTABLISHING_CONNECTION_ERROR); > return device; > } > And the calling sites can do a NULL check Done. https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:130: ChangeStage(STAGE_ESTABLISHING_CONNECTION_ERROR); On 2014/08/21 07:08:58, achuithb wrote: > Should this log an error? Put the LOG in the shared function. https://codereview.chromium.org/469613002/diff/20001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:299: controller_device_id_ = ""; On 2014/08/21 07:08:58, achuithb wrote: > nit: clear() Done.
lgtm https://codereview.chromium.org/469613002/diff/40001/components/pairing/bluet... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/40001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:140: if (!device) Nit: Might just read better to do if (device) { device->ConnectToService( } instead of the early return.
https://codereview.chromium.org/469613002/diff/40001/components/pairing/bluet... File components/pairing/bluetooth_controller_pairing_controller.cc (right): https://codereview.chromium.org/469613002/diff/40001/components/pairing/bluet... components/pairing/bluetooth_controller_pairing_controller.cc:140: if (!device) On 2014/08/26 19:41:06, achuithb wrote: > Nit: > Might just read better to do > if (device) { > device->ConnectToService( > } > > instead of the early return. Done.
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/469613002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #4 (60001) as a7d3efc8ed47a892ed2b76951b42b5c9d8788c67
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/de5908f77c572532a0d76a72b02c49a9165f582e Cr-Commit-Position: refs/heads/master@{#292005} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
