SetManagementSettings() is moved to OwnerSettingsServiceChromeOS.

SetManagementSettings() is moved to OwnerSettingsServiceChromeOS.
Also, removed SignAndStore() functionality from DeviceSettingsService.

BUG=433840
TEST=unit_tests:DeviceSettingsService*

Committed: https://crrev.com/3b6e2b7d67086528afb759be65aaed7822496863
Cr-Commit-Position: refs/heads/master@{#307017}

[ygorshenin1, 2014-12-01 15:03:03]

ygorshenin@chromium.org changed reviewers:
+ davidyu@chromium.org, mnissler@chromium.org

[Mattias Nissler (ping if slow), 2014-12-01 15:25:41]

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:152: void
DeviceSettingsService::InitOwner(
Now that we no longer need to know the owner's username here, we should probably
remove this, as well as |username_|, |owner_settings_service_| and
HasPrivateOwnerKey().

Probably not in this CL though (I haven't checked the above for how much churn
it'll generate).

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service_unittest.cc
(right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service_unittest.cc:178:
base::Unretained(this));
It seems like these tests should rather migrate over to
owner_settings_service_chromeos_unittest.cc?

[ygorshenin1, 2014-12-01 15:59:09]

PTAL

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:152: void
DeviceSettingsService::InitOwner(
On 2014/12/01 15:25:41, Mattias Nissler wrote:
> Now that we no longer need to know the owner's username here, we should
probably
> remove this, as well as |username_|, |owner_settings_service_| and
> HasPrivateOwnerKey().
> 
> Probably not in this CL though (I haven't checked the above for how much churn
> it'll generate).

Sounds good, but currently we can't do that because device settings write path
still depends on OwnerSettingsService and username. This could be done when
we'll completely get rid of the old write path.

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service_unittest.cc
(right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service_unittest.cc:178:
base::Unretained(this));
On 2014/12/01 15:25:41, Mattias Nissler wrote:
> It seems like these tests should rather migrate over to
> owner_settings_service_chromeos_unittest.cc?

Good point, thanks!

[davidyu, 2014-12-01 16:45:33]

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
Is there any case that |user| might be null at this point? If not, we probably
should CHECK(user). Same for CHECK(profile) and CHECK(service).

[Mattias Nissler (ping if slow), 2014-12-02 08:52:40]

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:152: void
DeviceSettingsService::InitOwner(
On 2014/12/01 15:59:09, ygorshenin1 wrote:
> On 2014/12/01 15:25:41, Mattias Nissler wrote:
> > Now that we no longer need to know the owner's username here, we should
> probably
> > remove this, as well as |username_|, |owner_settings_service_| and
> > HasPrivateOwnerKey().
> > 
> > Probably not in this CL though (I haven't checked the above for how much
churn
> > it'll generate).
> 
> Sounds good, but currently we can't do that because device settings write path
> still depends on OwnerSettingsService and username. This could be done when
> we'll completely get rid of the old write path.

Good point. Can you add a TODO to the header that declares the respective data
members to call out that they're deprecated and what our removal plan is for
them?

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:670:
current_mode = policy->management_mode();
I think the preceding 3 lines should use David's new GetManagementMode helper
introduced in https://codereview.chromium.org/742513006/

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:162:
pending_management_settings_set_requests_;
I think we only ever need to store one of these. If there's another change
coming in, we'll just overwrite anyways.

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:359: const
user_manager::User* user = UserManager::Get()->FindUser(username_);
Question for David: Are there any flows that will require consumer enrollment to
happen during OOBE while we haven't created the owner's user account yet?

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/01 16:45:33, davidyu wrote:
> Is there any case that |user| might be null at this point? If not, we probably
> should CHECK(user). Same for CHECK(profile) and CHECK(service).

It might be better to just pass the OwnerSettingsServiceChromeOS in the ctor.
Or, given that we have quite some if-management-mode-something business already
in this file, split into separate classes for consumer and enterprise enrollment
to reduce confusion. Not in this CL, but let's discuss where we want to take
this and document the decision per TODO comments. David, WDYT?

[davidyu, 2014-12-02 08:59:04]

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:359: const
user_manager::User* user = UserManager::Get()->FindUser(username_);
On 2014/12/02 08:52:40, Mattias Nissler wrote:
> Question for David: Are there any flows that will require consumer enrollment
to
> happen during OOBE while we haven't created the owner's user account yet?

No. Consumer manager enrollment during OOBE is currently not in our plan.

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/02 08:52:40, Mattias Nissler wrote:
> On 2014/12/01 16:45:33, davidyu wrote:
> > Is there any case that |user| might be null at this point? If not, we
probably
> > should CHECK(user). Same for CHECK(profile) and CHECK(service).
> 
> It might be better to just pass the OwnerSettingsServiceChromeOS in the ctor.
> Or, given that we have quite some if-management-mode-something business
already
> in this file, split into separate classes for consumer and enterprise
enrollment
> to reduce confusion. Not in this CL, but let's discuss where we want to take
> this and document the decision per TODO comments. David, WDYT?

The current logic is wrong for consumer enrollment. I think it needs to be
fixed. SetManagementSettings is not an optional step for consumer management. We
should do some error handling here instead of ignoring it silently.

I think passing OwnerSettingsServiceChromeOS in the ctor is fine.

[ygorshenin1, 2014-12-02 19:18:58]

PTAL

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/769703003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:152: void
DeviceSettingsService::InitOwner(
On 2014/12/02 08:52:40, Mattias Nissler wrote:
> On 2014/12/01 15:59:09, ygorshenin1 wrote:
> > On 2014/12/01 15:25:41, Mattias Nissler wrote:
> > > Now that we no longer need to know the owner's username here, we should
> > probably
> > > remove this, as well as |username_|, |owner_settings_service_| and
> > > HasPrivateOwnerKey().
> > > 
> > > Probably not in this CL though (I haven't checked the above for how much
> churn
> > > it'll generate).
> > 
> > Sounds good, but currently we can't do that because device settings write
path
> > still depends on OwnerSettingsService and username. This could be done when
> > we'll completely get rid of the old write path.
> 
> Good point. Can you add a TODO to the header that declares the respective data
> members to call out that they're deprecated and what our removal plan is for
> them?

Done.

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:670:
current_mode = policy->management_mode();
On 2014/12/02 08:52:40, Mattias Nissler wrote:
> I think the preceding 3 lines should use David's new GetManagementMode helper
> introduced in https://codereview.chromium.org/742513006/

Done.

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:162:
pending_management_settings_set_requests_;
On 2014/12/02 08:52:40, Mattias Nissler wrote:
> I think we only ever need to store one of these. If there's another change
> coming in, we'll just overwrite anyways.

Done.

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/02 08:59:04, davidyu wrote:
> On 2014/12/02 08:52:40, Mattias Nissler wrote:
> > On 2014/12/01 16:45:33, davidyu wrote:
> > > Is there any case that |user| might be null at this point? If not, we
> probably
> > > should CHECK(user). Same for CHECK(profile) and CHECK(service).
> > 
> > It might be better to just pass the OwnerSettingsServiceChromeOS in the
ctor.
> > Or, given that we have quite some if-management-mode-something business
> already
> > in this file, split into separate classes for consumer and enterprise
> enrollment
> > to reduce confusion. Not in this CL, but let's discuss where we want to take
> > this and document the decision per TODO comments. David, WDYT?
> 
> The current logic is wrong for consumer enrollment. I think it needs to be
> fixed. SetManagementSettings is not an optional step for consumer management.
We
> should do some error handling here instead of ignoring it silently.
> 
> I think passing OwnerSettingsServiceChromeOS in the ctor is fine. 

OwnerSettingsServiceChromeOS here corresponds to user with user id equals to
username_, which is set in
EnrollmentHandlerChromeOS::HandlePolicyValidationResult(). How to get correct
instance of OwnerSettingsServiceChromeOS before username is known, at the time
of EnrollmentHandlerChromeOS instantiation?

[davidyu, 2014-12-03 08:44:02]

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/02 19:18:58, ygorshenin1 wrote:
> On 2014/12/02 08:59:04, davidyu wrote:
> > On 2014/12/02 08:52:40, Mattias Nissler wrote:
> > > On 2014/12/01 16:45:33, davidyu wrote:
> > > > Is there any case that |user| might be null at this point? If not, we
> > probably
> > > > should CHECK(user). Same for CHECK(profile) and CHECK(service).
> > > 
> > > It might be better to just pass the OwnerSettingsServiceChromeOS in the
> ctor.
> > > Or, given that we have quite some if-management-mode-something business
> > already
> > > in this file, split into separate classes for consumer and enterprise
> > enrollment
> > > to reduce confusion. Not in this CL, but let's discuss where we want to
take
> > > this and document the decision per TODO comments. David, WDYT?
> > 
> > The current logic is wrong for consumer enrollment. I think it needs to be
> > fixed. SetManagementSettings is not an optional step for consumer
management.
> We
> > should do some error handling here instead of ignoring it silently.
> > 
> > I think passing OwnerSettingsServiceChromeOS in the ctor is fine. 
> 
> OwnerSettingsServiceChromeOS here corresponds to user with user id equals to
> username_, which is set in
> EnrollmentHandlerChromeOS::HandlePolicyValidationResult(). How to get correct
> instance of OwnerSettingsServiceChromeOS before username is known, at the time
> of EnrollmentHandlerChromeOS instantiation?

I took another look. Since the service is only used for consumer management, we
can pass the service from
ConsumerEnrollmentHandler::OnOwnerAccessTokenAvailable() to
DeviceCloudPolicyInitializer::StartEnrollment() to the constructor. In
ConsumerEnrollmentHandler, you can just use |profile_| to get the service. For
enterprise flow, you can simply pass nullptr, as it is not used.

I am not sure if we should have a separate enrollment handler class for
consumer, as this is the only one step that needs to be handled differently from
the enterprise flow. Other steps still can be shared.

[Mattias Nissler (ping if slow), 2014-12-03 09:07:55]

Looks good mostly (pending the resolution of the OwnerSettingsServiceChromeOS
lookup discussion).

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/03 08:44:02, davidyu wrote:
> On 2014/12/02 19:18:58, ygorshenin1 wrote:
> > On 2014/12/02 08:59:04, davidyu wrote:
> > > On 2014/12/02 08:52:40, Mattias Nissler wrote:
> > > > On 2014/12/01 16:45:33, davidyu wrote:
> > > > > Is there any case that |user| might be null at this point? If not, we
> > > probably
> > > > > should CHECK(user). Same for CHECK(profile) and CHECK(service).
> > > > 
> > > > It might be better to just pass the OwnerSettingsServiceChromeOS in the
> > ctor.
> > > > Or, given that we have quite some if-management-mode-something business
> > > already
> > > > in this file, split into separate classes for consumer and enterprise
> > > enrollment
> > > > to reduce confusion. Not in this CL, but let's discuss where we want to
> take
> > > > this and document the decision per TODO comments. David, WDYT?
> > > 
> > > The current logic is wrong for consumer enrollment. I think it needs to be
> > > fixed. SetManagementSettings is not an optional step for consumer
> management.
> > We
> > > should do some error handling here instead of ignoring it silently.
> > > 
> > > I think passing OwnerSettingsServiceChromeOS in the ctor is fine. 
> > 
> > OwnerSettingsServiceChromeOS here corresponds to user with user id equals to
> > username_, which is set in
> > EnrollmentHandlerChromeOS::HandlePolicyValidationResult(). How to get
correct
> > instance of OwnerSettingsServiceChromeOS before username is known, at the
time
> > of EnrollmentHandlerChromeOS instantiation?
> 
> I took another look. Since the service is only used for consumer management,
we
> can pass the service from
> ConsumerEnrollmentHandler::OnOwnerAccessTokenAvailable() to
> DeviceCloudPolicyInitializer::StartEnrollment() to the constructor. In
> ConsumerEnrollmentHandler, you can just use |profile_| to get the service. For
> enterprise flow, you can simply pass nullptr, as it is not used.

This solution seems fine for now.

> 
> I am not sure if we should have a separate enrollment handler class for
> consumer, as this is the only one step that needs to be handled differently
from
> the enterprise flow. Other steps still can be shared.

There are three tests for |management_mode_ == MANAGEMENT_MODE_CONSUMER_MANAGED|
in the code already (and a fourth is missing per my other comment). I'm fine
with not forking just yet, but if we keep adding more special-casing it doesn't
make sense to pretend enterprise and consumer are the same thing IMHO.

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(left):

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:180: 
I don't think this change is intentional?

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:217: if
(enrollment_step_ == STEP_STORE_TOKEN_AND_ID) {
Side note: This bailout path should only be enabled for management_mode ==
MANAGEMENT_MODE_CONSUMER_MANAGED.

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.h:162: // to
OwnerSettingsServiceCHromeOS.
nit: lower-case H

[davidyu, 2014-12-03 09:13:53]

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/769703003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:361: user ?
chromeos::ProfileHelper::Get()->GetProfileByUser(user) : nullptr;
On 2014/12/03 09:07:54, Mattias Nissler wrote:
> On 2014/12/03 08:44:02, davidyu wrote:
> > On 2014/12/02 19:18:58, ygorshenin1 wrote:
> > > On 2014/12/02 08:59:04, davidyu wrote:
> > > > On 2014/12/02 08:52:40, Mattias Nissler wrote:
> > > > > On 2014/12/01 16:45:33, davidyu wrote:
> > > > > > Is there any case that |user| might be null at this point? If not,
we
> > > > probably
> > > > > > should CHECK(user). Same for CHECK(profile) and CHECK(service).
> > > > > 
> > > > > It might be better to just pass the OwnerSettingsServiceChromeOS in
the
> > > ctor.
> > > > > Or, given that we have quite some if-management-mode-something
business
> > > > already
> > > > > in this file, split into separate classes for consumer and enterprise
> > > > enrollment
> > > > > to reduce confusion. Not in this CL, but let's discuss where we want
to
> > take
> > > > > this and document the decision per TODO comments. David, WDYT?
> > > > 
> > > > The current logic is wrong for consumer enrollment. I think it needs to
be
> > > > fixed. SetManagementSettings is not an optional step for consumer
> > management.
> > > We
> > > > should do some error handling here instead of ignoring it silently.
> > > > 
> > > > I think passing OwnerSettingsServiceChromeOS in the ctor is fine. 
> > > 
> > > OwnerSettingsServiceChromeOS here corresponds to user with user id equals
to
> > > username_, which is set in
> > > EnrollmentHandlerChromeOS::HandlePolicyValidationResult(). How to get
> correct
> > > instance of OwnerSettingsServiceChromeOS before username is known, at the
> time
> > > of EnrollmentHandlerChromeOS instantiation?
> > 
> > I took another look. Since the service is only used for consumer management,
> we
> > can pass the service from
> > ConsumerEnrollmentHandler::OnOwnerAccessTokenAvailable() to
> > DeviceCloudPolicyInitializer::StartEnrollment() to the constructor. In
> > ConsumerEnrollmentHandler, you can just use |profile_| to get the service.
For
> > enterprise flow, you can simply pass nullptr, as it is not used.
> 
> This solution seems fine for now.
> 
> > 
> > I am not sure if we should have a separate enrollment handler class for
> > consumer, as this is the only one step that needs to be handled differently
> from
> > the enterprise flow. Other steps still can be shared.
> 
> There are three tests for |management_mode_ ==
MANAGEMENT_MODE_CONSUMER_MANAGED|
> in the code already (and a fourth is missing per my other comment). I'm fine
> with not forking just yet, but if we keep adding more special-casing it
doesn't
> make sense to pretend enterprise and consumer are the same thing IMHO.

Ah. you are right. I miscounted. I think it makes sense to have a separate class
then.

[ygorshenin1, 2014-12-03 14:59:50]

Patchset #5 (id:80001) has been deleted

[ygorshenin1, 2014-12-03 15:50:15]

Patchset #6 (id:120001) has been deleted

[ygorshenin1, 2014-12-03 15:50:20]

Patchset #5 (id:100001) has been deleted

[ygorshenin1, 2014-12-03 15:54:05]

I've added OwnerSettingsServiceChromeOS* to the list of
EnrollmentHandlerChromeOS ctor's arguments.

PTAL

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(left):

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:180: 
On 2014/12/03 09:07:54, Mattias Nissler wrote:
> I don't think this change is intentional?

Sorry, I've removed it occasionally. Restored.

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/769703003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.h:162: // to
OwnerSettingsServiceCHromeOS.
On 2014/12/03 09:07:54, Mattias Nissler wrote:
> nit: lower-case H

Done.

[davidyu, 2014-12-03 16:06:04]

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:171:
nullptr /* owner_settings_service */, token, is_auto_enrollment(),
Put each parameter at its own line.

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/consumer_enrollment_handler.cc (right):

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/consumer_enrollment_handler.cc:114:
MANAGEMENT_MODE_CONSUMER_MANAGED, device_management_service_,
Put each parameter at its own line.

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:110:
device_store_, install_attributes_, state_keys_broker_,
Put each parameter at its own line.

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

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:354:
enrollment_step_ = STEP_STORE_TOKEN_AND_ID;
CHECK(owner_settings_service_);

[ygorshenin1, 2014-12-04 10:21:47]

PTAL

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:171:
nullptr /* owner_settings_service */, token, is_auto_enrollment(),
On 2014/12/03 16:06:04, davidyu wrote:
> Put each parameter at its own line.

This formatting is made by git cl format (actually, clang-format) tool, and this
tool is quite popular among Chromium developers, so, I'd prefer to leave it's
formatting as is, if you don't mind. WDYT?

[ygorshenin1, 2014-12-04 10:27:24]

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

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:354:
enrollment_step_ = STEP_STORE_TOKEN_AND_ID;
On 2014/12/03 16:06:04, davidyu wrote:
> CHECK(owner_settings_service_);

Done.

[davidyu, 2014-12-04 10:29:24]

lgtm

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/769703003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:171:
nullptr /* owner_settings_service */, token, is_auto_enrollment(),
On 2014/12/04 10:21:47, ygorshenin1 wrote:
> On 2014/12/03 16:06:04, davidyu wrote:
> > Put each parameter at its own line.
> 
> This formatting is made by git cl format (actually, clang-format) tool, and
this
> tool is quite popular among Chromium developers, so, I'd prefer to leave it's
> formatting as is, if you don't mind. WDYT?

Ah, okay, then.

[Mattias Nissler (ping if slow), 2014-12-04 13:05:46]

LGTM w/ nits.

https://codereview.chromium.org/769703003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/769703003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:726: for
(const auto& callback : pending_management_settings_callbacks_) {
Suggestion: Declare a temporary callbacks vector, swap with
|pending_management_settings_callbacks_|. This will prevent issues when the
callback calls SetManagementSettings again and thus modifies
pending_management_settings_callbacks_.

https://codereview.chromium.org/769703003/diff/160001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.cc (right):

https://codereview.chromium.org/769703003/diff/160001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.cc:123: default:
I would prefer moving the NOTREACHED after the switch and removing the default
case label, so the compiler flags this switch statement to require updating when
somebody adds a new mode.

[ygorshenin1, 2014-12-05 09:31:48]

Many thanks, David and Mattias!

https://codereview.chromium.org/769703003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/769703003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:726: for
(const auto& callback : pending_management_settings_callbacks_) {
On 2014/12/04 13:05:46, Mattias Nissler wrote:
> Suggestion: Declare a temporary callbacks vector, swap with
> |pending_management_settings_callbacks_|. This will prevent issues when the
> callback calls SetManagementSettings again and thus modifies
> pending_management_settings_callbacks_.

Thanks for the suggestion! Done.

https://codereview.chromium.org/769703003/diff/160001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.cc (right):

https://codereview.chromium.org/769703003/diff/160001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.cc:123: default:
On 2014/12/04 13:05:46, Mattias Nissler wrote:
> I would prefer moving the NOTREACHED after the switch and removing the default
> case label, so the compiler flags this switch statement to require updating
when
> somebody adds a new mode.

Done.

[ygorshenin1, 2014-12-05 09:31:54]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-12-05 09:32:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769703003/180001

[commit-bot: I haz the power, 2014-12-05 09:35:51]

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

[commit-bot: I haz the power, 2014-12-05 09:35:52]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/90369)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[ygorshenin1, 2014-12-05 10:01:07]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-12-05 10:01:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769703003/200001

[commit-bot: I haz the power, 2014-12-05 10:53:10]

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

[commit-bot: I haz the power, 2014-12-05 10:53:11]

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

[ygorshenin1, 2014-12-05 12:35:51]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-12-05 12:36:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/769703003/220001

[commit-bot: I haz the power, 2014-12-05 13:23:19]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2014-12-05 13:23:57]

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