Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492043002:
Introduced switches for Master/Slave bootstrapping process. (Closed)
|
Created:
5 years ago by xdai1 Modified:
5 years ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduced switches for Master/Slave bootstrapping process.
BUG=564370
Committed: https://crrev.com/7300b9e6caf4851d86b6aaa1e55d9cde8a920324
Cr-Commit-Position: refs/heads/master@{#363646}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : Remove the unused shark requisition. #
Total comments: 1
Patch Set 4 : Reverted unnecessary changes back. #
Total comments: 8
Patch Set 5 : Address achuith@'s comment #
Total comments: 2
Patch Set 6 : Address dzhioev@'s comment. #
Messages
Total messages: 56 (21 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/20001
xdai@chromium.org changed reviewers: + achuith@chromium.org
achuith@, could you help to review this CL please? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce switches for shark/remora bootstrapping process. BUG=564370 ========== to ========== Introduced switches for shark/remora bootstrapping process. And also cleaned up the unused shark requisition. BUG=564370 ==========
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
xdai@chromium.org changed reviewers: + dzhioev@chromium.org
+dzhioev@ as a reviewer.
What's the point of getting rid of shark? Could you please revert all changes that delete shark? https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/webui_login_view.cc (left): https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/webui_login_view.cc:68: const char kAccelNameDeviceRequisitionShark[] = "device_requisition_shark"; What is the purpose of removing this?
On 2015/12/02 23:18:34, achuithb wrote: > What's the point of getting rid of shark? Could you please revert all changes > that delete shark? > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/ui/webui_login_view.cc (left): > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > chrome/browser/chromeos/login/ui/webui_login_view.cc:68: const char > kAccelNameDeviceRequisitionShark[] = "device_requisition_shark"; > What is the purpose of removing this? I'm not deleting shark. I just delete shark requisition since it's unused now? Shark still exists. Any device with the switch kOobeBootstrappingShark specified is regarded as a shark. Does that make sense? Btw: Patch 2 is the one without removing shark requisition.
On 2015/12/02 23:25:52, xdai1 wrote: > On 2015/12/02 23:18:34, achuithb wrote: > > What's the point of getting rid of shark? Could you please revert all changes > > that delete shark? > > > > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > > File chrome/browser/chromeos/login/ui/webui_login_view.cc (left): > > > > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/ui/webui_login_view.cc:68: const char > > kAccelNameDeviceRequisitionShark[] = "device_requisition_shark"; > > What is the purpose of removing this? > > I'm not deleting shark. I just delete shark requisition since it's unused now? > Shark still exists. Any device with the switch kOobeBootstrappingShark specified > is regarded as a shark. Does that make sense? Why change this though? Shark requisition is understood by the backend, even though it's not in production. If I understand correctly, you're replacing an accelerator with a command line switch, and I'm not able to see how it helps? I looked at the design doc and the bug, and there are no notes on what the strategy is. I'd rather not break remora/shark, and I don't think it's necessary to change anything in how that works. You can just add another accelerator that works with the current flow. Maybe you should add a section to the design document with details on how you intend to implement your feature, and we can have a discussion there. It's a bit hard to see the big picture from these CLs. > Btw: Patch 2 is the one without removing shark requisition.
On 2015/12/02 23:32:22, achuithb wrote: > On 2015/12/02 23:25:52, xdai1 wrote: > > On 2015/12/02 23:18:34, achuithb wrote: > > > What's the point of getting rid of shark? Could you please revert all > changes > > > that delete shark? > > > > > > > > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/login/ui/webui_login_view.cc (left): > > > > > > > > > https://codereview.chromium.org/1492043002/diff/50001/chrome/browser/chromeos... > > > chrome/browser/chromeos/login/ui/webui_login_view.cc:68: const char > > > kAccelNameDeviceRequisitionShark[] = "device_requisition_shark"; > > > What is the purpose of removing this? > > > > I'm not deleting shark. I just delete shark requisition since it's unused now? > > Shark still exists. Any device with the switch kOobeBootstrappingShark > specified > > is regarded as a shark. Does that make sense? > > Why change this though? > > Shark requisition is understood by the backend, even though it's not in > production. If I understand correctly, you're replacing an accelerator with a > command line switch, and I'm not able to see how it helps? > > I looked at the design doc and the bug, and there are no notes on what the > strategy is. I'd rather not break remora/shark, and I don't think it's necessary > to change anything in how that works. You can just add another accelerator that > works with the current flow. > > Maybe you should add a section to the design document with details on how you > intend to implement your feature, and we can have a discussion there. It's a bit > hard to see the big picture from these CLs. > > > Btw: Patch 2 is the one without removing shark requisition. I reverted all unnecessary changes back. Please take another look. Thanks for your review.
Description was changed from ========== Introduced switches for shark/remora bootstrapping process. And also cleaned up the unused shark requisition. BUG=564370 ========== to ========== Introduced switches for shark/remora bootstrapping process. BUG=564370 ==========
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Please also update the description of the CL. This is much better! https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:150: bool IsBootstrappingMaster() { This is just for development, right? The master will be an android client in production, right? https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:1312: if (!shark_connection_listener_) { We need to rename this, correct? You don't have to do it in this CL. https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const bool is_remora = g_browser_process->platform_part() Shouldn't we keep this logic as well? https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:215: !user_manager::UserManager::Get()->IsUserLoggedIn() && !is_slave; && !is_remora as well?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/70001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduced switches for shark/remora bootstrapping process. BUG=564370 ========== to ========== Introduced switches for Master/Slave bootstrapping process. BUG=564370 ==========
xdai@chromium.org changed reviewers: + alemate@chromium.org
achuithb@, please take another look. Thanks for your review. alemate@, could you help to take a look at the question in the comments in network_screen_handler.cc file? Thanks for your help. https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:150: bool IsBootstrappingMaster() { On 2015/12/04 09:14:11, achuithb wrote: > This is just for development, right? The master will be an android client in > production, right? Right. This is only for the Cros side demo and test. https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:1312: if (!shark_connection_listener_) { On 2015/12/04 09:14:10, achuithb wrote: > We need to rename this, correct? You don't have to do it in this CL. Yes, shark_connection_listener_ is not accurate here. Probably rename it to controller_connection_listener_? https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const bool is_remora = g_browser_process->platform_part() On 2015/12/04 09:14:11, achuithb wrote: > Shouldn't we keep this logic as well? Yes, probably I should not delete this logic. Based on our discussion, I thought I should keep the old logic as is... However, if we keep this logic, suppose we try to manually set up a new "ChromeBox for meeting" device. During the OOBE flow, because |is_remora| is true, EnableLoginLayouts() will not be called, which might violate the case described above in 1). Actually I feel a little confused about this part of logic here. I tried to hard-code the |enable_layouts| to |false| and tested on the device, and it seems nothing is affected. It seems the active input methods will be enabled by SetApplicationLocale() in the network screen anyway and it doesn't matter whether we enable these input methods here. +alemate@ suggestions?
On 2015/12/04 19:04:15, xdai1 wrote: > achuithb@, please take another look. Thanks for your review. > alemate@, could you help to take a look at the question in the comments in > network_screen_handler.cc file? Thanks for your help. > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wizard_controller.cc:150: bool > IsBootstrappingMaster() { > On 2015/12/04 09:14:11, achuithb wrote: > > This is just for development, right? The master will be an android client in > > production, right? > > Right. This is only for the Cros side demo and test. > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wizard_controller.cc:1312: if > (!shark_connection_listener_) { > On 2015/12/04 09:14:10, achuithb wrote: > > We need to rename this, correct? You don't have to do it in this CL. > > Yes, shark_connection_listener_ is not accurate here. Probably rename it to > controller_connection_listener_? > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const bool > is_remora = g_browser_process->platform_part() > On 2015/12/04 09:14:11, achuithb wrote: > > Shouldn't we keep this logic as well? > > Yes, probably I should not delete this logic. Based on our discussion, I thought > I should keep the old logic as is... > > However, if we keep this logic, suppose we try to manually set up a new > "ChromeBox for meeting" device. During the OOBE flow, because |is_remora| is > true, EnableLoginLayouts() will not be called, which might violate the case > described above in 1). > > Actually I feel a little confused about this part of logic here. I tried to > hard-code the |enable_layouts| to |false| and tested on the device, and it seems > nothing is affected. It seems the active input methods will be enabled by > SetApplicationLocale() in the network screen anyway and it doesn't matter > whether we enable these input methods here. +alemate@ suggestions? This part is the only thing that needs to be resolved. I don't think it's a good idea to change the existing remora behavior in this CL
On 2015/12/04 20:03:24, achuithb wrote: > On 2015/12/04 19:04:15, xdai1 wrote: > > achuithb@, please take another look. Thanks for your review. > > alemate@, could you help to take a look at the question in the comments in > > network_screen_handler.cc file? Thanks for your help. > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/wizard_controller.cc:150: bool > > IsBootstrappingMaster() { > > On 2015/12/04 09:14:11, achuithb wrote: > > > This is just for development, right? The master will be an android client in > > > production, right? > > > > Right. This is only for the Cros side demo and test. > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/wizard_controller.cc:1312: if > > (!shark_connection_listener_) { > > On 2015/12/04 09:14:10, achuithb wrote: > > > We need to rename this, correct? You don't have to do it in this CL. > > > > Yes, shark_connection_listener_ is not accurate here. Probably rename it to > > controller_connection_listener_? > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const > bool > > is_remora = g_browser_process->platform_part() > > On 2015/12/04 09:14:11, achuithb wrote: > > > Shouldn't we keep this logic as well? > > > > Yes, probably I should not delete this logic. Based on our discussion, I > thought > > I should keep the old logic as is... > > > > However, if we keep this logic, suppose we try to manually set up a new > > "ChromeBox for meeting" device. During the OOBE flow, because |is_remora| is > > true, EnableLoginLayouts() will not be called, which might violate the case > > described above in 1). > > > > Actually I feel a little confused about this part of logic here. I tried to > > hard-code the |enable_layouts| to |false| and tested on the device, and it > seems > > nothing is affected. It seems the active input methods will be enabled by > > SetApplicationLocale() in the network screen anyway and it doesn't matter > > whether we enable these input methods here. +alemate@ suggestions? > > This part is the only thing that needs to be resolved. I don't think it's a good > idea to change the existing remora behavior in this CL I see your point. I probably will leave it for future CLs. Addressed the comment. Please take another look. Thanks for your review.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/90001
On 2015/12/04 20:19:14, xdai1 wrote: > On 2015/12/04 20:03:24, achuithb wrote: > > On 2015/12/04 19:04:15, xdai1 wrote: > > > achuithb@, please take another look. Thanks for your review. > > > alemate@, could you help to take a look at the question in the comments in > > > network_screen_handler.cc file? Thanks for your help. > > > > > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > > chrome/browser/chromeos/login/wizard_controller.cc:150: bool > > > IsBootstrappingMaster() { > > > On 2015/12/04 09:14:11, achuithb wrote: > > > > This is just for development, right? The master will be an android client > in > > > > production, right? > > > > > > Right. This is only for the Cros side demo and test. > > > > > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/chromeos... > > > chrome/browser/chromeos/login/wizard_controller.cc:1312: if > > > (!shark_connection_listener_) { > > > On 2015/12/04 09:14:10, achuithb wrote: > > > > We need to rename this, correct? You don't have to do it in this CL. > > > > > > Yes, shark_connection_listener_ is not accurate here. Probably rename it to > > > controller_connection_listener_? > > > > > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > > File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc > (left): > > > > > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const > > bool > > > is_remora = g_browser_process->platform_part() > > > On 2015/12/04 09:14:11, achuithb wrote: > > > > Shouldn't we keep this logic as well? > > > > > > Yes, probably I should not delete this logic. Based on our discussion, I > > thought > > > I should keep the old logic as is... > > > > > > However, if we keep this logic, suppose we try to manually set up a new > > > "ChromeBox for meeting" device. During the OOBE flow, because |is_remora| is > > > true, EnableLoginLayouts() will not be called, which might violate the case > > > described above in 1). > > > > > > Actually I feel a little confused about this part of logic here. I tried to > > > hard-code the |enable_layouts| to |false| and tested on the device, and it > > seems > > > nothing is affected. It seems the active input methods will be enabled by > > > SetApplicationLocale() in the network screen anyway and it doesn't matter > > > whether we enable these input methods here. +alemate@ suggestions? > > > > This part is the only thing that needs to be resolved. I don't think it's a > good > > idea to change the existing remora behavior in this CL > > I see your point. I probably will leave it for future CLs. Addressed the > comment. Please take another look. Thanks for your review. Yup, if this doesn't work as expected, please open a new bug+CL. lgtm! Thank you for taking care of the suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:144: bool IsBootstrappingSlave() { I don't get it. Does this flag affect anything? As far as I can see, this flag alone (w/o remora requisition) only enables a forced enrollment (meaning we show the enrollment screen after the update phase), and that is not what we want. With remora requisition enabled, this flag becomes meaningless. Nothing will change if we remove it from the both places where it is checked: 1.if (IsBootstrappingSlave() || IsBootstrappingMaster()) GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); If remora requisition enabled, kDeviceEnrollmentAutoStart is set to true in device_cloud_policy_manager_chromeos.cc. 2. (!IsRemoraRequisition() && !IsBootstrappingSlave()) this expression is always false if remora requisition enabled. Am I wrong?
On 2015/12/04 21:23:00, dzhioev wrote: > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wizard_controller.cc:144: bool > IsBootstrappingSlave() { > I don't get it. Does this flag affect anything? As far as I can see, this flag > alone (w/o remora requisition) only enables a forced enrollment (meaning we show > the enrollment screen after the update phase), and that is not what we want. > With remora requisition enabled, this flag becomes meaningless. Nothing will > change if we remove it from the both places where it is checked: > 1.if (IsBootstrappingSlave() || IsBootstrappingMaster()) > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > > If remora requisition enabled, kDeviceEnrollmentAutoStart is set to true in > device_cloud_policy_manager_chromeos.cc. > 2. (!IsRemoraRequisition() && !IsBootstrappingSlave()) > > this expression is always false if remora requisition enabled. Is remora requisition enabled, then !IsRemoraRequisition() is false, then it will try to listen to the incoming shark connections. > > Am I wrong? The thing is bootstrapping project has nothing to do with shark/remora. So shark/remora and master/slave are totally different things. A slave device is not necessarily a remora device and vice versa. The bootstrapping master/slave project is based on the existing shark/remora framework, but is not supposed to modify the existing shark/remora behavior (according to achuith@). So that's why we introduce extra flags for master/slave to put the devices into master/slave mode. These flags 1) tell the device if it's a master or a slave; 2) force the enrollment flow. I hope that makes sense to you.
On 2015/12/04 21:45:35, xdai1 wrote: > On 2015/12/04 21:23:00, dzhioev wrote: > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/wizard_controller.cc:144: bool > > IsBootstrappingSlave() { > > I don't get it. Does this flag affect anything? As far as I can see, this flag > > alone (w/o remora requisition) only enables a forced enrollment (meaning we > show > > the enrollment screen after the update phase), and that is not what we want. > > With remora requisition enabled, this flag becomes meaningless. Nothing will > > change if we remove it from the both places where it is checked: > > 1.if (IsBootstrappingSlave() || IsBootstrappingMaster()) > > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > > > > If remora requisition enabled, kDeviceEnrollmentAutoStart is set to true in > > device_cloud_policy_manager_chromeos.cc. > > 2. (!IsRemoraRequisition() && !IsBootstrappingSlave()) > > > > this expression is always false if remora requisition enabled. > > Is remora requisition enabled, then !IsRemoraRequisition() is false, then it > will try to listen to the incoming shark connections. > > > > > Am I wrong? > > The thing is bootstrapping project has nothing to do with shark/remora. So > shark/remora and master/slave are totally different things. A slave device is > not necessarily a remora device and vice versa. The bootstrapping master/slave > project is based on the existing shark/remora framework, but is not supposed to > modify the existing shark/remora behavior (according to achuith@). So that's why > we introduce extra flags for master/slave to put the devices into master/slave > mode. These flags 1) tell the device if it's a master or a slave; 2) force the > enrollment flow. > > I hope that makes sense to you. Does the bootstrapping involve a code confirmation?
On 2015/12/04 23:29:47, dzhioev wrote: > On 2015/12/04 21:45:35, xdai1 wrote: > > On 2015/12/04 21:23:00, dzhioev wrote: > > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > > chrome/browser/chromeos/login/wizard_controller.cc:144: bool > > > IsBootstrappingSlave() { > > > I don't get it. Does this flag affect anything? As far as I can see, this > flag > > > alone (w/o remora requisition) only enables a forced enrollment (meaning we > > show > > > the enrollment screen after the update phase), and that is not what we want. > > > With remora requisition enabled, this flag becomes meaningless. Nothing will > > > change if we remove it from the both places where it is checked: > > > 1.if (IsBootstrappingSlave() || IsBootstrappingMaster()) > > > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > > > > > > If remora requisition enabled, kDeviceEnrollmentAutoStart is set to true > in > > > device_cloud_policy_manager_chromeos.cc. > > > 2. (!IsRemoraRequisition() && !IsBootstrappingSlave()) > > > > > > this expression is always false if remora requisition enabled. > > > > Is remora requisition enabled, then !IsRemoraRequisition() is false, then it > > will try to listen to the incoming shark connections. > > > > > > > > Am I wrong? > > > > The thing is bootstrapping project has nothing to do with shark/remora. So > > shark/remora and master/slave are totally different things. A slave device is > > not necessarily a remora device and vice versa. The bootstrapping master/slave > > project is based on the existing shark/remora framework, but is not supposed > to > > modify the existing shark/remora behavior (according to achuith@). So that's > why > > we introduce extra flags for master/slave to put the devices into master/slave > > mode. These flags 1) tell the device if it's a master or a slave; 2) force the > > enrollment flow. > > > > I hope that makes sense to you. > > Does the bootstrapping involve a code confirmation? Currently it does. Why? The strings on the controller/host screens need to be modified to also fit into the bootstrapping project. But eventually the bootstrapping project will has its own screen for the slave device (according to omrilio@).
https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const bool is_remora = g_browser_process->platform_part() > However, if we keep this logic, suppose we try to manually set up a new > "ChromeBox for meeting" device. I don't understand this. Do you mean "by connecting USB keyboard"? I didn't know this is possible. In this case "is_remora" must be true only if shark-remora OOBE is in progress. If no shark device is connected, is_slave must be false. > During the OOBE flow, because |is_remora| is > true, EnableLoginLayouts() will not be called, which might violate the case > described above in 1). > > Actually I feel a little confused about this part of logic here. I tried to > hard-code the |enable_layouts| to |false| and tested on the device, and it seems > nothing is affected. It seems the active input methods will be enabled by > SetApplicationLocale() in the network screen anyway and it doesn't matter > whether we enable these input methods here. +alemate@ suggestions? Did you try selecting any non-Latin languages?
On 2015/12/04 23:43:46, xdai1 wrote: > On 2015/12/04 23:29:47, dzhioev wrote: > > On 2015/12/04 21:45:35, xdai1 wrote: > > > On 2015/12/04 21:23:00, dzhioev wrote: > > > > > > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > > > chrome/browser/chromeos/login/wizard_controller.cc:144: bool > > > > IsBootstrappingSlave() { > > > > I don't get it. Does this flag affect anything? As far as I can see, this > > flag > > > > alone (w/o remora requisition) only enables a forced enrollment (meaning > we > > > show > > > > the enrollment screen after the update phase), and that is not what we > want. > > > > With remora requisition enabled, this flag becomes meaningless. Nothing > will > > > > change if we remove it from the both places where it is checked: > > > > 1.if (IsBootstrappingSlave() || IsBootstrappingMaster()) > > > > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > > > > > > > > If remora requisition enabled, kDeviceEnrollmentAutoStart is set to true > > in > > > > device_cloud_policy_manager_chromeos.cc. > > > > 2. (!IsRemoraRequisition() && !IsBootstrappingSlave()) > > > > > > > > this expression is always false if remora requisition enabled. > > > > > > Is remora requisition enabled, then !IsRemoraRequisition() is false, then it > > > will try to listen to the incoming shark connections. > > > > > > > > > > > Am I wrong? > > > > > > The thing is bootstrapping project has nothing to do with shark/remora. So > > > shark/remora and master/slave are totally different things. A slave device > is > > > not necessarily a remora device and vice versa. The bootstrapping > master/slave > > > project is based on the existing shark/remora framework, but is not supposed > > to > > > modify the existing shark/remora behavior (according to achuith@). So that's > > why > > > we introduce extra flags for master/slave to put the devices into > master/slave > > > mode. These flags 1) tell the device if it's a master or a slave; 2) force > the > > > enrollment flow. > > > > > > I hope that makes sense to you. > > > > Does the bootstrapping involve a code confirmation? > > Currently it does. Why? Sorry, I couldn't figure out how "is-bootstrapping-slave" switch causes the confirmation UI to display. Now I see that.
On 2015/12/05 00:04:15, Alexander Alekseev wrote: > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const bool > is_remora = g_browser_process->platform_part() > > However, if we keep this logic, suppose we try to manually set up a new > > "ChromeBox for meeting" device. > > I don't understand this. Do you mean "by connecting USB keyboard"? > I didn't know this is possible. In this case "is_remora" must be true only if > shark-remora OOBE is in progress. If no shark device is connected, is_slave must > be false. No, actually it's not the case. I think "remora" just stands for "ChromeBox for meeting" (according to achuith@), so basically every "ChromeBox for meeting" is a remora, and you can also manually put a Chrome OS device into the remora mode by pressing the accelerator "Ctrl+Alt+H", which will write the "remora" requisition to the device. So "is_remora" can be true without a shark. > > > During the OOBE flow, because |is_remora| is > > true, EnableLoginLayouts() will not be called, which might violate the case > > described above in 1). > > > > Actually I feel a little confused about this part of logic here. I tried to > > hard-code the |enable_layouts| to |false| and tested on the device, and it > seems > > nothing is affected. It seems the active input methods will be enabled by > > SetApplicationLocale() in the network screen anyway and it doesn't matter > > whether we enable these input methods here. +alemate@ suggestions? > > Did you try selecting any non-Latin languages? Yes, I tried Chinese and Japanese.
https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:287: GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); Could you please move this code to DeviceCloudPolicyManagerChromeOS? DeviceCloudPolicyManagerChromeOS owns the pref and contains some non-trivial logic for changing it. I don't think it's a good idea to change this preference directly from here.
On 2015/12/05 00:23:34, dzhioev wrote: > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wizard_controller.cc:287: > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > Could you please move this code to DeviceCloudPolicyManagerChromeOS? > DeviceCloudPolicyManagerChromeOS owns the pref and contains some non-trivial > logic for changing it. I don't think it's a good idea to change this preference > directly from here. Addressed! Please take another look. Thanks for your review.
On 2015/12/05 00:21:29, xdai1 wrote: > On 2015/12/05 00:04:15, Alexander Alekseev wrote: > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (left): > > > > > https://codereview.chromium.org/1492043002/diff/70001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:211: const > bool > > is_remora = g_browser_process->platform_part() > > > However, if we keep this logic, suppose we try to manually set up a new > > > "ChromeBox for meeting" device. > > > > I don't understand this. Do you mean "by connecting USB keyboard"? > > I didn't know this is possible. In this case "is_remora" must be true only if > > shark-remora OOBE is in progress. If no shark device is connected, is_slave > must > > be false. > > No, actually it's not the case. I think "remora" just stands for "ChromeBox for > meeting" (according to achuith@), so basically every "ChromeBox for meeting" is > a remora, and you can also manually put a Chrome OS device into the remora mode > by pressing the accelerator "Ctrl+Alt+H", which will write the "remora" > requisition to the device. So "is_remora" can be true without a shark. I meant "this |is_remora| flag". So if there is no shark device connected, I think logic should remain as it was before (here). (It is not important for remora device, but this is likely to become "one more special case" here.) > > > During the OOBE flow, because |is_remora| is > > > true, EnableLoginLayouts() will not be called, which might violate the case > > > described above in 1). > > > > > > Actually I feel a little confused about this part of logic here. I tried to > > > hard-code the |enable_layouts| to |false| and tested on the device, and it > > seems > > > nothing is affected. It seems the active input methods will be enabled by > > > SetApplicationLocale() in the network screen anyway and it doesn't matter > > > whether we enable these input methods here. +alemate@ suggestions? > > > > Did you try selecting any non-Latin languages? > > Yes, I tried Chinese and Japanese. I'll verify this by checking if https://crbug.com/358249#c20 is still valid if I cut out these lines later. But for now I still believe there will be an issue.
On 2015/12/05 01:18:03, xdai1 wrote: > On 2015/12/05 00:23:34, dzhioev wrote: > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > > > > https://codereview.chromium.org/1492043002/diff/90001/chrome/browser/chromeos... > > chrome/browser/chromeos/login/wizard_controller.cc:287: > > GetLocalState()->SetBoolean(prefs::kDeviceEnrollmentAutoStart, true); > > Could you please move this code to DeviceCloudPolicyManagerChromeOS? > > DeviceCloudPolicyManagerChromeOS owns the pref and contains some non-trivial > > logic for changing it. I don't think it's a good idea to change this > preference > > directly from here. > > Addressed! Please take another look. Thanks for your review. LGTM
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org Link to the patchset: https://codereview.chromium.org/1492043002/#ps110001 (title: "Address dzhioev@'s comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492043002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492043002/110001
Message was sent while issue was closed.
Description was changed from ========== Introduced switches for Master/Slave bootstrapping process. BUG=564370 ========== to ========== Introduced switches for Master/Slave bootstrapping process. BUG=564370 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Introduced switches for Master/Slave bootstrapping process. BUG=564370 ========== to ========== Introduced switches for Master/Slave bootstrapping process. BUG=564370 Committed: https://crrev.com/7300b9e6caf4851d86b6aaa1e55d9cde8a920324 Cr-Commit-Position: refs/heads/master@{#363646} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7300b9e6caf4851d86b6aaa1e55d9cde8a920324 Cr-Commit-Position: refs/heads/master@{#363646} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
