Mixed Licenses Enrollment

Mixed Licenses Enrollment Flow
Let user select license type during enrollment flow if multiple licenses are available.

BUG=725124
Review-Url: https://codereview.chromium.org/2977033002
Cr-Commit-Position: refs/heads/master@{#491990}
Committed: https://chromium.googlesource.com/chromium/src/+/6233533c9a07a014d1a31079bbb4758992491786

[Denis Kuznetsov (DE-MUC), 2017-07-13 12:06:05]

Description was changed from

==========
Mixed Licenses Enrollment
Flow Let user select license type during enrollment flow if multiple licenses
are available.

BUG=725124
==========

to

==========
Mixed Licenses Enrollment Flow
Let user select license type during enrollment flow if multiple licenses are
available.

BUG=725124
==========

[Denis Kuznetsov (DE-MUC), 2017-07-13 12:06:05]

antrim@chromium.org changed reviewers:
+ emaxx@chromium.org

[emaxx, 2017-07-21 12:57:30]

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:47: const char *
const kMetricEnrollmentTimeCancel =
nit: While you're here - please remove the space before "*"
(as per
https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expre...
)

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:74: const
std::string GetLicenseIdByType(::policy::LicenseType type) {
nit: s/const //
(There's no benefit from making the result returned by value as const.)

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:304: for (auto it
= licenses.begin(); it != licenses.end(); it++) {
nit: Replace with range-based for?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:26:
using EnrollmentLicenseMap = std::map<policy::LicenseType, int>;
nit: #include <map>

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:54: //
and admin allowed customer to choose license type to assign.
nit: s/customer/user/? (IIUC)

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:166:
if (!enrollment_config_.is_mode_attestation()) {
I'm unaware of what the "attestation-based enrollment" is - could you explain
why the license selection shouldn't happen in this case, and why it should
always happen otherwise?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:178:
dcp_initializer->PrepareEnrollment(
Introducing this "half-step" of enrollment preparation obscures the overall
logic around policy initializer.
Would it be possible to pass all the parameters to StartEnrollment() instead -
just as it was before? You'll only have to pass one more additional argument -
optional "license". And won't need to have PrepareEnrollment() and
StartEnrollmentWithLicense().

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:282:
void EnterpriseEnrollmentHelperImpl::OnMultipleLicensesAvailable(
nit: Maybe rename this to something like "OnLicenseMapObtained"?
The current name sounds like there are always multiple licenses, not 0 or 1 -
which isn't the case.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:286:
for (auto it = licenses.begin(); it != licenses.end(); it++) {
nit: Use range-based for?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:192: LOG(ERROR) <<
"Host-paired enrollment is not yet compatible "
nit: #include "base/logging.h"

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:192: LOG(ERROR) <<
"Host-paired enrollment is not yet compatible "
nit: Is this a TODO, or should a separate bug be filed for that?
Or will this be something that won't be realistically implemented?
(To clarify: I have no idea what "host-paired enrollment" is.)

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:125: void
DeviceCloudPolicyInitializer::CheckAvailableLicenses(
This method effectively does nothing now - so is it a TODO or NOTIMPLEMENTED? Or
the uploaded patchset is broken again?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:136:
enrollment_handler_->StartEnrollment();
The license type, received as a parameter, is just dropped in this function. So,
again - should this be TODO or something is wrong with the CL now?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.h (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:59: using
EnrollmentLicenseMap = std::map<::policy::LicenseType, int>;
You have the same definition put into enterprise_enrollment_helper.h too (and
that one is not even belonging to any class).
Would it be possible to get rid of the duplication?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:59: using
EnrollmentLicenseMap = std::map<::policy::LicenseType, int>;
nit: #include <map>

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:59: using
EnrollmentLicenseMap = std::map<::policy::LicenseType, int>;
nit: s/::policy//

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:63:
LicenseSelectionCallback;
nit: Maybe "AvailableLicensesCallback"?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:134:
license_type_(enterprise_management::LicenseType::UNDEFINED),
nit: s/enterprise_management/em/

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:174: if
(!license_selection_callback_.is_null()) {
nit: It's possible to write it simply as:
  if (license_selection_callback_) {

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:182:
CHECK(license_type != ::policy::LicenseType::UNKNOWN);
nit: Use CHECK_NE?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:185: license_type_
= ::enterprise_management::LicenseType::CDM_PERPETUAL;
nit: s/enterprise_management/em/

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:186: return;
This method is called "StartEnrollment...", but does nothing except updating the
data field. Shouldn't the enrollment be started?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:92: void
StartEnrollmentWithLicense(::policy::LicenseType license_type);
nit: s/::policy//

[Denis Kuznetsov (DE-MUC), 2017-07-25 21:51:06]

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:47: const char *
const kMetricEnrollmentTimeCancel =
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: While you're here - please remove the space before "*"
> (as per
>
https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expre...
> )

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:74: const
std::string GetLicenseIdByType(::policy::LicenseType type) {
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: s/const //
> (There's no benefit from making the result returned by value as const.)

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:304: for (auto it
= licenses.begin(); it != licenses.end(); it++) {
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: Replace with range-based for?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:26:
using EnrollmentLicenseMap = std::map<policy::LicenseType, int>;
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: #include <map>

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:54: //
and admin allowed customer to choose license type to assign.
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: s/customer/user/? (IIUC)

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:166:
if (!enrollment_config_.is_mode_attestation()) {
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> I'm unaware of what the "attestation-based enrollment" is - could you explain
> why the license selection shouldn't happen in this case, and why it should
> always happen otherwise?

Attestation-based enrollment is also called "Zero touch", when user
authenticates once on Android device, and then can mass-enroll ChromeOS devices
using that auth instead of entering credentials on each device. As that is done
to avoid interacting with devices, we don't want to show license selection
dialog here.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:178:
dcp_initializer->PrepareEnrollment(
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> Introducing this "half-step" of enrollment preparation obscures the overall
> logic around policy initializer.
> Would it be possible to pass all the parameters to StartEnrollment() instead -
> just as it was before? You'll only have to pass one more additional argument -
> optional "license". And won't need to have PrepareEnrollment() and
> StartEnrollmentWithLicense().
Still it makes semantics much more clear - instead of yet another boolean flag
that changes behavior significantly, you have method names that improves code
readability.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:282:
void EnterpriseEnrollmentHelperImpl::OnMultipleLicensesAvailable(
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: Maybe rename this to something like "OnLicenseMapObtained"?
> The current name sounds like there are always multiple licenses, not 0 or 1 -
> which isn't the case.

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:286:
for (auto it = licenses.begin(); it != licenses.end(); it++) {
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: Use range-based for?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:192: LOG(ERROR) <<
"Host-paired enrollment is not yet compatible "
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> nit: Is this a TODO, or should a separate bug be filed for that?
> Or will this be something that won't be realistically implemented?
> (To clarify: I have no idea what "host-paired enrollment" is.)

This is an outdated branch. It was a plan that user could have keyboardless
device to enroll (e.g. chromebox) that can be controlled from other ChromeOS
device (like tablet) during enrollment. There are no  devices in production now
that have this feature.
There is a special protocol for interaction between two devices, and supporting
Mixed Licenses would require update of this protocol also. This is out of the
scope of this issue.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:125: void
DeviceCloudPolicyInitializer::CheckAvailableLicenses(
On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> This method effectively does nothing now - so is it a TODO or NOTIMPLEMENTED?
Or
> the uploaded patchset is broken again?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:136:
enrollment_handler_->StartEnrollment();
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> The license type, received as a parameter, is just dropped in this function.
So,
> again - should this be TODO or something is wrong with the CL now?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.h (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:59: using
EnrollmentLicenseMap = std::map<::policy::LicenseType, int>;
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: s/::policy//

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:63:
LicenseSelectionCallback;
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: Maybe "AvailableLicensesCallback"?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:134:
license_type_(enterprise_management::LicenseType::UNDEFINED),
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: s/enterprise_management/em/

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:174: if
(!license_selection_callback_.is_null()) {
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: It's possible to write it simply as:
>   if (license_selection_callback_) {

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:182:
CHECK(license_type != ::policy::LicenseType::UNKNOWN);
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: Use CHECK_NE?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:185: license_type_
= ::enterprise_management::LicenseType::CDM_PERPETUAL;
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: s/enterprise_management/em/

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:186: return;
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> This method is called "StartEnrollment...", but does nothing except updating
the
> data field. Shouldn't the enrollment be started?

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:92: void
StartEnrollmentWithLicense(::policy::LicenseType license_type);
On 2017/07/21 12:57:30, emaxx (OOO till July 31st) wrote:
> nit: s/::policy//

Done.

[Denis Kuznetsov (DE-MUC), 2017-07-27 14:46:03]

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

[commit-bot: I haz the power, 2017-07-27 14:46:15]

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-07-27 14:57:46]

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

[commit-bot: I haz the power, 2017-07-27 14:57:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Denis Kuznetsov (DE-MUC), 2017-07-27 18:00:43]

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

[commit-bot: I haz the power, 2017-07-27 18:00:52]

Dry run: CQ is trying da patch.

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

[Denis Kuznetsov (DE-MUC), 2017-07-27 18:09:26]

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

[Denis Kuznetsov (DE-MUC), 2017-07-27 18:09:27]

+achuith, as most of MUC team is OOO.

[commit-bot: I haz the power, 2017-07-27 18:24:50]

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

[commit-bot: I haz the power, 2017-07-27 18:24:51]

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

[achuithb, 2017-07-27 21:05:59]

On 2017/07/27 18:09:27, Denis Kuznetsov (DE-MUC) wrote:
> +achuith, as most of MUC team is OOO.

I think you're missing a fix to cloud_policy_client_unittest.cc

[achuithb, 2017-07-27 21:07:53]

On 2017/07/27 21:05:59, achuithb wrote:
> On 2017/07/27 18:09:27, Denis Kuznetsov (DE-MUC) wrote:
> > +achuith, as most of MUC team is OOO.
> 
> I think you're missing a fix to cloud_policy_client_unittest.cc

Rietveld is going to be read-only very soon. You might want to move this CL over
to gerrit.

[Denis Kuznetsov (DE-MUC), 2017-07-31 13:39:28]

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

[commit-bot: I haz the power, 2017-07-31 13:39:38]

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-07-31 14:33:48]

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

[commit-bot: I haz the power, 2017-07-31 14:33:49]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2017-07-31 14:51:39]

On 2017/07/27 21:05:59, achuithb wrote:
> On 2017/07/27 18:09:27, Denis Kuznetsov (DE-MUC) wrote:
> > +achuith, as most of MUC team is OOO.
> 
> I think you're missing a fix to cloud_policy_client_unittest.cc

Yes, that came under my radar. PTAL.

[achuithb, 2017-07-31 21:03:23]

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:235: auto license
= GetLicenseTypeById(license_type);
I think you should use the actual type here instead of auto

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: CHECK(license
!= ::policy::LicenseType::UNKNOWN);
We should print the license_type in the log here:
<< "license_type = " << license_type;

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:304: for (auto it
: licenses) {
Shouldn't this be const auto&

You can also drop {}

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:286:
for (auto it : licenses) {
Shouldn't this be const auto&

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:196:
EnterpriseEnrollmentHelper::OTHER_ERROR_FATAL);
Could we add a more specific error here?

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:63: typedef
base::Callback<void(const EnrollmentLicenseMap&)>
We prefer using to typedef, and switch EnrollmentCallback as well.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:134:
license_type_(em::LicenseType::UNDEFINED),
Initialize in header

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:174: if
(!available_licenses_callback_) {
Drop {}

is_null?

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:61: typedef
DeviceCloudPolicyInitializer::AvailableLicensesCallback
We prefer using, and switch EnrollmentCallback as well.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:229:
enterprise_management::LicenseType::LicenseTypeEnum license_type_;
Initialize this here

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc
(right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc:50: if
(!enrollment_callback_.is_null()) {
Drop {}

[Denis Kuznetsov (DE-MUC), 2017-08-01 10:50:55]

PTAL

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:235: auto license
= GetLicenseTypeById(license_type);
On 2017/07/31 21:03:23, achuithb wrote:
> I think you should use the actual type here instead of auto

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: CHECK(license
!= ::policy::LicenseType::UNKNOWN);
On 2017/07/31 21:03:22, achuithb wrote:
> We should print the license_type in the log here:
> << "license_type = " << license_type;

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:304: for (auto it
: licenses) {
On 2017/07/31 21:03:22, achuithb wrote:
> Shouldn't this be const auto&
> 
> You can also drop {}

Done.

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:286:
for (auto it : licenses) {
On 2017/07/31 21:03:23, achuithb wrote:
> Shouldn't this be const auto&

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:196:
EnterpriseEnrollmentHelper::OTHER_ERROR_FATAL);
On 2017/07/31 21:03:23, achuithb wrote:
> Could we add a more specific error here?

Well, we have logged detailed explanation above. And having some part of code
not implemented is pretty fatal, we can not recover from it.

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.h:63: typedef
base::Callback<void(const EnrollmentLicenseMap&)>
On 2017/07/31 21:03:23, achuithb wrote:
> We prefer using to typedef, and switch EnrollmentCallback as well.

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:134:
license_type_(em::LicenseType::UNDEFINED),
On 2017/07/31 21:03:23, achuithb wrote:
> Initialize in header

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:174: if
(!available_licenses_callback_) {
On 2017/07/31 21:03:23, achuithb wrote:
> Drop {}

Done

> is_null?

No need, callback_internal.h:107:
  explicit operator bool() const { return !is_null(); }

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

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:61: typedef
DeviceCloudPolicyInitializer::AvailableLicensesCallback
On 2017/07/31 21:03:23, achuithb wrote:
> We prefer using, and switch EnrollmentCallback as well.

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:229:
enterprise_management::LicenseType::LicenseTypeEnum license_type_;
On 2017/07/31 21:03:23, achuithb wrote:
> Initialize this here

Done.

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc
(right):

https://codereview.chromium.org/2977033002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc:50: if
(!enrollment_callback_.is_null()) {
On 2017/07/31 21:03:23, achuithb wrote:
> Drop {}

Done.

[emaxx, 2017-08-01 14:41:39]

Please write tests for the new code. Is that possible in this CL, or for this
you'd want some other pieces wired up?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:166:
if (!enrollment_config_.is_mode_attestation()) {
On 2017/07/25 21:51:05, Denis Kuznetsov (DE-MUC) wrote:
> On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> > I'm unaware of what the "attestation-based enrollment" is - could you
explain
> > why the license selection shouldn't happen in this case, and why it should
> > always happen otherwise?
> 
> Attestation-based enrollment is also called "Zero touch", when user
> authenticates once on Android device, and then can mass-enroll ChromeOS
devices
> using that auth instead of entering credentials on each device. As that is
done
> to avoid interacting with devices, we don't want to show license selection
> dialog here.

Could you please leave this hint as a short comment in the code, e.g.:

// The license selection dialog is not used when doing Zero Touch.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:178:
dcp_initializer->PrepareEnrollment(
On 2017/07/25 21:51:05, Denis Kuznetsov (DE-MUC) wrote:
> On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> > Introducing this "half-step" of enrollment preparation obscures the overall
> > logic around policy initializer.
> > Would it be possible to pass all the parameters to StartEnrollment() instead
-
> > just as it was before? You'll only have to pass one more additional argument
-
> > optional "license". And won't need to have PrepareEnrollment() and
> > StartEnrollmentWithLicense().
> Still it makes semantics much more clear - instead of yet another boolean flag
> that changes behavior significantly, you have method names that improves code
> readability.

Hmm, I'm not sure, maybe we're talking about different things?
I was proposing not a boolean flag that "changes behavior significantly", but
instead passing the license:

void StartEnrollment(
    DeviceManagementService* device_management_service,
    chromeos::ActiveDirectoryJoinDelegate* ad_join_delegate,
    const EnrollmentConfig& enrollment_config,
    const std::string& auth_token,
+   policy::LicenseType license_type,
    const EnrollmentCallback& enrollment_callback);

The benefit is that this would preserve this single point of the enrollment
initiation, instead of spreading this logic over three partial methods.
The semantics would be: if license_type is UNKNOWN, then the old behavior is
selected, otherwise this license is used for the enrollment.
I don't think that those two cases are so much different, that they deserve
separate methods: basically, from what I see, the EnrollmentHandlerChromeOS just
stores this flag and then mostly reuses the general logic.

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: CHECK(license
!= ::policy::LicenseType::UNKNOWN) << "license_type = UNKNOWN";
nit: Or you could just use the CHECK_NE macro, which already contains all the
logging.

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:184: return;
I guess, "break" should be used instead of "return"? With return, no enrollment
is started, only the internal field has to be updated.

A test is needed for this part of code - several mistakes have already been done
in this new code.

[Denis Kuznetsov (DE-MUC), 2017-08-02 18:19:38]

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:166:
if (!enrollment_config_.is_mode_attestation()) {
On 2017/08/01 14:41:39, emaxx wrote:
> On 2017/07/25 21:51:05, Denis Kuznetsov (DE-MUC) wrote:
> > On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> > > I'm unaware of what the "attestation-based enrollment" is - could you
> explain
> > > why the license selection shouldn't happen in this case, and why it should
> > > always happen otherwise?
> > 
> > Attestation-based enrollment is also called "Zero touch", when user
> > authenticates once on Android device, and then can mass-enroll ChromeOS
> devices
> > using that auth instead of entering credentials on each device. As that is
> done
> > to avoid interacting with devices, we don't want to show license selection
> > dialog here.
> 
> Could you please leave this hint as a short comment in the code, e.g.:
> 
> // The license selection dialog is not used when doing Zero Touch.

Done.

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:178:
dcp_initializer->PrepareEnrollment(
On 2017/08/01 14:41:39, emaxx wrote:
> On 2017/07/25 21:51:05, Denis Kuznetsov (DE-MUC) wrote:
> > On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> > > Introducing this "half-step" of enrollment preparation obscures the
overall
> > > logic around policy initializer.
> > > Would it be possible to pass all the parameters to StartEnrollment()
instead
> -
> > > just as it was before? You'll only have to pass one more additional
argument
> -
> > > optional "license". And won't need to have PrepareEnrollment() and
> > > StartEnrollmentWithLicense().
> > Still it makes semantics much more clear - instead of yet another boolean
flag
> > that changes behavior significantly, you have method names that improves
code
> > readability.
> 
> Hmm, I'm not sure, maybe we're talking about different things?
> I was proposing not a boolean flag that "changes behavior significantly", but
> instead passing the license:
> 
> void StartEnrollment(
>     DeviceManagementService* device_management_service,
>     chromeos::ActiveDirectoryJoinDelegate* ad_join_delegate,
>     const EnrollmentConfig& enrollment_config,
>     const std::string& auth_token,
> +   policy::LicenseType license_type,
>     const EnrollmentCallback& enrollment_callback);
> 
> The benefit is that this would preserve this single point of the enrollment
> initiation, instead of spreading this logic over three partial methods.
> The semantics would be: if license_type is UNKNOWN, then the old behavior is
> selected, otherwise this license is used for the enrollment.
> I don't think that those two cases are so much different, that they deserve
> separate methods: basically, from what I see, the EnrollmentHandlerChromeOS
just
> stores this flag and then mostly reuses the general logic.

Picking the license IS the part of enrollment flow, and it happens after
authorization.
If we decide to keep single StartEnrollment, then we would need to keep all
params required for StartEnrollment call until we complete a check. Instead they
are kept inside dcp_initializer.

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:184: return;
On 2017/08/01 14:41:39, emaxx wrote:
> I guess, "break" should be used instead of "return"? With return, no
enrollment
> is started, only the internal field has to be updated.
> 
> A test is needed for this part of code - several mistakes have already been
done
> in this new code.

Acknowledged.

[emaxx, 2017-08-03 18:31:50]

Your last patchset is titled "First test part" - so do you plan to add other
tests into this CL, or you want to follow up with other tests (ideally, with a
browser test which covers the new dcp_initializer functionality) in a separate
CL?

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:178:
dcp_initializer->PrepareEnrollment(
On 2017/08/02 18:19:37, Denis Kuznetsov (DE-MUC) wrote:
> On 2017/08/01 14:41:39, emaxx wrote:
> > On 2017/07/25 21:51:05, Denis Kuznetsov (DE-MUC) wrote:
> > > On 2017/07/21 12:57:29, emaxx (OOO till July 31st) wrote:
> > > > Introducing this "half-step" of enrollment preparation obscures the
> overall
> > > > logic around policy initializer.
> > > > Would it be possible to pass all the parameters to StartEnrollment()
> instead
> > -
> > > > just as it was before? You'll only have to pass one more additional
> argument
> > -
> > > > optional "license". And won't need to have PrepareEnrollment() and
> > > > StartEnrollmentWithLicense().
> > > Still it makes semantics much more clear - instead of yet another boolean
> flag
> > > that changes behavior significantly, you have method names that improves
> code
> > > readability.
> > 
> > Hmm, I'm not sure, maybe we're talking about different things?
> > I was proposing not a boolean flag that "changes behavior significantly",
but
> > instead passing the license:
> > 
> > void StartEnrollment(
> >     DeviceManagementService* device_management_service,
> >     chromeos::ActiveDirectoryJoinDelegate* ad_join_delegate,
> >     const EnrollmentConfig& enrollment_config,
> >     const std::string& auth_token,
> > +   policy::LicenseType license_type,
> >     const EnrollmentCallback& enrollment_callback);
> > 
> > The benefit is that this would preserve this single point of the enrollment
> > initiation, instead of spreading this logic over three partial methods.
> > The semantics would be: if license_type is UNKNOWN, then the old behavior is
> > selected, otherwise this license is used for the enrollment.
> > I don't think that those two cases are so much different, that they deserve
> > separate methods: basically, from what I see, the EnrollmentHandlerChromeOS
> just
> > stores this flag and then mostly reuses the general logic.
> 
> Picking the license IS the part of enrollment flow, and it happens after
> authorization.
> If we decide to keep single StartEnrollment, then we would need to keep all
> params required for StartEnrollment call until we complete a check. Instead
they
> are kept inside dcp_initializer. 

OK.

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc
(right):

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:26: using
testing::NotNull;
nit: Are these "A" and "NotNull" actually used anywhere?

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:270:
EXPECT_CALL(*GetMockScreenView(), Show()).Times(1);
Isn't it also necessary to do VerifyAndClearExpectations() at the right places
(like after "enrollment_screen_->Show()"), so that the expectations not met at
the "checkpoints" make the test fail?

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:273: //
Start zero-touch enrollment.
nit: Is the comment accurate? This same patchset introduces comment "The license
selection dialog is not used when doing Zero Touch.".

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:167: const
policy::CloudPolicyClient::LicenseMap& license_map) {
nit: s/policy:://

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:182: case
::policy::LicenseType::PERPETUAL:
nit: s/::policy:://

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:186: const
policy::CloudPolicyClient::LicenseMap& license_map);
nit: s/policy:://
(sorry for spotting this only now)

[Denis Kuznetsov (DE-MUC), 2017-08-03 20:08:17]

Yes, there will be one more CL with extra tests.

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc
(right):

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:270:
EXPECT_CALL(*GetMockScreenView(), Show()).Times(1);
On 2017/08/03 18:31:49, emaxx wrote:
> Isn't it also necessary to do VerifyAndClearExpectations() at the right places
> (like after "enrollment_screen_->Show()"), so that the expectations not met at
> the "checkpoints" make the test fail?

Other tests in this file do not do that.

Anyway, I will do that in upcoming test CL.

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:273: //
Start zero-touch enrollment.
On 2017/08/03 18:31:49, emaxx wrote:
> nit: Is the comment accurate? This same patchset introduces comment "The
license
> selection dialog is not used when doing Zero Touch.".

Done.

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/2977033002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:186: const
policy::CloudPolicyClient::LicenseMap& license_map);
On 2017/08/03 18:31:50, emaxx wrote:
> nit: s/policy:://
> (sorry for spotting this only now)

Done here and in other places.

[emaxx, 2017-08-03 20:29:13]

LGTM

[achuithb, 2017-08-03 20:30:19]

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

[commit-bot: I haz the power, 2017-08-03 20:30:34]

Dry run: CQ is trying da patch.

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

[achuithb, 2017-08-03 20:53:52]

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:235:
::policy::LicenseType license = GetLicenseTypeById(license_type);
nit const

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: CHECK(license
!= ::policy::LicenseType::UNKNOWN) << "license_type = UNKNOWN";
I was suggesting that you print out the license_type string here

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc
(right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:61: // A
pointer to the EnrollmentScreen.
This comment adds no value

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:81:
ZeroTouchEnrollmentScreenUnitTest() {}
= default

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:229:
MultiLicenseEnrollmentScreenUnitTest() {}
= default

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc:260:
TEST_F(MultiLicenseEnrollmentScreenUnitTest, TestLicenseSelection) {
Add test comment above this line describing this test

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:168:
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
This seems more succinct to me:
check_license_type =
base::CommandLine::ForCurrentProcess()->HasSwitch(chromeos::switches::kEnterpriseEnableLicenseTypeSelection);

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:196:
policy::BrowserPolicyConnectorChromeOS* connector =
Why not eliminate this temporary and simply do:
policy::DeviceCloudPolicyInitializer* dcp_initializer =
g_browser_process->platform_part()->browser_policy_connector_chromeos()->GetDeviceCloudPolicyInitializer();

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:196:
EnterpriseEnrollmentHelper::OTHER_ERROR_FATAL);
This error is incorrect - it is meant for internal code errors, which I don't
think this is. We should introduce a new error type like
MULITPLE_LICENSES_NOT_SUPPORTED or something.

If this requires a new i18n string and you'd like to do it in a follow-up change
in another CL, that's fine.

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc
(right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/fake_device_cloud_policy_initializer.cc:32:
FakeDeviceCloudPolicyInitializer::~FakeDeviceCloudPolicyInitializer() {}
= default

[commit-bot: I haz the power, 2017-08-03 22:33:25]

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

[commit-bot: I haz the power, 2017-08-03 22:33:26]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2017-08-03 23:05:51]

The CQ bit was checked by antrim@chromium.org

[Denis Kuznetsov (DE-MUC), 2017-08-03 23:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2977033002/#ps200001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-08-03 23:06:07]

CQ is trying da patch.

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

[Denis Kuznetsov (DE-MUC), 2017-08-03 23:07:08]

The CQ bit was unchecked by antrim@chromium.org

[Denis Kuznetsov (DE-MUC), 2017-08-03 23:07:52]

addressed most of the nits.

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:196:
EnterpriseEnrollmentHelper::OTHER_ERROR_FATAL);
On 2017/08/03 20:53:51, achuithb wrote:
> This error is incorrect - it is meant for internal code errors, which I don't
> think this is. We should introduce a new error type like
> MULITPLE_LICENSES_NOT_SUPPORTED or something.
> 
> If this requires a new i18n string and you'd like to do it in a follow-up
change
> in another CL, that's fine. 

Ok, will do.

[achuithb, 2017-08-03 23:59:56]

lgtm

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2977033002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:236: CHECK(license
!= ::policy::LicenseType::UNKNOWN) << "license_type = UNKNOWN";
On 2017/08/03 20:53:51, achuithb wrote:
> I was suggesting that you print out the license_type string here

ping

[Denis Kuznetsov (DE-MUC), 2017-08-04 09:11:08]

The CQ bit was checked by antrim@chromium.org

[Denis Kuznetsov (DE-MUC), 2017-08-04 09:11:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2977033002/#ps220001
(title: "Fix last nit")

[commit-bot: I haz the power, 2017-08-04 09:11:18]

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-08-04 09:54:07]

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

[commit-bot: I haz the power, 2017-08-04 09:54:08]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Denis Kuznetsov (DE-MUC), 2017-08-04 10:04:24]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2017-08-04 10:04:37]

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-08-04 11:00:49]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1501841064577210,
"parent_rev": "7fadbd3bad722eebddbca59fe1a3a61a2c5dc654", "commit_rev":
"6233533c9a07a014d1a31079bbb4758992491786"}

[commit-bot: I haz the power, 2017-08-04 11:01:17]

Description was changed from

==========
Mixed Licenses Enrollment Flow
Let user select license type during enrollment flow if multiple licenses are
available.

BUG=725124
==========

to

==========
Mixed Licenses Enrollment Flow
Let user select license type during enrollment flow if multiple licenses are
available.

BUG=725124

Review-Url: https://codereview.chromium.org/2977033002
Cr-Commit-Position: refs/heads/master@{#491990}
Committed:
https://chromium.googlesource.com/chromium/src/+/6233533c9a07a014d1a31079bbb4...
==========

[commit-bot: I haz the power, 2017-08-04 11:01:18]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6233533c9a07a014d1a31079bbb4...