Redirect to the enterprise enrollment screen during remora and shark pairing

Redirect to the enterprise enrollment screen during remora and shark pairing.

BUG=381007
Committed: https://crrev.com/11bb859312e520f71a291971bb2ee7bcb30a9a43
Cr-Commit-Position: refs/heads/master@{#294232}

[achuithb, 2014-09-05 00:26:23]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2014-09-05 00:26:23]

Great, I like this.

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:41: const
std::string& auth_token,
Maybe we should just pass in a callback instead:
base::Bind(remora_controller,
&ControllerPairingController::OnAuthenticationDone, "", auth_token)

that way we don't need to keep track of the auth_token or the controller.

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:104:
pairing_chromeos::ControllerPairingController* remora_controller_;
Maybe move this up next to actor_?

[Zachary Kuznia, 2014-09-09 00:51:53]

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:41: const
std::string& auth_token,
On 2014/09/05 00:26:23, achuithb wrote:
> Maybe we should just pass in a callback instead:
> base::Bind(remora_controller,
> &ControllerPairingController::OnAuthenticationDone, "", auth_token)
> 
> that way we don't need to keep track of the auth_token or the controller.

auth_token_ and the controller are actually mutually exclusive; one is used for
remora and the other for shark.

https://codereview.chromium.org/547503002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:104:
pairing_chromeos::ControllerPairingController* remora_controller_;
On 2014/09/05 00:26:23, achuithb wrote:
> Maybe move this up next to actor_?

Done.

[Zachary Kuznia, 2014-09-09 01:08:56]

zork@chromium.org changed reviewers:
+ dzhioev@chromium.org

[dzhioev (left Google), 2014-09-09 01:35:41]

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
a) Why do you use ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED code?
b) Shouldn't we just reboot host at this point?

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:320:
remora_controller_->SetEnrollmentComplete(true);
Why do you assume everywhere that |remora_controller_| is not null?

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/controller_pairing_screen.cc:110: case
ControllerPairingController::STAGE_WAITING_FOR_CREDENTIALS: {
You need to remove |this| from |controller_|'s observers here.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:126: auth_token);
You need to remove |this| from |controller_|'s observers here.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:639:
ShowAutoEnrollmentCheckScreen();
Why not ShowEnrollmentScreen()?

[dzhioev (left Google), 2014-09-09 01:58:09]

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:124: case
HostPairingController::STAGE_ENROLLING:
nit: you can replace all these cases with a single "default" section.

[Zachary Kuznia, 2014-09-09 02:17:51]

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:124: case
HostPairingController::STAGE_ENROLLING:
On 2014/09/09 01:58:09, dzhioev wrote:
> nit: you can replace all these cases with a single "default" section.

When switching on enums, I leave off the default case so that the compiler will
catch missing statements.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
On 2014/09/09 01:35:40, dzhioev wrote:
> a) Why do you use ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED code?
> b) Shouldn't we just reboot host at this point? 

a: I reused the exit code from elsewhere in this file that fit our use case.
b: I don't think we need the reboot until we fix the flow to defer the update.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:320:
remora_controller_->SetEnrollmentComplete(true);
On 2014/09/09 01:35:40, dzhioev wrote:
> Why do you assume everywhere that |remora_controller_| is not null?

That is a bad assumption on my part.  Fixed.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/controller_pairing_screen.cc:110: case
ControllerPairingController::STAGE_WAITING_FOR_CREDENTIALS: {
On 2014/09/09 01:35:41, dzhioev wrote:
> You need to remove |this| from |controller_|'s observers here.

Done.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:126: auth_token);
On 2014/09/09 01:35:41, dzhioev wrote:
> You need to remove |this| from |controller_|'s observers here.

Done.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:639:
ShowAutoEnrollmentCheckScreen();
On 2014/09/09 01:35:41, dzhioev wrote:
> Why not ShowEnrollmentScreen()?

Misread the logic in OnUpdateCompleted().  Fixed.

[dzhioev (left Google), 2014-09-09 02:46:51]

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
On 2014/09/09 02:17:51, Zachary Kuznia wrote:
> On 2014/09/09 01:35:40, dzhioev wrote:
> > a) Why do you use ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED code?
> > b) Shouldn't we just reboot host at this point? 
> 
> a: I reused the exit code from elsewhere in this file that fit our use case.
> b: I don't think we need the reboot until we fix the flow to defer the update.

