Don't start FRE from the login screen

Don't start FRE from the login screen

Starting FRE from the login screen goes back to the auto-enrollment
days and is not needed anymore (and causes crashes).  Removing that
code path has the benefit of allowing to move the
AutoEnrollmentController from LoginDisplayHost to WizardController.

Also:
- Fold AutoEnrollmentController::Cancel() into ...::Timeout() which had
  become its only caller.
- Add VLOG() to gain insight into the workings of AutoEnrollmentClient.

BUG=703658
Review-Url: https://codereview.chromium.org/2771803003
Cr-Commit-Position: refs/heads/master@{#466031}
Committed: https://chromium.googlesource.com/chromium/src/+/723dbf022b3038d9241fbf7767b46b61c0085e39

[Thiemo Nagel, 2017-03-23 23:35:12]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:36:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-03-23 23:45:51]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:46:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-03-23 23:51:13]

Patchset #1 (id:1) has been deleted

[Thiemo Nagel, 2017-03-23 23:55:59]

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

[Thiemo Nagel, 2017-03-23 23:56:00]

Hi Achuith,

could you please take a look at this cleanup of FRE?

Thank you and kind regards,
Thiemo

[commit-bot: I haz the power, 2017-03-24 00:20:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 00:20:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achuithb, 2017-03-24 01:33:29]

lgtm.

This CL is mainly about auto-enrollment, but has a sprinkling of random things
like fixing comments, and the seemingly unrelated
WizardController::OnNetworkConnected change. I think having a single theme in a
CL makes it easier to review, so this CL may have been better split into two.

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

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:1524: if
(!auto_enrollment_controller_) {
don't need {}

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:395: friend void
WaitForAutoEnrollmentState(policy::AutoEnrollmentState);
Are there examples of friend functions like this? It feels like it might be
better to wrap this in a class.

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1333: //
TODO(dzhioev): Add test emulating device with wrong HWID.
Just drop this TODO since there's no bug id

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1340: //
TODO(dzhioev): Add tests for controller/host pairing flow.
dzhioev, nkostylev, merkulova don't work in chromium any more. We should just
get rid of the names in the TODOs.

[Thiemo Nagel, 2017-04-12 13:29:01]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:29:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-04-12 13:32:28]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 14:00:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 14:00:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Thiemo Nagel, 2017-04-12 14:26:56]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 14:27:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-04-12 14:28:49]

Description was changed from

==========
Don't start FRE from the login screen

Starting FRE from the login screen goes back to the auto-enrollment
days and is not needed anymore (and causes crashes).  Removing that
code path has the benefit of allowing to move the
AutoEnrollmentController from LoginDisplayHost to WizardController.

Also:
- Small refactoring of WizardController::OnNetworkConnected() to merge
  the flows of official and inofficial builds as much as possible which
  is required to properly trigger FRE on a Linux build.
- Fold AutoEnrollmentController::Cancel() into ...::Timeout() which had
  become its only caller.
- Add VLOG() to gain insight into the workings of AutoEnrollmentClient.

BUG=703658
==========

to

==========
Don't start FRE from the login screen

Starting FRE from the login screen goes back to the auto-enrollment
days and is not needed anymore (and causes crashes).  Removing that
code path has the benefit of allowing to move the
AutoEnrollmentController from LoginDisplayHost to WizardController.

Also:
- Fold AutoEnrollmentController::Cancel() into ...::Timeout() which had
  become its only caller.
- Add VLOG() to gain insight into the workings of AutoEnrollmentClient.

BUG=703658
==========

[Thiemo Nagel, 2017-04-12 14:30:40]

Hi Achuith,

sorry for the slow turnaround, I was OOO for a week (and also perf).  I've
removed the unrelated OnNetworkConnected change and addressed your other
comments.  Could you please take another look?

Thank you,
Thiemo

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

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:1524: if
(!auto_enrollment_controller_) {
On 2017/03/24 01:33:29, achuithb wrote:
> don't need {}

Done.

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:395: friend void
WaitForAutoEnrollmentState(policy::AutoEnrollmentState);
On 2017/03/24 01:33:29, achuithb wrote:
> Are there examples of friend functions like this? It feels like it might be
> better to wrap this in a class.

I've counted 244 occurrences:
https://cs.chromium.org/search/?q=friend%5C+.*%5C(+-file:.*.pb.h+-file:third_...

I don't mind either way, though.  Thus I've made the free function a member of
WizardControllerDeviceStateTest.

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1333: //
TODO(dzhioev): Add test emulating device with wrong HWID.
On 2017/03/24 01:33:29, achuithb wrote:
> Just drop this TODO since there's no bug id

Imho, missing a bug number or referencing a person that has left the team
doesn't render a TODO invalid.

https://google.github.io/styleguide/cppguide.html#TODO_Comments

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1340: //
TODO(dzhioev): Add tests for controller/host pairing flow.
On 2017/03/24 01:33:29, achuithb wrote:
> dzhioev, nkostylev, merkulova don't work in chromium any more. We should just
> get rid of the names in the TODOs.

See above.

[commit-bot: I haz the power, 2017-04-12 15:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 15:41:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Thiemo Nagel, 2017-04-12 16:06:46]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 16:07:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 18:12:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 18:12:32]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-04-19 19:04:43]

