Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
252503002:
Base version of HID detection OOBE screen. (Closed)
|
Created:
6 years, 8 months ago by merkulova Modified:
6 years, 7 months ago CC:
chromium-reviews, arv+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBase version of HID detection OOBE screen.
Random pics used as icons as icons are not ready.
USB mouse/keyboard detection enabled.
Bluetooth mouse detection enabled.
Basic support of pincode keyboards added.
Bluetooth keyboard detection with passphrase was not checked.
Layout should be polished.
Not all device-types are supported (e.g. joystick or keyboard_mouse_combo class).
Screen is put behind the flag enable-hid-detection-on-oobe.
BUG=127016
TBR=oshima@chromium.org
for chromeos_switches
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268185
Patch Set 1 #
Total comments: 76
Patch Set 2 : Crash fixed. Logic for Bluetooth switching applied. #
Total comments: 4
Patch Set 3 : First bunch of fixes. #Patch Set 4 : Fixes #Patch Set 5 : Proper initialization and setPower of BT adapter. #
Total comments: 11
Patch Set 6 : Some unused code removed. Basic support of pincode keyboards added. #
Total comments: 24
Patch Set 7 : Logic bug-fix. Progress dots hidden. Style improved. BT names support. #
Total comments: 20
Patch Set 8 : Flag logic switched to disabled-by-default. #Patch Set 9 : Passkey support added. Improved BT-connection logic. #
Total comments: 24
Patch Set 10 : Unused code removed. Style fixes. #
Total comments: 31
Patch Set 11 : KeysEntered support added. Style fixes. #
Total comments: 4
Patch Set 12 : More device types added. Passkey conversion to string set properly. #Patch Set 13 : Unused variabled removed. #Messages
Total messages: 33 (0 generated)
+Nikita
First bunch of comments. https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/hid_detection_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Add some tests or remove this file from the current CL. https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:14: #include "ash/shell.h" nit: not needed here. https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.h:268: bool CanShowHIDDetectionScreen(); Move this method to the anonymous namespace in the corresponding source file. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css:83: /* searching pairing paired connected*/ What's the purpose of this comment? https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:9: alt=""> Why do you need alt property here (and for all images in the file)? https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: DISMISSED: 'bluetoothPairingDismissed', // pairing dismissed(succeeded or nit: move the comment between lines #30 and #31. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:72: * @param {state} one of 'searching', 'connected', 'pairing', 'paired'. Add tag @private. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:87: if ('searching' in data) { No need in curly braces here, as all branches are one-liners. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:11: #include "chrome/browser/chromeos/device/input_service_proxy.h" Not needed here. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:13: #include "device/bluetooth/bluetooth_adapter.h" Not needed here. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:15: #include "device/bluetooth/bluetooth_device.h" Not needed here. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: adapter_(NULL), scoped_refptr is initialized to nullptr automatically. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:61: pairing_device_passkey_(1000000), Where this field is used? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:62: pairing_device_entered_(kInvalidEntered), Where this field is used? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:203: VLOG(1) << "DisplayPassKey id = " << device->GetDeviceID() << " name = " << Add "// TODO" comment + NOTIMPLEMENTED() to all methods which are not currently implemented. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:288: void HIDDetectionScreenHandler::UpdateDevices(bool skipScreenIfDevicesPresent) { Use unix_hacker_style instead of camelCaseStyle for parameter name. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:302: it++) { nit: use ++it when |it| is an iterator. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:343: nit: delete blank line. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:380: web_ui()->CallJavascriptFunction( Use CallJS when it's possible. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:7: #include <string> https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:10: #include "base/callback.h" Where this header is used in this file? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:14: #include "base/strings/string16.h" Where this header is used in this file? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:14: #include "base/strings/string16.h" Where this header is used in this file? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:40: HIDDetectionScreenHandler(); nit: add a black line after the typedef. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:58: // This method will be called when the Bluetooth daemon requires a Don't copy comments from the interface methods, because when interface will be changed, your comments become obsolete. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:247: void UpdateDevices(bool skipScreenIfDevicesPresent = false); We don't use default arguments in Chrome code: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:284: // Weak pointer factory for generating 'this' pointers that might live longer nit: weak factories are very common, so comment is not needed here. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:135: const char OobeUI::kScreenOobeHIDDetection[] nit: incorrect indent here. https://codereview.chromium.org/252503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: setPointingDeviceState: function(data) { Why not to pass a string instead of dictionary? You will be able to call this.setDeviceBlockState_('hid-mouse-block', state); instead of if-else chain. The same question for setKeyboardDeviceState.
https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:46: i18n-content="hidDetectionBTEnterKey" nit: 4 spaces indent for same element (through whole html file). https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: DISMISSED: 'bluetoothPairingDismissed', // pairing dismissed(succeeded or nit: Capitalize comment. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:66: // device::BluetoothDevice::PairingDelegate override. Please remove comments for these methods (overrides). One can always check comments in device/bluetooth/bluetooth_device.h https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:247: void UpdateDevices(bool skipScreenIfDevicesPresent = false); nit: Comment is missing.
https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:96: UpdateDevices(/*true*/false /*skip screen if devices present*/); Debug code? https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:181: VLOG(1) << "RequestPinCode id = " << device->GetDeviceID() << " name = " << nit: More readable: VLOG(1) << "RequestPinCode id = " << device->GetDeviceID() << " name = " << device->GetName(); Same for other logging code in this file. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:196: params.SetString("pairing", kRemotePinCode); nit: Extract string constants into unnamed namespace. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:298: nit: Drop empty line. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:316: if (adapter_ && nit: Add short comment for this block. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:387: base::DictionaryValue js_properties; nit: Use more descriptive name like |state| instead of generic |js_properties| https://codereview.chromium.org/252503002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:87: VLOG(1) << "Failed to power BT adapter"; LOG(ERROR)
https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/hid_detection_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/04/24 11:30:24, ygorshenin1 wrote: > Add some tests or remove this file from the current CL. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:14: #include "ash/shell.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: not needed here. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.h:268: bool CanShowHIDDetectionScreen(); On 2014/04/24 11:30:24, ygorshenin1 wrote: > Move this method to the anonymous namespace in the corresponding source file. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css:83: /* searching pairing paired connected*/ On 2014/04/24 11:30:24, ygorshenin1 wrote: > What's the purpose of this comment? Left from logging. Removed. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:9: alt=""> On 2014/04/24 11:30:24, ygorshenin1 wrote: > Why do you need alt property here (and for all images in the file)? http://dev.chromium.org/developers/accessibility/html-accessibility 'All <img> tags must have an alt attribute with an accessible name. If an image is purely decorative or if the accessible name is already present in an adjacent element in the DOM, you can use alt="".' Accessible names will be added on accessibility-changes stage. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:46: i18n-content="hidDetectionBTEnterKey" On 2014/04/24 12:14:23, Nikita Kostylev wrote: > nit: 4 spaces indent for same element (through whole html file). Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: DISMISSED: 'bluetoothPairingDismissed', // pairing dismissed(succeeded or On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: move the comment between lines #30 and #31. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: DISMISSED: 'bluetoothPairingDismissed', // pairing dismissed(succeeded or On 2014/04/24 12:14:23, Nikita Kostylev wrote: > nit: Capitalize comment. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:72: * @param {state} one of 'searching', 'connected', 'pairing', 'paired'. On 2014/04/24 11:30:24, ygorshenin1 wrote: > Add tag @private. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:87: if ('searching' in data) { On 2014/04/24 11:30:24, ygorshenin1 wrote: > No need in curly braces here, as all branches are one-liners. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:11: #include "chrome/browser/chromeos/device/input_service_proxy.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Not needed here. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:13: #include "device/bluetooth/bluetooth_adapter.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Not needed here. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:15: #include "device/bluetooth/bluetooth_device.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Not needed here. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: adapter_(NULL), On 2014/04/24 11:30:24, ygorshenin1 wrote: > scoped_refptr is initialized to nullptr automatically. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:61: pairing_device_passkey_(1000000), On 2014/04/24 11:30:24, ygorshenin1 wrote: > Where this field is used? Removed https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:62: pairing_device_entered_(kInvalidEntered), On 2014/04/24 11:30:24, ygorshenin1 wrote: > Where this field is used? Removed https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:96: UpdateDevices(/*true*/false /*skip screen if devices present*/); On 2014/04/24 12:22:45, Nikita Kostylev wrote: > Debug code? Removed https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:181: VLOG(1) << "RequestPinCode id = " << device->GetDeviceID() << " name = " << On 2014/04/24 12:22:45, Nikita Kostylev wrote: > nit: More readable: > > VLOG(1) << "RequestPinCode id = " << device->GetDeviceID() > << " name = " << device->GetName(); > > Same for other logging code in this file. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:196: params.SetString("pairing", kRemotePinCode); On 2014/04/24 12:22:45, Nikita Kostylev wrote: > nit: Extract string constants into unnamed namespace. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:203: VLOG(1) << "DisplayPassKey id = " << device->GetDeviceID() << " name = " << On 2014/04/24 11:30:24, ygorshenin1 wrote: > Add "// TODO" comment + NOTIMPLEMENTED() to all methods which are not currently > implemented. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:288: void HIDDetectionScreenHandler::UpdateDevices(bool skipScreenIfDevicesPresent) { On 2014/04/24 11:30:24, ygorshenin1 wrote: > Use unix_hacker_style instead of camelCaseStyle for parameter name. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:298: On 2014/04/24 12:22:45, Nikita Kostylev wrote: > nit: Drop empty line. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:302: it++) { On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: use ++it when |it| is an iterator. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:316: if (adapter_ && On 2014/04/24 12:22:45, Nikita Kostylev wrote: > nit: Add short comment for this block. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:343: On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: delete blank line. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:380: web_ui()->CallJavascriptFunction( On 2014/04/24 11:30:24, ygorshenin1 wrote: > Use CallJS when it's possible. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:387: base::DictionaryValue js_properties; On 2014/04/24 12:22:45, Nikita Kostylev wrote: > nit: Use more descriptive name like |state| instead of generic |js_properties| Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:7: On 2014/04/24 11:30:24, ygorshenin1 wrote: > #include <string> Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:10: #include "base/callback.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Where this header is used in this file? Removed https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:14: #include "base/strings/string16.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Where this header is used in this file? Removed https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:14: #include "base/strings/string16.h" On 2014/04/24 11:30:24, ygorshenin1 wrote: > Where this header is used in this file? Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:40: HIDDetectionScreenHandler(); On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: add a black line after the typedef. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:58: // This method will be called when the Bluetooth daemon requires a On 2014/04/24 11:30:24, ygorshenin1 wrote: > Don't copy comments from the interface methods, because when interface will be > changed, your comments become obsolete. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:66: // device::BluetoothDevice::PairingDelegate override. On 2014/04/24 12:14:23, Nikita Kostylev wrote: > Please remove comments for these methods (overrides). One can always check > comments in device/bluetooth/bluetooth_device.h Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:247: void UpdateDevices(bool skipScreenIfDevicesPresent = false); On 2014/04/24 11:30:24, ygorshenin1 wrote: > We don't use default arguments in Chrome code: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:247: void UpdateDevices(bool skipScreenIfDevicesPresent = false); On 2014/04/24 12:14:23, Nikita Kostylev wrote: > nit: Comment is missing. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:284: // Weak pointer factory for generating 'this' pointers that might live longer On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: weak factories are very common, so comment is not needed here. Done. https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/252503002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:135: const char OobeUI::kScreenOobeHIDDetection[] On 2014/04/24 11:30:24, ygorshenin1 wrote: > nit: incorrect indent here. Done. https://codereview.chromium.org/252503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: setPointingDeviceState: function(data) { On 2014/04/24 11:30:24, ygorshenin1 wrote: > Why not to pass a string instead of dictionary? You will be able to call > this.setDeviceBlockState_('hid-mouse-block', state); instead of if-else chain. > The same question for setKeyboardDeviceState. I agree on this one method. For aetKeyboardDeviceState we should also pass pincode/passkey and label value (generated from name of device), so there we definitely need a dictionary. https://codereview.chromium.org/252503002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:87: VLOG(1) << "Failed to power BT adapter"; On 2014/04/24 12:22:46, Nikita Kostylev wrote: > LOG(ERROR) Done.
https://codereview.chromium.org/252503002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:92: chromeos::switches::kDisableHIDDetectionOnOOBE); You wrote in description that several features are not working yet. Then why we enable this screen by default? https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/base_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/base_screen_handler.cc:92: LOG(ERROR) << FullMethodPath(method); Remove this. https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:36: // |SendDeviceNotification| may include a pairing parameter whose value What is |SendDeviceNotification|? https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:57: Lines 21-57 are copy-pasted from bluetooth_options_handler.cc . Why? https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:73: void ReportError(const std::string& error, const std::string& address); Please, do not mix overridden methods with own methods in definition. First list all own methods, then overridden methods. https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:112: Do we really need all these methods (lines 56-112) to be public?
https://codereview.chromium.org/252503002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:92: chromeos::switches::kDisableHIDDetectionOnOOBE); On 2014/04/25 10:59:59, dzhioev wrote: > You wrote in description that several features are not working yet. Then why we > enable this screen by default? Current state covers most of use-cases, widening possibilities of workflow: allowing BT mouse. Also it notifies user on device needs. It doesn't spoil any current workflow. https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/base_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/base_screen_handler.cc:92: LOG(ERROR) << FullMethodPath(method); On 2014/04/25 10:59:59, dzhioev wrote: > Remove this. Done. https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:36: // |SendDeviceNotification| may include a pairing parameter whose value On 2014/04/25 10:59:59, dzhioev wrote: > What is |SendDeviceNotification|? Done. https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:57: On 2014/04/25 10:59:59, dzhioev wrote: > Lines 21-57 are copy-pasted from bluetooth_options_handler.cc . Why? https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:73: void ReportError(const std::string& error, const std::string& address); On 2014/04/25 10:59:59, dzhioev wrote: > Please, do not mix overridden methods with own methods in definition. First list > all own methods, then overridden methods. Done.
https://codereview.chromium.org/252503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:787: if (CanShowHIDDetectionScreen()) { nit: curly braces are not needed here. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:23: STARTUP: 'bluetoothStartConnecting', nit: 2 spaces indent. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: // Pairing dismissed(succeeded or canceled). nit: a space before (. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:94: Add a comment to setKeyboardDeviceState(). https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:95: setKeyboardDeviceState: function(data) { After discussion: could you please add a field 'state' to the data, which will be equal to the one of the following strings: 'searching', 'connected', 'paired', 'pairing'? https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:109: // remove pincode pictures if exist. nit: when your comment is a complete expression (with dot at the end) first word should be capitalized. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:113: } nit: curly braces are not needed here. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:137: // One device can be in the process of pairing. If found, display Incorrect indentation. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:151: nit: delete a blank line. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:61: UpdateDevices(false /*skip screen if devices present*/); nit: add a whitespace before 'skip' and a whitespace after 'present'. The same for other comments to the argument to UpdateDevices() in this file. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:387: if (pointing_device_id_.empty()) { nit: curly braces are not needed here. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:187: void ConnectBTDevice(device::BluetoothDevice* device); Add a comment to this method.
What about oobe screen progress dot? I think it should not be shown for this screen since it is optional.
Progress dots removed, Yuri's notes applied. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:787: if (CanShowHIDDetectionScreen()) { On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: curly braces are not needed here. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:23: STARTUP: 'bluetoothStartConnecting', On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: 2 spaces indent. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:31: // Pairing dismissed(succeeded or canceled). On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: a space before (. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:94: On 2014/04/25 15:05:08, ygorshenin1 wrote: > Add a comment to setKeyboardDeviceState(). Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:95: setKeyboardDeviceState: function(data) { On 2014/04/25 15:05:08, ygorshenin1 wrote: > After discussion: could you please add a field 'state' to the data, which will > be equal to the one of the following strings: 'searching', 'connected', > 'paired', 'pairing'? Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:109: // remove pincode pictures if exist. On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: when your comment is a complete expression (with dot at the end) first word > should be capitalized. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:113: } On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: curly braces are not needed here. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:137: // One device can be in the process of pairing. If found, display On 2014/04/25 15:05:08, ygorshenin1 wrote: > Incorrect indentation. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:151: On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: delete a blank line. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:61: UpdateDevices(false /*skip screen if devices present*/); On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: add a whitespace before 'skip' and a whitespace after 'present'. The same > for other comments to the argument to UpdateDevices() in this file. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:387: if (pointing_device_id_.empty()) { On 2014/04/25 15:05:08, ygorshenin1 wrote: > nit: curly braces are not needed here. Done. https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:187: void ConnectBTDevice(device::BluetoothDevice* device); On 2014/04/25 15:05:08, ygorshenin1 wrote: > Add a comment to this method. Done.
lgtm https://codereview.chromium.org/252503002/diff/140001/chromeos/chromeos_switc... File chromeos/chromeos_switches.h (right): https://codereview.chromium.org/252503002/diff/140001/chromeos/chromeos_switc... chromeos/chromeos_switches.h:32: CHROMEOS_EXPORT extern const char kDisableHIDDetectionOnOOBE[]; I suggest you to replace "disable-*" flag by "enable-*" flag, so the feature will be turned off by default. You can enable it by default after the branch point.
https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:158: NOTIMPLEMENTED(); device->CancelPairing() is acceptable here https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:165: NOTIMPLEMENTED(); device->CancelPairing() is acceptable here https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:184: NOTIMPLEMENTED(); This should be implemented, it'll be used for HID over GATT keyboards and Bluetooth 2.1+ keyboards https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:190: NOTIMPLEMENTED(); Useful to do the pairing UI property https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:196: NOTIMPLEMENTED(); device->CancelPairing() is acceptable here https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:231: } should also handle DEVICE_KEYBOARD_MOUSE_COMBO https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:238: NOTIMPLEMENTED(); The property that changed might be the class, so you should have the same logic as DeviceAdded here https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:245: NOTIMPLEMENTED(); Probably no need to implement, the method is optional https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:313: } Also handle DEVICE_KEYBOARD_MOUSE_COMBO
https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:158: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > device->CancelPairing() is acceptable here Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:165: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > device->CancelPairing() is acceptable here Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:184: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > This should be implemented, it'll be used for HID over GATT keyboards and > Bluetooth 2.1+ keyboards Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:190: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > Useful to do the pairing UI property What do you mean? https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:196: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > device->CancelPairing() is acceptable here Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:231: } On 2014/04/27 10:11:55, keybuk wrote: > should also handle DEVICE_KEYBOARD_MOUSE_COMBO Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:238: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > The property that changed might be the class, so you should have the same logic > as DeviceAdded here Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:245: NOTIMPLEMENTED(); On 2014/04/27 10:11:55, keybuk wrote: > Probably no need to implement, the method is optional Done. https://codereview.chromium.org/252503002/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:313: } On 2014/04/27 10:11:55, keybuk wrote: > Also handle DEVICE_KEYBOARD_MOUSE_COMBO Done. https://codereview.chromium.org/252503002/diff/140001/chromeos/chromeos_switc... File chromeos/chromeos_switches.h (right): https://codereview.chromium.org/252503002/diff/140001/chromeos/chromeos_switc... chromeos/chromeos_switches.h:32: CHROMEOS_EXPORT extern const char kDisableHIDDetectionOnOOBE[]; On 2014/04/25 16:42:03, ygorshenin1 wrote: > I suggest you to replace "disable-*" flag by "enable-*" flag, so the feature > will be turned off by default. You can enable it by default after the branch > point. Done.
https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:34: id="hid-keyboard-label-searching" class="label"> nit: Place id attribute first (for all elements through this file). https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... File chrome/browser/resources/login/screen_container.css (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... chrome/browser/resources/login/screen_container.css:165: #oobe.hid-detection #progress-dots { nit: I think this is probably not what we since dots element will be displayed on the next out-of-box step. They way it is implemented right now is that it results in screen layout "jump" because of dots element got added to the layout. I suggest keep showing progress-dots element (for the whole oobe) but don't add a separate dot to it.
https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:122: void EnableChangeError(); This method has no implementation in *.cc files. Same true DeviceConnecting and maybe for some other methods.
https://codereview.chromium.org/252503002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:467: SetStatusAreaVisible(false); SetStatusAreaVisible(true); Since we're now showing status area (and control bar) through out-of-box. Btw, this means that users will be able to handle those edge cases like turn bluetooth off and on. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:22: const char kRemotePinCode[] = "bluetoothRemotePinCode"; nit: Comment is missing. What are these constants for? https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:45: weak_ptr_factory_(this) { pairing_device_passkey_ pairing_device_entered_ should_run_device_discovery_ pointing_device_connect_type_ keyboard_device_connect_type_ should be initialized here. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:46: pointing_device_id_.clear(); nit: Remove strings 46..47. std::string is initialized properly to an empty one. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:58: void HIDDetectionScreenHandler::PrepareToShow() {} nit: Is this default clang formatting? No particular preference but we tend to put closing bracket at separate line in .cc files (even if body is empty). https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:68: LOG(ERROR) << "Failed to power BT adapter"; I wonder if we need a user facing error here for instance as a notification. User may be expecting bluetooth to just work while we have issues enabling adapter. How is this handled inside session? Please add as an open question to PRD. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:72: VLOG(1) << "Failed to start Bluetooth discovery."; nit: Same question here. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:277: // TODO(merkulova): deal with all available device types. nit: mentions something in comments like joystick. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:22: nit: drop empty line.
https://codereview.chromium.org/252503002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:467: SetStatusAreaVisible(false); On 2014/04/28 13:51:57, Nikita Kostylev wrote: > SetStatusAreaVisible(true); > > Since we're now showing status area (and control bar) through out-of-box. > > Btw, this means that users will be able to handle those edge cases like turn > bluetooth off and on. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:34: id="hid-keyboard-label-searching" class="label"> On 2014/04/28 08:30:28, Nikita Kostylev wrote: > nit: Place id attribute first (for all elements through this file). Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... File chrome/browser/resources/login/screen_container.css (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/resource... chrome/browser/resources/login/screen_container.css:165: #oobe.hid-detection #progress-dots { On 2014/04/28 08:30:28, Nikita Kostylev wrote: > nit: I think this is probably not what we since dots element will be displayed > on the next out-of-box step. > They way it is implemented right now is that it results in screen layout "jump" > because of dots element got added to the layout. > > I suggest keep showing progress-dots element (for the whole oobe) but don't add > a separate dot to it. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:22: const char kRemotePinCode[] = "bluetoothRemotePinCode"; On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: Comment is missing. What are these constants for? Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:45: weak_ptr_factory_(this) { On 2014/04/28 13:51:57, Nikita Kostylev wrote: > pairing_device_passkey_ > pairing_device_entered_ > > should_run_device_discovery_ > > pointing_device_connect_type_ > keyboard_device_connect_type_ > > should be initialized here. Removed as unused. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:46: pointing_device_id_.clear(); On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: Remove strings 46..47. std::string is initialized properly to an empty one. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:58: void HIDDetectionScreenHandler::PrepareToShow() {} On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: Is this default clang formatting? > No particular preference but we tend to put closing bracket at separate line in > .cc files (even if body is empty). Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:68: LOG(ERROR) << "Failed to power BT adapter"; On 2014/04/28 13:51:57, Nikita Kostylev wrote: > I wonder if we need a user facing error here for instance as a notification. > User may be expecting bluetooth to just work while we have issues enabling > adapter. > > How is this handled inside session? > > Please add as an open question to PRD. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:72: VLOG(1) << "Failed to start Bluetooth discovery."; On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: Same question here. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:277: // TODO(merkulova): deal with all available device types. On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: mentions something in comments like joystick. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h (right): https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:22: On 2014/04/28 13:51:57, Nikita Kostylev wrote: > nit: drop empty line. Done. https://codereview.chromium.org/252503002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:122: void EnableChangeError(); On 2014/04/28 12:54:22, dzhioev wrote: > This method has no implementation in *.cc files. Same true DeviceConnecting and > maybe for some other methods. Done.
lgtm with nits https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:156: nit: drop empty line. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:201: NOTIMPLEMENTED(); nit: TODO? https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:207: device->CancelPairing(); Why cancel pairing when passkey is confirmed? https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:231: void HIDDetectionScreenHandler::PairAsPointingDevice( nit: Rename to TryPairingAsPointingDevice https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:234: (device->GetDeviceType() == device::BluetoothDevice::DEVICE_MOUSE || nit: Extract this code into helper function in unnamed namespace device->GetDeviceType() == device::BluetoothDevice::DEVICE_MOUSE || device->GetDeviceType() == device::BluetoothDevice::DEVICE_KEYBOARD_MOUSE_COMBO https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:239: } /*else if (device->GetDeviceId() == pointing_device_id_ && )*/ Debug code? https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:242: void HIDDetectionScreenHandler::PairAsKeyboardDevice( nit: rename to TryPairing... https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:245: (device->GetDeviceType() == device::BluetoothDevice::DEVICE_KEYBOARD || nit: Same here, please extract helper method. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:273: NOTIMPLEMENTED(); I think you should just update UI state here (in case input device is removed). It seems UI update is already handled in OnInputDeviceRemoved() so you may as well just remove NOTIMPLEMENTED(). https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:317: base::Unretained(this), nit: Align parameters base::Bind() Suggestions: you may as well run git cl format which runs clang-format for your changes only. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:324: // If no connected devices found as pointing device and keyboard, we try to nit: Insert empty line before comment. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:385: device::BluetoothDevice::DeviceType device_type = device->GetDeviceType(); nit: insert empty line. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:411: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) nit: Add {} for all blocks. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:426: LOG(WARNING) << "BTConnectError"; nit: Add extra info such as address, device_type and error_code. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:427: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) nit: Add {} for all blocks.
https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:156: On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: drop empty line. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:201: NOTIMPLEMENTED(); On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: TODO? Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:207: device->CancelPairing(); On 2014/04/29 10:59:37, Nikita Kostylev wrote: > Why cancel pairing when passkey is confirmed? It's confirmation of user-set pincode. We don't support this functionality on discussion with @keybuk. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:231: void HIDDetectionScreenHandler::PairAsPointingDevice( On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Rename to TryPairingAsPointingDevice Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:234: (device->GetDeviceType() == device::BluetoothDevice::DEVICE_MOUSE || On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Extract this code into helper function in unnamed namespace > > device->GetDeviceType() == device::BluetoothDevice::DEVICE_MOUSE || > device->GetDeviceType() == device::BluetoothDevice::DEVICE_KEYBOARD_MOUSE_COMBO Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:239: } /*else if (device->GetDeviceId() == pointing_device_id_ && )*/ On 2014/04/29 10:59:37, Nikita Kostylev wrote: > Debug code? Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:242: void HIDDetectionScreenHandler::PairAsKeyboardDevice( On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: rename to TryPairing... Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:245: (device->GetDeviceType() == device::BluetoothDevice::DEVICE_KEYBOARD || On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Same here, please extract helper method. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:273: NOTIMPLEMENTED(); On 2014/04/29 10:59:37, Nikita Kostylev wrote: > I think you should just update UI state here (in case input device is removed). > > It seems UI update is already handled in OnInputDeviceRemoved() so you may as > well just remove NOTIMPLEMENTED(). https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:317: base::Unretained(this), On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Align parameters base::Bind() > > Suggestions: you may as well run git cl format > which runs clang-format for your changes only. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:324: // If no connected devices found as pointing device and keyboard, we try to On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Insert empty line before comment. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:385: device::BluetoothDevice::DeviceType device_type = device->GetDeviceType(); On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: insert empty line. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:411: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Add {} for all blocks. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:426: LOG(WARNING) << "BTConnectError"; On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Add extra info such as address, device_type and error_code. Done. https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:427: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) On 2014/04/29 10:59:37, Nikita Kostylev wrote: > nit: Add {} for all blocks. Done.
https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:207: device->CancelPairing(); On 2014/04/29 14:30:26, merkulova wrote: > On 2014/04/29 10:59:37, Nikita Kostylev wrote: > > Why cancel pairing when passkey is confirmed? > > It's confirmation of user-set pincode. We don't support this functionality on > discussion with @keybuk. It's not "passkey is confirmed" it's "when the user needs to confirm a passkey" - confirming a passkey requires that the user be able to use an input device to select Accept or Reject Since you're still pairing the input devices at this point, you don't have an input device to do that, so it's appropriate to cancel any pairing attempt that would use that pairing method (And keyboards and mice don't use this pairing method, so that really just means you picked the wrong device by accident) https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:205: params.SetString("pincode", base::UintToString(passkey)); Needs to be left-0 padded to 6 characters if passkey=47 then pincode needs to be 000047 Enter https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:397: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) { also DEVICE_TABLET for things like the Apple Magic Trackpad
https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:205: params.SetString("pincode", base::UintToString(passkey)); On 2014/04/29 18:51:04, keybuk wrote: > Needs to be left-0 padded to 6 characters > > if passkey=47 then pincode needs to be 000047 Enter Done. https://codereview.chromium.org/252503002/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:397: if (device_type == device::BluetoothDevice::DEVICE_MOUSE) { On 2014/04/29 18:51:04, keybuk wrote: > also DEVICE_TABLET for things like the Apple Magic Trackpad Done.
Bluetooth bits lgtm (no OWNERS stake in this CL)
+oshima for chromeos_switches TBR
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/252503002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/252503002/270001
Message was sent while issue was closed.
Change committed as 268185 |