What do you see on host's screen after enrollment is done? I thought our case is
to reboot and launch a kiosk app.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:61: if
(remora_controller_) {
nit: remove {}

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:79: if
(remora_controller_) {
nit: remove {}

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:315: if
(remora_controller_) {
ditto

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:392: if
(remora_controller_) {
ditto

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.cc:35:
controller_->RemoveObserver(this);
nit: it is safe to try to remove observer, that is not in a list, so no need for
|if (observing_controller_)| check.

[achuithb, 2014-09-09 09:02:03]

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:117: switch
(new_stage) {
Can't we just replace this switch with
if (new_stage == STAGE_FINISHED) {
  RemoveObserver();
  OnExit();
}

It just seems like a lot of lines of code that do nothing, especially since we
don't run debug builds of chrome on devices.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:147: const
std::string& keyboard_layout) {}
Shouldn't the } be on a new line?

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: void
EnrollmentScreen::EnrollHost(const std::string& auth_token) {}
same

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:44: void
SetParameters(
Let's add a comment explaining what these parameters are (or at least the ones
we've added).

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.h:58: bool
observing_controller_;
Looks like we don't need this? It's only used before calling RemoveObserver
right?

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:56: bool
observing_controller_;
ditto - let's get rid of this.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:633: // TODO(achuith, zork):
This will be called via Bluetooth from a remote
Don't need the TODO any more.

[achuithb, 2014-09-09 12:59:43]

Let's also update the description with the proper BUG=

[Zachary Kuznia, 2014-09-09 23:53:58]

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
> What do you see on host's screen after enrollment is done? I thought our case
is
> to reboot and launch a kiosk app.

After enrollment, the host launches Hangouts.

achuith: Is this the proper exit status to use here, or is something else
better?

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:61: if
(remora_controller_) {
On 2014/09/09 02:46:50, dzhioev wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:79: if
(remora_controller_) {
On 2014/09/09 02:46:51, dzhioev wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:117: switch
(new_stage) {
On 2014/09/09 09:02:02, achuithb wrote:
> Can't we just replace this switch with
> if (new_stage == STAGE_FINISHED) {
>   RemoveObserver();
>   OnExit();
> }
> 
> It just seems like a lot of lines of code that do nothing, especially since we
> don't run debug builds of chrome on devices.

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:147: const
std::string& keyboard_layout) {}
On 2014/09/09 09:02:02, achuithb wrote:
> Shouldn't the } be on a new line?

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:149: void
EnrollmentScreen::EnrollHost(const std::string& auth_token) {}
On 2014/09/09 09:02:02, achuithb wrote:
> same

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:315: if
(remora_controller_) {
On 2014/09/09 02:46:51, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:392: if
(remora_controller_) {
On 2014/09/09 02:46:50, dzhioev wrote:
> ditto

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:44: void
SetParameters(
On 2014/09/09 09:02:02, achuithb wrote:
> Let's add a comment explaining what these parameters are (or at least the ones
> we've added).

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.cc:35:
controller_->RemoveObserver(this);
On 2014/09/09 02:46:51, dzhioev wrote:
> nit: it is safe to try to remove observer, that is not in a list, so no need
for
> |if (observing_controller_)| check.

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/controller_pairing_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/controller_pairing_screen.h:58: bool
observing_controller_;
On 2014/09/09 09:02:02, achuithb wrote:
> Looks like we don't need this? It's only used before calling RemoveObserver
> right?

Aye, I thought it asserted there was an observer.  Removed.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:56: bool
observing_controller_;
On 2014/09/09 09:02:03, achuithb wrote:
> ditto - let's get rid of this.

Done.

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/547503002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:633: // TODO(achuith, zork):
This will be called via Bluetooth from a remote
On 2014/09/09 09:02:03, achuithb wrote:
> Don't need the TODO any more.

Done.

[achuithb, 2014-09-10 08:04:20]

Please update the BUG= and the description fields.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
On 2014/09/09 23:53:58, Zachary Kuznia wrote:
> > What do you see on host's screen after enrollment is done? I thought our
case
> is
> > to reboot and launch a kiosk app.
> 
> After enrollment, the host launches Hangouts.
> 
> achuith: Is this the proper exit status to use here, or is something else
> better?

We can do this, or use ENTERPRISE_ENROLLMENT_COMPLETED, or have a new enum for
the purpose of this remote remora enrollment. This particular enum is used for
forced re-enrollment. I think it's fine to use here for now. You could also ask
Bartosz if he has an opinion on this.

https://codereview.chromium.org/547503002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:47: //     remora
device to synchronize enrollment.
Nit: s/to synchronize enrollment/for remote enrollment/
I prefer this language over the word synchronize which has other meanings.

[Zachary Kuznia, 2014-09-10 15:57:26]

Bug and description updated.

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/547503002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:136:
WizardController::ENTERPRISE_AUTO_MAGIC_ENROLLMENT_COMPLETED);
> We can do this, or use ENTERPRISE_ENROLLMENT_COMPLETED, or have a new enum for
> the purpose of this remote remora enrollment. This particular enum is used for
> forced re-enrollment. I think it's fine to use here for now. You could also
ask
> Bartosz if he has an opinion on this. 

Ok, I'll leave this as is for now.  I've filed a bug and added a comment.

https://codereview.chromium.org/547503002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:47: //     remora
device to synchronize enrollment.
On 2014/09/10 08:04:20, achuithb wrote:
> Nit: s/to synchronize enrollment/for remote enrollment/
> I prefer this language over the word synchronize which has other meanings.

Done.

[achuithb, 2014-09-10 18:13:37]

lgtm

https://codereview.chromium.org/547503002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:47: //     remora
device to synchronize enrollment.
Nit: Here too please.
s/to synchronize/for remote/

[Zachary Kuznia, 2014-09-10 20:01:19]

https://codereview.chromium.org/547503002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.h (right):

https://codereview.chromium.org/547503002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.h:47: //     remora
device to synchronize enrollment.
On 2014/09/10 18:13:37, achuithb wrote:
> Nit: Here too please.
> s/to synchronize/for remote/

Done.

[Zachary Kuznia, 2014-09-10 20:01:23]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-09-10 20:17:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/547503002/160001

[commit-bot: I haz the power, 2014-09-10 21:20:16]

Committed patchset #9 (id:160001) as 90b3aae834187f3d7bca1b8f7e3938e19f718a24

[commit-bot: I haz the power, 2014-09-10 21:25:38]

Patchset 9 (id:??) landed as
https://crrev.com/11bb859312e520f71a291971bb2ee7bcb30a9a43
Cr-Commit-Position: refs/heads/master@{#294232}