lgtm

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1333: //
TODO(dzhioev): Add test emulating device with wrong HWID.
On 2017/04/12 14:30:40, Thiemo Nagel wrote:
> On 2017/03/24 01:33:29, achuithb wrote:
> > Just drop this TODO since there's no bug id
> 
> Imho, missing a bug number or referencing a person that has left the team
> doesn't render a TODO invalid.
> 
> https://google.github.io/styleguide/cppguide.html#TODO_Comments

Perhaps the right thing to do would be to file a bug, mark it as GoodFirstBug
and drop this TODO, but let's not worry about it.

https://codereview.chromium.org/2771803003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:1340: //
TODO(dzhioev): Add tests for controller/host pairing flow.
On 2017/04/12 14:30:40, Thiemo Nagel wrote:
> On 2017/03/24 01:33:29, achuithb wrote:
> > dzhioev, nkostylev, merkulova don't work in chromium any more. We should
just
> > get rid of the names in the TODOs.
> 
> See above.

Acknowledged.

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc
(right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc:117: if
(auto_enrollment_controller_->state() ==
Why not do this state check inside Start()?

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
(right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:161: if
(GetMode() == MODE_NONE) {
Maybe add the state check here?

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/auto_enrollment_client.h (right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/auto_enrollment_client.h:39: enum
AutoEnrollmentState {
Maybe not in this CL, but we should consider making this an enum class, and
changing the enum values below to kIdle, kPending, kConnectionError,
kServerError, kTriggerEnrollment, kNoEnrollment.

For logging, it might be nicer to have a function like: 
const std::string& AutoEnrollmentStateString(AutoEnrollmentState state);

Again, not necessary for this CL.

[Thiemo Nagel, 2017-04-20 13:17:26]

Patchset #8 (id:160001) has been deleted

[Thiemo Nagel, 2017-04-20 13:17:34]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 13:17:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-04-20 13:24:00]

Igor, could you please take a look at PS#6 and PS#8 and let me know whether
seems right to you?

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc
(right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc:117: if
(auto_enrollment_controller_->state() ==
On 2017/04/19 19:04:43, achuithb wrote:
> Why not do this state check inside Start()?

Done.

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
(right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:144: //
This method is called at the point in the OOBE/login flow at which the
Deleting this comment since it's incomplete (doesn't mention the Retry() case). 
In general, commenting on things that are happening in a different part of the
code will be outdated or wrong sooner or later and it's probably better to not
do that.

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:161: if
(GetMode() == MODE_NONE) {
On 2017/04/19 19:04:43, achuithb wrote:
> Maybe add the state check here?

I've put it right at the start of the method.

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/auto_enrollment_client.h (right):

https://codereview.chromium.org/2771803003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/auto_enrollment_client.h:39: enum
AutoEnrollmentState {
> Maybe not in this CL, but we should consider making this an enum class, and
> changing the enum values below to kIdle, kPending, kConnectionError,
> kServerError, kTriggerEnrollment, kNoEnrollment.
>
> For logging, it might be nicer to have a function like: 
> const std::string& AutoEnrollmentStateString(AutoEnrollmentState state);
> 
> Again, not necessary for this CL.

I agree that what you suggest is a nicer approach, though why not go all the way
and overload the << operator for logging?

std::ostream& operator<<(std::ostream&, const AutoEnrollmentSate&).

Otoh, adding all the strings bloats the binary and I'm not entirely sure this is
justified.

[commit-bot: I haz the power, 2017-04-20 13:49:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 13:49:18]

Dry run: This issue passed the CQ dry run.

[igorcov, 2017-04-20 16:29:15]

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

[igorcov, 2017-04-20 16:29:16]

lgtm

[Thiemo Nagel, 2017-04-20 16:32:45]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2017-04-20 16:32:45]

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/2771803003/#ps180001
(title: "Improve comments")

[Thiemo Nagel, 2017-04-20 16:33:02]

Thank you Achuith and Igor!

[commit-bot: I haz the power, 2017-04-20 16:33:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 17:10:23]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492705965208170,
"parent_rev": "3f7781da5e43f9c32b5e4654e08b6d5386597fc9", "commit_rev":
"723dbf022b3038d9241fbf7767b46b61c0085e39"}

[commit-bot: I haz the power, 2017-04-20 17:10:35]

Description was changed from

==========
Don't start FRE from the login screen

Starting FRE from the login screen goes back to the auto-enrollment
days and is not needed anymore (and causes crashes).  Removing that
code path has the benefit of allowing to move the
AutoEnrollmentController from LoginDisplayHost to WizardController.

Also:
- Fold AutoEnrollmentController::Cancel() into ...::Timeout() which had
  become its only caller.
- Add VLOG() to gain insight into the workings of AutoEnrollmentClient.

BUG=703658
==========

to

==========
Don't start FRE from the login screen

Starting FRE from the login screen goes back to the auto-enrollment
days and is not needed anymore (and causes crashes).  Removing that
code path has the benefit of allowing to move the
AutoEnrollmentController from LoginDisplayHost to WizardController.

Also:
- Fold AutoEnrollmentController::Cancel() into ...::Timeout() which had
  become its only caller.
- Add VLOG() to gain insight into the workings of AutoEnrollmentClient.

BUG=703658

Review-Url: https://codereview.chromium.org/2771803003
Cr-Commit-Position: refs/heads/master@{#466031}
Committed:
https://chromium.googlesource.com/chromium/src/+/723dbf022b3038d9241fbf7767b4...
==========

[commit-bot: I haz the power, 2017-04-20 17:10:36]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/723dbf022b3038d9241fbf7767b4...