Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
528803002:
Host pairing OOBE starts at the right time. (Closed)
|
Created:
6 years, 3 months ago by dzhioev (left Google) Modified:
6 years, 3 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:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionHost pairing OOBE starts at the right time.
The following behaviour implmented:
* If "new-remora-oobe" switch is not set, OOBE starts with with the regular
"remora" UI. At the same time wizard controller we start to listen for
incoming connection from shark controller. If connection is established, we
switch UI to the new one.
* Otherwise, we launch the new UI from the beginning of OOBE.
Several small issues fixed as well.
BUG=405150
TEST=manually
Committed: https://crrev.com/958e55816c7f9960588b3da7021cd37ec910846b
Cr-Commit-Position: refs/heads/master@{#295721}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Comments addressed. #
Total comments: 26
Patch Set 3 : Comments addressed. Unrelated changes removed. #Patch Set 4 : Fix. #
Total comments: 25
Patch Set 5 : Comments addressed. #Patch Set 6 : Nit fixed. #
Messages
Total messages: 29 (4 generated)
dzhioev@chromium.org changed reviewers: + achuith@google.com, maruel@chromium.org, zork@chromium.org
Hi, please review. maruel@: for PRESUBMIT.py review. I've temporarily allowed ScopedAllowIO for couple of files. zork@, achuith@: rest of the files.
On 2014/09/01 16:32:47, dzhioev wrote: > Hi, please review. > > maruel@: for PRESUBMIT.py review. I've temporarily allowed ScopedAllowIO for > couple of files. > > zork@, achuith@: rest of the files. One more thing, I'm working on tests now, will submit them later.
dzhioev@chromium.org changed reviewers: + achuith@chromium.org - achuith@google.com
lgtm
There's a lot of different CLs in here. the contextReady CL the sharkconnection listener CL removing old switches adding new switches + static Create functions. random fixes such as core_oobe_handler and bluetooth_host_pairing_controller. Could you please split these apart into separate CLs? This mega-CLs are hard to review. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/host_pairing_screen.cc:139: // TODO(dzhioev): This is a temporary solution. HostPairingScreen should I think we have a fundamental misunderstanding of how this should work. The pairing screen should only handle pairing, and pass off enrollment to the enrollment screen. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/screen_context.h:88: const base::DictionaryValue& storage() { return storage_; } shouldn't this function be const. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:140: // Can't be defined in the anonymous namespace because it is not possible to It is actually possible to fwd declare classes defined in anonymous namespace (it is against the style guide to have an anonymous namespace in the header file). Anyway, you should just drop this comment since it's a unimportant technical detail. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:142: class SharkConnectionListener I would prefer to move this to a separate file if there isn't a more appropriate place for this. This shouldn't be in wizard_controller I think. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:682: // Stop listening for a shark controller if we leaved the network screen. s/leaved/left. This is too early. We should stop listening once we either connect to a shark controller or if we start enrollment. The logic here is a little tricky given that we want to eventually support multiple shark controllers, but we definitely should not stop listening on the EULA screen, or the enrollment screen. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:1041: if (CanShowHIDDetectionScreen() && !IsNewRemoraOobe()) { Why are we not showing HIDDetection screen here? https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.h:323: bool IsSharkOobe() const; Please add function comments. Are these functions: IsSharkOobe(), IsRemoraOobe() used anywhere? If not, they should be in anonymous scope in the cc file. https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:286: StartupUtils::SaveOobePendingScreen(""); What is this?
https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/host_pairing_screen.cc:139: // TODO(dzhioev): This is a temporary solution. HostPairingScreen should On 2014/09/02 22:32:10, achuithb wrote: > I think we have a fundamental misunderstanding of how this should work. > > The pairing screen should only handle pairing, and pass off enrollment to the > enrollment screen. So this code is correct (and not just temporary). The comment is incorrect.
https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/host_pairing_screen.cc:32: base::ThreadRestrictions::ScopedAllowIO allow_io; Instead of trying to land this exception, might be easier to use DeleteSoon and delete this on the blocking pool or IO thread instead.
rubberstamp PRESUBMIT.py. It could easily be a separate CL too.
On 2014/09/03 01:22:50, M-A Ruel wrote: > rubberstamp PRESUBMIT.py. It could easily be a separate CL too. err, I mean rubberstamp lgtm for PRESUBMIT.py.
https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/host_pairing_screen.cc:32: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2014/09/02 23:49:50, achuithb wrote: > Instead of trying to land this exception, might be easier to use DeleteSoon and > delete this on the blocking pool or IO thread instead. It will not work, because BluetoothSocketChromeOS expects that it will be destructed on the UI thread. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/host_pairing_screen.cc:139: // TODO(dzhioev): This is a temporary solution. HostPairingScreen should On 2014/09/02 23:32:06, achuithb wrote: > On 2014/09/02 22:32:10, achuithb wrote: > > I think we have a fundamental misunderstanding of how this should work. > > > > The pairing screen should only handle pairing, and pass off enrollment to the > > enrollment screen. > > So this code is correct (and not just temporary). The comment is incorrect. It is correct. HostPairingScreen and ControllerPairingScreen will handle enrollment by themselves. We simply can't implement it in an another way from the UI perspective. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/screen_context.h:88: const base::DictionaryValue& storage() { return storage_; } On 2014/09/02 22:32:10, achuithb wrote: > shouldn't this function be const. Yes, done. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:140: // Can't be defined in the anonymous namespace because it is not possible to On 2014/09/02 22:32:10, achuithb wrote: > It is actually possible to fwd declare classes defined in anonymous namespace > (it is against the style guide to have an anonymous namespace in the header > file). Anyway, you should just drop this comment since it's a unimportant > technical detail. Done. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:142: class SharkConnectionListener On 2014/09/02 22:32:10, achuithb wrote: > I would prefer to move this to a separate file if there isn't a more appropriate > place for this. This shouldn't be in wizard_controller I think. Done. Moved to "components/pairing/connection_listener.*" https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:682: // Stop listening for a shark controller if we leaved the network screen. On 2014/09/02 22:32:10, achuithb wrote: > s/leaved/left. > > This is too early. We should stop listening once we either connect to a shark > controller or if we start enrollment. The logic here is a little tricky given > that we want to eventually support multiple shark controllers, but we definitely > should not stop listening on the EULA screen, or the enrollment screen. Is it really necessary? If the user has input device connected, and he has set up network for host, wouldn't it mean that he is going to set up device without controller? Anyway, let's do it in an another CL. It'll require nontrivial changes, because we want to be sure that we don't switch to the pairing screen when update or enrollment processes are in progress. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:1041: if (CanShowHIDDetectionScreen() && !IsNewRemoraOobe()) { On 2014/09/02 22:32:10, achuithb wrote: > Why are we not showing HIDDetection screen here? Because if the new remora OOBE is enabled, we don't want to set up input devices, we want to show something like "Connect controller" message from the beginning. And if we don't put this check here, HID detection will pop up as a first screen. https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.h:323: bool IsSharkOobe() const; On 2014/09/02 22:32:10, achuithb wrote: > Please add function comments. > > Are these functions: > IsSharkOobe(), IsRemoraOobe() used anywhere? If not, they should be in anonymous > scope in the cc file. Done. Moved Is{Shark,Remora}Oobe to *.cc. Added comments. https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:286: StartupUtils::SaveOobePendingScreen(""); On 2014/09/02 22:32:11, achuithb wrote: > What is this? WizardController saves the last screen he was showing into kOobeScreenPending preference, to resume to this screen after reboot. So, for example, if the user sets requisition on eula screen, wizard controller decides to show eula screen after reboot. We don't want such behavior. Setting this preference to default value indicates that we want to begin OOBE from scratch after reboot.
On 2014/09/04 15:59:48, dzhioev wrote: > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right): > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screens/host_pairing_screen.cc:32: > base::ThreadRestrictions::ScopedAllowIO allow_io; > On 2014/09/02 23:49:50, achuithb wrote: > > Instead of trying to land this exception, might be easier to use DeleteSoon > and > > delete this on the blocking pool or IO thread instead. > > It will not work, because BluetoothSocketChromeOS expects that it will be > destructed on the UI thread. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screens/host_pairing_screen.cc:139: // > TODO(dzhioev): This is a temporary solution. HostPairingScreen should > On 2014/09/02 23:32:06, achuithb wrote: > > On 2014/09/02 22:32:10, achuithb wrote: > > > I think we have a fundamental misunderstanding of how this should work. > > > > > > The pairing screen should only handle pairing, and pass off enrollment to > the > > > enrollment screen. > > > > So this code is correct (and not just temporary). The comment is incorrect. > > It is correct. HostPairingScreen and ControllerPairingScreen will handle > enrollment by themselves. We simply can't implement it in an another way from > the UI perspective. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/screens/screen_context.h (right): > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screens/screen_context.h:88: const > base::DictionaryValue& storage() { return storage_; } > On 2014/09/02 22:32:10, achuithb wrote: > > shouldn't this function be const. > > Yes, done. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/wizard_controller.cc:140: // Can't be defined in > the anonymous namespace because it is not possible to > On 2014/09/02 22:32:10, achuithb wrote: > > It is actually possible to fwd declare classes defined in anonymous namespace > > (it is against the style guide to have an anonymous namespace in the header > > file). Anyway, you should just drop this comment since it's a unimportant > > technical detail. > > Done. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/wizard_controller.cc:142: class > SharkConnectionListener > On 2014/09/02 22:32:10, achuithb wrote: > > I would prefer to move this to a separate file if there isn't a more > appropriate > > place for this. This shouldn't be in wizard_controller I think. > > Done. Moved to "components/pairing/connection_listener.*" > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/wizard_controller.cc:682: // Stop listening for a > shark controller if we leaved the network screen. > On 2014/09/02 22:32:10, achuithb wrote: > > s/leaved/left. > > > > This is too early. We should stop listening once we either connect to a shark > > controller or if we start enrollment. The logic here is a little tricky given > > that we want to eventually support multiple shark controllers, but we > definitely > > should not stop listening on the EULA screen, or the enrollment screen. > > Is it really necessary? If the user has input device connected, and he has set > up network for host, wouldn't it mean that he is going to set up device without > controller? > Anyway, let's do it in an another CL. It'll require nontrivial changes, because > we want to be sure that we don't switch to the pairing screen when update or > enrollment processes are in progress. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/wizard_controller.cc:1041: if > (CanShowHIDDetectionScreen() && !IsNewRemoraOobe()) { > On 2014/09/02 22:32:10, achuithb wrote: > > Why are we not showing HIDDetection screen here? > Because if the new remora OOBE is enabled, we don't want to set up input > devices, we want to show something like "Connect controller" message from the > beginning. And if we don't put this check here, HID detection will pop up as a > first screen. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/wizard_controller.h (right): > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/wizard_controller.h:323: bool IsSharkOobe() const; > On 2014/09/02 22:32:10, achuithb wrote: > > Please add function comments. > > > > Are these functions: > > IsSharkOobe(), IsRemoraOobe() used anywhere? If not, they should be in > anonymous > > scope in the cc file. > > Done. Moved Is{Shark,Remora}Oobe to *.cc. Added comments. > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... > File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): > > https://codereview.chromium.org/528803002/diff/1/chrome/browser/ui/webui/chro... > chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:286: > StartupUtils::SaveOobePendingScreen(""); > On 2014/09/02 22:32:11, achuithb wrote: > > What is this? > WizardController saves the last screen he was showing into kOobeScreenPending > preference, to resume to this screen after reboot. So, for example, if the user > sets requisition on eula screen, wizard controller decides to show eula screen > after reboot. We don't want such behavior. Setting this preference to default > value indicates that we want to begin OOBE from scratch after reboot. Achuith, please take another look.
Pavel, I'm not going to be able to review this change any further with 23 files - I've asked you to break this up into smaller CLs. Please have separate CLs for: 1. HostPairingController::Create/ControllerPairingController::Create, with the new flags. 2. SharkConnectionListener stuff, after you have rebased on top of Zach's changes. 3. bluetooth_host_pairing_controller.cc 4. Removal of show-host-pairing-demo 5. contextReady business in HostPairingScreenHandler. 6. core_oobe_handler.cc 7. screen_context.h I feel like I'm trying to review all 7 of these CLs at once and it's cognitive overload figuring out which changes are independent of others. I would be able to provide faster reviews if your CLs were more easily reviewable. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right): https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screens/host_pairing_screen.h:26: void SetController( You need to rebase this on top of Zach's changes - I don't think this is necessary any longer. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:136: bool IsSharkOobe() { Please remove this - it's used in only one place https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:143: bool IsRemoraOobe() { Let's call this IsRemoraRequisition https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:615: if (IsSharkOobe()) { I prefer inlining IsSharkOobe in the way that this was previously done: const bool is_shark = g_browser_process->platform_part()->browser_policy_connector_chromeos()-> GetDeviceCloudPolicyManager()->IsSharkRequisition(); This is easier to read rather than a one-time use function. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.cc:31: switch (new_stage) { Replace this switch with an if statement handling STAGE_WAITING_FOR_CODE_CONFIRMATION, since the other cases don't do anything useful. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.h (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:5: #ifndef COMPONENTS_PAIRING_CONNECTION_LISTENER_H_ Rename this to SharkConnectionListener - ConnectionListener is very generic. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:24: scoped_ptr<HostPairingController>)> ConnectionCallback; OnConnectedCallback. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:26: explicit ConnectionListener(ConnectionCallback callback); You're using this callback to pass ownership of controller_ back to the caller - this isn't obvious and difficult to read. Could you have a method like: scoped_ptr<HostPairingController> GetController(); You should still have an OnConnectedCallback, but with either a status or no arguments. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:32: virtual void PairingStageChanged(Stage new_stage) OVERRIDE; Add comment: // HostPairingController::Observer overrides:
https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:948: } else if (IsNewRemoraOobe()) { Just make this the first if stmt in this sequence and then you don't need the awkward !IsNewRemoraOobe()
https://codereview.chromium.org/528803002/diff/20001/components/pairing/pairi... File components/pairing/pairing_switches.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/pairi... components/pairing/pairing_switches.cc:12: const char kNewRemoraOobe[] = "new-remora-oobe"; Is this supposed to be a permanent switch? If so, new-remora-oobe seems like a bad name. Maybe --host-pairing-oobe or something like this? If this is supposed to be a temporary switch, we should just use the show-host-pairing-demo that we had before - there's no point in replacing one temporary switch with another.
https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right): https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screens/host_pairing_screen.h:26: void SetController( On 2014/09/10 11:32:40, achuithb wrote: > You need to rebase this on top of Zach's changes - I don't think this is > necessary any longer. Done. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:136: bool IsSharkOobe() { On 2014/09/10 11:32:40, achuithb wrote: > Please remove this - it's used in only one place Done. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:143: bool IsRemoraOobe() { On 2014/09/10 11:32:40, achuithb wrote: > Let's call this IsRemoraRequisition Done. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:615: if (IsSharkOobe()) { On 2014/09/10 11:32:40, achuithb wrote: > I prefer inlining IsSharkOobe in the way that this was previously done: > > const bool is_shark = > g_browser_process->platform_part()->browser_policy_connector_chromeos()-> > GetDeviceCloudPolicyManager()->IsSharkRequisition(); > > This is easier to read rather than a one-time use function. Done. https://codereview.chromium.org/528803002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:948: } else if (IsNewRemoraOobe()) { On 2014/09/11 13:49:56, achuithb wrote: > Just make this the first if stmt in this sequence and then you don't need the > awkward !IsNewRemoraOobe() Done. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.cc:31: switch (new_stage) { On 2014/09/10 11:32:40, achuithb wrote: > Replace this switch with an if statement handling > STAGE_WAITING_FOR_CODE_CONFIRMATION, since the other cases don't do anything > useful. 'default' branch is useful for detecting unexpected stages. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.h (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:5: #ifndef COMPONENTS_PAIRING_CONNECTION_LISTENER_H_ On 2014/09/10 11:32:40, achuithb wrote: > Rename this to SharkConnectionListener - ConnectionListener is very generic. Done. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:24: scoped_ptr<HostPairingController>)> ConnectionCallback; On 2014/09/10 11:32:40, achuithb wrote: > OnConnectedCallback. Done. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:26: explicit ConnectionListener(ConnectionCallback callback); On 2014/09/10 11:32:40, achuithb wrote: > You're using this callback to pass ownership of controller_ back to the caller - > this isn't obvious and difficult to read. > > Could you have a method like: > scoped_ptr<HostPairingController> GetController(); > > You should still have an OnConnectedCallback, but with either a status or no > arguments. I think your approach is more error-prone. Somebody can call GetController too early, or too late, or call it twice, or write GetController()->DoSomething(). https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:32: virtual void PairingStageChanged(Stage new_stage) OVERRIDE; On 2014/09/10 11:32:40, achuithb wrote: > Add comment: > // HostPairingController::Observer overrides: Done. https://codereview.chromium.org/528803002/diff/20001/components/pairing/pairi... File components/pairing/pairing_switches.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/pairi... components/pairing/pairing_switches.cc:12: const char kNewRemoraOobe[] = "new-remora-oobe"; On 2014/09/11 13:54:05, achuithb wrote: > Is this supposed to be a permanent switch? If so, new-remora-oobe seems like a > bad name. Maybe --host-pairing-oobe or something like this? > > If this is supposed to be a temporary switch, we should just use the > show-host-pairing-demo that we had before - there's no point in replacing one > temporary switch with another. Yes, it is permanent switch. Renamed to "host-pairing-oobe".
achuith@chromium.org changed reviewers: - maruel@chromium.org
Thanks for making this CL much more focused - it's a lot easier to review and should be easier to understand by sheriffs, revert, etc. This CL is looking pretty good overall. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.cc:31: switch (new_stage) { On 2014/09/18 09:07:02, dzhioev wrote: > On 2014/09/10 11:32:40, achuithb wrote: > > Replace this switch with an if statement handling > > STAGE_WAITING_FOR_CODE_CONFIRMATION, since the other cases don't do anything > > useful. > 'default' branch is useful for detecting unexpected stages. I've commented on this elsewhere - I don't think we should do ad-hoc debugging of the state machine like this. NOTREACHED is also not useful for chromeos testing because the devices almost never run debug builds. This function only cares about one particular state (waiting for code confirmation), so just check for that state. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.h (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:26: explicit ConnectionListener(ConnectionCallback callback); On 2014/09/18 09:07:02, dzhioev wrote: > On 2014/09/10 11:32:40, achuithb wrote: > > You're using this callback to pass ownership of controller_ back to the caller > - > > this isn't obvious and difficult to read. > > > > Could you have a method like: > > scoped_ptr<HostPairingController> GetController(); > > > > You should still have an OnConnectedCallback, but with either a status or no > > arguments. > > I think your approach is more error-prone. Somebody can call GetController too > early, or too late, or call it twice, or write GetController()->DoSomething(). Obviously the expectation is that GetController is called after OnConnectedCallback, just as there are expectations on how often and when ConnectionCallback is invoked. The callback may also be called by this class twice, or not at all, and at the wrong time, and there is no protection against that either. This is not a public api where anything can happen - it's two internal related classes and there is no reason to be super paranoid about misuse of the interface. The interface I propose is easier to read and understand; and as for error-prone, I think the equivalent bugs can be introduced with the interface you have here as well. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1245: return IsRemoraRequisition() && (CommandLine::ForCurrentProcess()->HasSwitch( I would break this after && and after || https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1251: if (!IsRemoraRequisition()) { Don't need {} https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1258: if (!shark_connection_listener_) You need {} here https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1260: new pairing_chromeos::SharkConnectionListener(base::Bind( base::Bind has to be on a new line I believe https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:306: // Returns true if the new (pairing) remora OOBE is active. Get rid of 'the new' since at some point this won't be so new any more. // Returns true for pairing remora OOBE. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:309: // Starts listen for incoming shark controller connection, if we are running Start listening for an incoming shark controller connection, if we are running remora OOBE. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:313: // Called when a connection to controller has been established. If you insist on doing it this way, please call out that you're transferring ownership of the controller in the callback. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:417: // new remora OOBE. This comment doesn't seem accurate? I think it's sufficient to say: // Listens for incoming shark controller connections. You don't need to talk about old and new remora OOBEs. https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:160: // If this switch is set, start pairing remora OOBE from the beginning. // With this switch, start remora OOBE with the pairing screen. You don't really need the second line. https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.h (right): https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.h:78: CHROMEOS_EXPORT extern const char kShowHostPairingDemo[]; Please also remove this switch since it's no longer used. https://codereview.chromium.org/528803002/diff/60001/components/pairing/shark... File components/pairing/shark_connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/60001/components/pairing/shark... components/pairing/shark_connection_listener.cc:31: case HostPairingController::STAGE_WAITING_FOR_CODE_CONFIRMATION: I really don't agree with this flow. I strongly prefer a single if statement, rather than random debugging state checks that are unlikely to catch any bugs. Another issue with the NOTREACHED is that it won't trigger on chromeos devices since it's very rare for somebody to install a debug build. I think this is a lot of cruft for no current or future benefit. If there's really a problem with the state machine we can add a function that verifies acceptable states, etc, but I don't think this is the way to do this.
I've tested the end-to-end with this patch, and it works as expected.
https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.cc:31: switch (new_stage) { On 2014/09/18 22:46:16, achuithb wrote: > On 2014/09/18 09:07:02, dzhioev wrote: > > On 2014/09/10 11:32:40, achuithb wrote: > > > Replace this switch with an if statement handling > > > STAGE_WAITING_FOR_CODE_CONFIRMATION, since the other cases don't do anything > > > useful. > > 'default' branch is useful for detecting unexpected stages. > > I've commented on this elsewhere - I don't think we should do ad-hoc debugging > of the state machine like this. NOTREACHED is also not useful for chromeos > testing because the devices almost never run debug builds. This function only > cares about one particular state (waiting for code confirmation), so just check > for that state. Done. https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... File components/pairing/connection_listener.h (right): https://codereview.chromium.org/528803002/diff/20001/components/pairing/conne... components/pairing/connection_listener.h:26: explicit ConnectionListener(ConnectionCallback callback); On 2014/09/18 22:46:16, achuithb wrote: > On 2014/09/18 09:07:02, dzhioev wrote: > > On 2014/09/10 11:32:40, achuithb wrote: > > > You're using this callback to pass ownership of controller_ back to the > caller > > - > > > this isn't obvious and difficult to read. > > > > > > Could you have a method like: > > > scoped_ptr<HostPairingController> GetController(); > > > > > > You should still have an OnConnectedCallback, but with either a status or no > > > arguments. > > > > I think your approach is more error-prone. Somebody can call GetController too > > early, or too late, or call it twice, or write GetController()->DoSomething(). > > Obviously the expectation is that GetController is called after > OnConnectedCallback, just as there are expectations on how often and when > ConnectionCallback is invoked. > > The callback may also be called by this class twice, or not at all, and at the > wrong time, and there is no protection against that either. This is not a public > api where anything can happen - it's two internal related classes and there is > no reason to be super paranoid about misuse of the interface. > > The interface I propose is easier to read and understand; and as for > error-prone, I think the equivalent bugs can be introduced with the interface > you have here as well. I prefer my implementation still. The current interface is extremely clear, because it has only one method -- constructor. An ownership transfer is enforced by the callback signature. It is almost impossible to do something wrong with such interface. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1245: return IsRemoraRequisition() && (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/09/18 22:46:16, achuithb wrote: > I would break this after && and after || Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1251: if (!IsRemoraRequisition()) { On 2014/09/18 22:46:16, achuithb wrote: > Don't need {} Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1258: if (!shark_connection_listener_) On 2014/09/18 22:46:16, achuithb wrote: > You need {} here Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:1260: new pairing_chromeos::SharkConnectionListener(base::Bind( On 2014/09/18 22:46:16, achuithb wrote: > base::Bind has to be on a new line I believe Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:306: // Returns true if the new (pairing) remora OOBE is active. On 2014/09/18 22:46:17, achuithb wrote: > Get rid of 'the new' since at some point this won't be so new any more. > > // Returns true for pairing remora OOBE. Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:309: // Starts listen for incoming shark controller connection, if we are running On 2014/09/18 22:46:17, achuithb wrote: > Start listening for an incoming shark controller connection, if we are running > remora OOBE. Done. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:313: // Called when a connection to controller has been established. On 2014/09/18 22:46:17, achuithb wrote: > If you insist on doing it this way, please call out that you're transferring > ownership of the controller in the callback. Yes, I prefer this way. Changed the comment. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:417: // new remora OOBE. On 2014/09/18 22:46:17, achuithb wrote: > This comment doesn't seem accurate? I think it's sufficient to say: > // Listens for incoming shark controller connections. > > You don't need to talk about old and new remora OOBEs. It's accurate enough. We only create use connection listener if "kHostPairingOobe" is not set. Otherwise we display the pairing screen ( http://goo.gl/q4oJbW ), and this screen listens for connection. I reworded the comment, check if the new version is OK. https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:160: // If this switch is set, start pairing remora OOBE from the beginning. On 2014/09/18 22:46:17, achuithb wrote: > // With this switch, start remora OOBE with the pairing screen. > > You don't really need the second line. Done. https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... File chromeos/chromeos_switches.h (right): https://codereview.chromium.org/528803002/diff/60001/chromeos/chromeos_switch... chromeos/chromeos_switches.h:78: CHROMEOS_EXPORT extern const char kShowHostPairingDemo[]; On 2014/09/18 22:46:17, achuithb wrote: > Please also remove this switch since it's no longer used. Done. https://codereview.chromium.org/528803002/diff/60001/components/pairing/shark... File components/pairing/shark_connection_listener.cc (right): https://codereview.chromium.org/528803002/diff/60001/components/pairing/shark... components/pairing/shark_connection_listener.cc:31: case HostPairingController::STAGE_WAITING_FOR_CODE_CONFIRMATION: On 2014/09/18 22:46:17, achuithb wrote: > I really don't agree with this flow. I strongly prefer a single if statement, > rather than random debugging state checks that are unlikely to catch any bugs. > > Another issue with the NOTREACHED is that it won't trigger on chromeos devices > since it's very rare for somebody to install a debug build. I think this is a > lot of cruft for no current or future benefit. > > If there's really a problem with the state machine we can add a function that > verifies acceptable states, etc, but I don't think this is the way to do this. I agree, replaced with a single if.
On 2014/09/18 23:37:07, Zachary Kuznia wrote: > I've tested the end-to-end with this patch, and it works as expected. Thanks!
Please fix the CL description (the name of the flag has changed). Fixing comment nit below is optional. LGTM otherwise! Thanks for bearing with me! https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:309: // Starts listen for incoming shark controller connection, if we are running On 2014/09/19 01:55:05, dzhioev wrote: > On 2014/09/18 22:46:17, achuithb wrote: > > Start listening for an incoming shark controller connection, if we are running > > remora OOBE. > > Done. Super nit (and this is my fault): Should be Starts, not Start. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:313: // Called when a connection to controller has been established. On 2014/09/19 01:55:04, dzhioev wrote: > On 2014/09/18 22:46:17, achuithb wrote: > > If you insist on doing it this way, please call out that you're transferring > > ownership of the controller in the callback. > Yes, I prefer this way. Changed the comment. Acknowledged. https://codereview.chromium.org/528803002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.h:417: // new remora OOBE. On 2014/09/19 01:55:05, dzhioev wrote: > On 2014/09/18 22:46:17, achuithb wrote: > > This comment doesn't seem accurate? I think it's sufficient to say: > > // Listens for incoming shark controller connections. > > > > You don't need to talk about old and new remora OOBEs. > > It's accurate enough. We only create use connection listener if > "kHostPairingOobe" is not set. Otherwise we display the pairing screen ( > http://goo.gl/q4oJbW ), and this screen listens for connection. > I reworded the comment, check if the new version is OK. Acknowledged.
The CQ bit was checked by dzhioev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/528803002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as da87bbca92ca25f5c475d989064b489d49a8f4db
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/958e55816c7f9960588b3da7021cd37ec910846b Cr-Commit-Position: refs/heads/master@{#295721}
Message was sent while issue was closed.
On 2014/09/19 17:10:14, I haz the power (commit-bot) wrote: > Patchset 6 (id:??) landed as > https://crrev.com/958e55816c7f9960588b3da7021cd37ec910846b > Cr-Commit-Position: refs/heads/master@{#295721} You didn't update the description...
My fault, sorry about that. On Fri Sep 19 2014 at 10:01:32 PM <achuith@chromium.org> wrote: > On 2014/09/19 17:10:14, I haz the power (commit-bot) wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/958e55816c7f9960588b3da7021cd37ec910846b > > Cr-Commit-Position: refs/heads/master@{#295721} > > You didn't update the description... > > https://codereview.chromium.org/528803002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sok! On Fri, Sep 19, 2014 at 11:14 AM, Pavel Sergeev <dzhioev@chromium.org> wrote: > My fault, sorry about that. > > On Fri Sep 19 2014 at 10:01:32 PM <achuith@chromium.org> wrote: > >> On 2014/09/19 17:10:14, I haz the power (commit-bot) wrote: >> > Patchset 6 (id:??) landed as >> > https://crrev.com/958e55816c7f9960588b3da7021cd37ec910846b >> > Cr-Commit-Position: refs/heads/master@{#295721} >> >> You didn't update the description... >> >> https://codereview.chromium.org/528803002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
