Implement Chrome OS device enrollment on the new cloud policy stack.

Implement Chrome OS device enrollment on the new cloud policy stack.

The enrollment flow is now concentrated in EnrollmentHandlerChromeOS,
which makes the code easier to follow and allows for better and more
consistent error reporting.

BUG=chromium:108928
TEST=unit tests

TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163822

[Mattias Nissler (ping if slow), 2012-09-06 19:25:27]

And this is the new enrollment implementation. Last one for today.

[pastarmovj, 2012-09-07 12:16:27]

it sgtm a few questions/comments though before the s turns into an l.

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_client.cc:196: device_mode_ !=
DEVICE_MODE_KIOSK))) {
I know that this check is happening only once but I think it might improve
readability and maintainability if you extract it in a function. something along
the lines ValidateResponse(const em::DeviceManagementResponse& response)...

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
File chrome/browser/policy/enrollment_handler_chromeos.cc (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:84:
validator.release()->StartValidation();
Apparently the validator is self destructing why not use plain ptr and document
write that it is managing its own lifetime?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:229: enrollment_step_ =
STEP_DONE;
Why not STEP_IDLE?

[pastarmovj, 2012-09-07 12:20:24]

...and I hope you have noticed the redness of most of your tryjobs on both
CLs...

[Joao da Silva, 2012-09-07 14:01:42]

Much nicer to follow having all the steps together!

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_client.cc:30: return  DEVICE_MODE_NOT_SET;
nit: one space after return

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_client.cc:196: device_mode_ !=
DEVICE_MODE_KIOSK))) {
Please add a comment explaining what the device_mode_ tests mean for user
policy, as discussed offline.

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_constants.h (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_constants.h:105: 
nit: remove extra newline

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:95:
StartIfInitialized();
Why? Is this for the re-enrollment case?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_c...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_c...
chrome/browser/policy/device_cloud_policy_manager_chromeos.h:52: // |callback|
is invoked, indicating success.
"indicating success" is a bit misleading, since this is also called on errors.
Maybe "|callback| is invoked with the result of the enrollment request"

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
File chrome/browser/policy/enrollment_handler_chromeos.cc (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:11: #include
"chrome/browser/policy/cloud_policy_validator.h"
nit: already in .h

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:104: }
else { ?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:108: << " in step " <<
enrollment_step_;
Shouldn't this report an error result, to avoid a stuck enrollment? Manual
enrollment can be cancelled but auto-enrollment can't.

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:125: << " in step " <<
enrollment_step_;
Report an error result?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:169:
DeviceCloudPolicyValidator* validator) {
CHECK_EQ(STEP_VALIDATION, enrollment_step_)?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:171: policy_ =
validator->policy().Pass();
enrollment_step_ = STEP_LOCK_DEVICE?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:184: // Since this method
is also called directly.
CHECK_EQ(STEP_LOCK_DEVICE, enrollment_step_)?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:208: // Fall through.
Also log that it timed out?

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.cc:251: }  // namespace
namespace policy

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
File chrome/browser/policy/enrollment_handler_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.h:11: #include
"base/callback_forward.h"
nit: not used

http://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollme...
chrome/browser/policy/enrollment_handler_chromeos.h:120:
base::WeakPtrFactory<EnrollmentHandlerChromeOS> weak_factory_;
#include "base/memory/weak_ptr.h"

http://codereview.chromium.org/10928036/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10928036/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:1259:
'browser/policy/enrollment_handler_chromeos.cc',
nit: order (.h and .cc)

[Mattias Nissler (ping if slow), 2012-10-23 15:30:41]

PTAL, I'd like to land this rather sooner than later :)

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:30: return  DEVICE_MODE_NOT_SET;
On 2012/09/07 14:01:42, Joao da Silva wrote:
> nit: one space after return

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:196: device_mode_ !=
DEVICE_MODE_KIOSK))) {
On 2012/09/07 14:01:42, Joao da Silva wrote:
> Please add a comment explaining what the device_mode_ tests mean for user
> policy, as discussed offline.

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:196: device_mode_ !=
DEVICE_MODE_KIOSK))) {
On 2012/09/07 12:16:27, pastarmovj wrote:
> I know that this check is happening only once but I think it might improve
> readability and maintainability if you extract it in a function. something
along
> the lines ValidateResponse(const em::DeviceManagementResponse& response)...

I thought about this some more and ended up re-structuring the code.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
File chrome/browser/policy/cloud_policy_constants.h (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_constants.h:105: 
On 2012/09/07 14:01:42, Joao da Silva wrote:
> nit: remove extra newline

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:95:
StartIfInitialized();
On 2012/09/07 14:01:42, Joao da Silva wrote:
> Why? Is this for the re-enrollment case?

Yes. Also, for robustness. While enrollment is on, we don't have a
CloudPolicyService. At all other times, we should have CloudPolicyService
running if there is valid policy present.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.h (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_cloud_policy_manager_chromeos.h:52: // |callback|
is invoked, indicating success.
On 2012/09/07 14:01:42, Joao da Silva wrote:
> "indicating success" is a bit misleading, since this is also called on errors.
> Maybe "|callback| is invoked with the result of the enrollment request"

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
File chrome/browser/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:11: #include
"chrome/browser/policy/cloud_policy_validator.h"
On 2012/09/07 14:01:42, Joao da Silva wrote:
> nit: already in .h

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:84:
validator.release()->StartValidation();
On 2012/09/07 12:16:27, pastarmovj wrote:
> Apparently the validator is self destructing why not use plain ptr and
document
> write that it is managing its own lifetime?

This is the pattern used for user policy (and elsewhere), so I thought I stick
with it. Also, it seems rather self-documenting to me: Once validation starts,
we pass on ownership.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:104: }
On 2012/09/07 14:01:42, Joao da Silva wrote:
> else { ?

Ah, yes :)

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:108: << " in step " <<
enrollment_step_;
On 2012/09/07 14:01:42, Joao da Silva wrote:
> Shouldn't this report an error result, to avoid a stuck enrollment? Manual
> enrollment can be cancelled but auto-enrollment can't.

Since the handler owns |client_|, this is not possible. I've upgraded this to a
LOG(FATAL) so we'll learn when somebody breaks this invariant :)

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:125: << " in step " <<
enrollment_step_;
On 2012/09/07 14:01:42, Joao da Silva wrote:
> Report an error result?

Not sure. If something else is screwing the client, we cannot do much, and in
the two cases where we do care we report the result already. So this is mostly
for debugging if somebody adds bad code.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:169:
DeviceCloudPolicyValidator* validator) {
On 2012/09/07 14:01:42, Joao da Silva wrote:
> CHECK_EQ(STEP_VALIDATION, enrollment_step_)?

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:171: policy_ =
validator->policy().Pass();
On 2012/09/07 14:01:42, Joao da Silva wrote:
> enrollment_step_ = STEP_LOCK_DEVICE?

Yes!

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:184: // Since this method
is also called directly.
On 2012/09/07 14:01:42, Joao da Silva wrote:
> CHECK_EQ(STEP_LOCK_DEVICE, enrollment_step_)?

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:208: // Fall through.
On 2012/09/07 14:01:42, Joao da Silva wrote:
> Also log that it timed out?

Line 197 will have flooded the log already, which should be a rather good
indication.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:229: enrollment_step_ =
STEP_DONE;
On 2012/09/07 12:16:27, pastarmovj wrote:
> Why not STEP_IDLE?

Because this is only to happen once we're done. I've updated the name to be more
obvious.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.cc:251: }  // namespace
On 2012/09/07 14:01:42, Joao da Silva wrote:
> namespace policy

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
File chrome/browser/policy/enrollment_handler_chromeos.h (right):

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.h:11: #include
"base/callback_forward.h"
On 2012/09/07 14:01:42, Joao da Silva wrote:
> nit: not used

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/browser/policy/enrollm...
chrome/browser/policy/enrollment_handler_chromeos.h:120:
base::WeakPtrFactory<EnrollmentHandlerChromeOS> weak_factory_;
On 2012/09/07 14:01:42, Joao da Silva wrote:
> #include "base/memory/weak_ptr.h"

Done.

https://codereview.chromium.org/10928036/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/10928036/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:1259:
'browser/policy/enrollment_handler_chromeos.cc',
On 2012/09/07 14:01:42, Joao da Silva wrote:
> nit: order (.h and .cc)

Done.

[pastarmovj, 2012-10-23 15:58:08]

i am generally ok with the code now. Please take a look at the comments in the
code though.

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/dev...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/dev...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:143: if
(status.status() == EnrollmentStatus::STATUS_SUCCESS) {
Isn't it possible to call StartIfManaged() unconditionally here?

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enr...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enr...
chrome/browser/policy/enrollment_status_chromeos.h:49: EnrollmentStatus(Status
status,
I am not sure that those helper functions that create all the possible statuses
help readability much. I can imagine many of them are called one or two times
throughout the code base and if new errors are introduced this will only
increase the amount of boilerplate code.

[Mattias Nissler (ping if slow), 2012-10-23 16:33:03]

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/dev...
File chrome/browser/policy/device_cloud_policy_manager_chromeos.cc (right):

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/dev...
chrome/browser/policy/device_cloud_policy_manager_chromeos.cc:143: if
(status.status() == EnrollmentStatus::STATUS_SUCCESS) {
On 2012/10/23 15:58:08, pastarmovj wrote:
> Isn't it possible to call StartIfManaged() unconditionally here? 

No, in this case we want to reuse the client. I guess I could pass the client in
a scoped_ptr parameter if you prefer that, but that would add noise to the other
call sites.

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enr...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

https://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enr...
chrome/browser/policy/enrollment_status_chromeos.h:49: EnrollmentStatus(Status
status,
On 2012/10/23 15:58:08, pastarmovj wrote:
> I am not sure that those helper functions that create all the possible
statuses
> help readability much. I can imagine many of them are called one or two times
> throughout the code base and if new errors are introduced this will only
> increase the amount of boilerplate code.

There are 9 call sites in the enrollment handler. All of them would grow to 4-5
lines. Note that we'd only increase boilerplate for a new category of errors,
but not for just another error condition in one of the existing categories.
Overall, I think it's worth it, but I don't feel too strongly here. Joao, what
do you think?

[pastarmovj, 2012-10-23 16:37:18]

lgtm

ok i am sold on both :)

[Joao da Silva, 2012-10-23 18:58:50]

lgtm after fixing the logic in cloud_policy_client.cc.

http://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_status_chromeos.h:49: EnrollmentStatus(Status
status,
On 2012/10/23 16:33:03, Mattias Nissler wrote:
> On 2012/10/23 15:58:08, pastarmovj wrote:
> > I am not sure that those helper functions that create all the possible
> statuses
> > help readability much. I can imagine many of them are called one or two
times
> > throughout the code base and if new errors are introduced this will only
> > increase the amount of boilerplate code.
> 
> There are 9 call sites in the enrollment handler. All of them would grow to
4-5
> lines. Note that we'd only increase boilerplate for a new category of errors,
> but not for just another error condition in one of the existing categories.
> Overall, I think it's worth it, but I don't feel too strongly here. Joao, what
> do you think?

No strong feeling either way, I find it more readable as is. I thought of
proposing to move this to cloud_policy_constants.h wrapped in an ugly #ifdef,
wdyt?

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_client.cc:192:
response.register_response().has_device_management_token())) {
The previous logic was correct. This will ref register_response() if
has_register_response() is false.

Did you mean to turn the || into an &&?

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc:329:
policy_.policy());
indent

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_handler_chromeos.cc (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:84:
validator->ValidateInitialKey();
Should this also ValidateDMToken()?

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:123: AttemptRegistration();
Technically it's possible that StartEnrollment() hasn't been called yet. Not
sure if you want to handle that case at all, or just document that
StartEnrollment must be called right after the ctor.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:199: NOTREACHED();
ReportResult() to avoid leaving the user stuck?

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_handler_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.h:41: CompletionCallback;
Why the different names?

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_status_chromeos.h:46: return
validation_status_; }
closing brace on a new line

[Mattias Nissler (ping if slow), 2012-10-24 10:33:17]

http://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/11001/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_status_chromeos.h:49: EnrollmentStatus(Status
status,
On 2012/10/23 18:58:50, Joao da Silva wrote:
> On 2012/10/23 16:33:03, Mattias Nissler wrote:
> > On 2012/10/23 15:58:08, pastarmovj wrote:
> > > I am not sure that those helper functions that create all the possible
> > statuses
> > > help readability much. I can imagine many of them are called one or two
> times
> > > throughout the code base and if new errors are introduced this will only
> > > increase the amount of boilerplate code.
> > 
> > There are 9 call sites in the enrollment handler. All of them would grow to
> 4-5
> > lines. Note that we'd only increase boilerplate for a new category of
errors,
> > but not for just another error condition in one of the existing categories.
> > Overall, I think it's worth it, but I don't feel too strongly here. Joao,
what
> > do you think?
> 
> No strong feeling either way, I find it more readable as is. I thought of
> proposing to move this to cloud_policy_constants.h wrapped in an ugly #ifdef,
> wdyt?

cloud_policy_constants.h currently contains only stuff that's relevant to the
cloud policy protocol. I wouldn't want to mingle platform-specific stuff (such
as enrollment) into that file.

I'll leave this as it is then.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_client.cc:192:
response.register_response().has_device_management_token())) {
On 2012/10/23 18:58:50, Joao da Silva wrote:
> The previous logic was correct. This will ref register_response() if
> has_register_response() is false.
> 
> Did you mean to turn the || into an &&?

Reverted, I find the old version actually easier to parse.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc:329:
policy_.policy());
On 2012/10/23 18:58:50, Joao da Silva wrote:
> indent

Done.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_handler_chromeos.cc (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:84:
validator->ValidateInitialKey();
On 2012/10/23 18:58:50, Joao da Silva wrote:
> Should this also ValidateDMToken()?

Yes! Fixed.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:123: AttemptRegistration();
On 2012/10/23 18:58:50, Joao da Silva wrote:
> Technically it's possible that StartEnrollment() hasn't been called yet. Not
> sure if you want to handle that case at all, or just document that
> StartEnrollment must be called right after the ctor.

Good observation. Better safe than sorry, I've added a new state to cover this.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.cc:199: NOTREACHED();
On 2012/10/23 18:58:50, Joao da Silva wrote:
> ReportResult() to avoid leaving the user stuck?

Done.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_handler_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_handler_chromeos.h:41: CompletionCallback;
On 2012/10/23 18:58:50, Joao da Silva wrote:
> Why the different names?

That's a code evolution artifact :) Fixed.

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
File chrome/browser/policy/enrollment_status_chromeos.h (right):

http://codereview.chromium.org/10928036/diff/23002/chrome/browser/policy/enro...
chrome/browser/policy/enrollment_status_chromeos.h:46: return
validation_status_; }
On 2012/10/23 18:58:50, Joao da Silva wrote:
> closing brace on a new line

Done.

[Joao da Silva, 2012-10-24 10:57:01]

Still lgtm

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc:329:
policy_.policy());
Still broken? (maybe there are tabs in this line?)

[Mattias Nissler (ping if slow), 2012-10-24 11:01:32]

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc:329:
policy_.policy());
On 2012/10/24 10:57:01, Joao da Silva wrote:
> Still broken? (maybe there are tabs in this line?)

What's broken? I see exactly 4 spaces of indentation, here and in my editor.

[Joao da Silva, 2012-10-24 11:37:06]

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
File chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

http://codereview.chromium.org/10928036/diff/31001/chrome/browser/policy/devi...
chrome/browser/policy/device_cloud_policy_manager_chromeos_unittest.cc:329:
policy_.policy());
On 2012/10/24 11:01:32, Mattias Nissler wrote:
> On 2012/10/24 10:57:01, Joao da Silva wrote:
> > Still broken? (maybe there are tabs in this line?)
> 
> What's broken? I see exactly 4 spaces of indentation, here and in my editor.

Weird. The diff to the original is good, but the diff to patch set 4 shows
different spaces:

http://codereview.chromium.org/10928036/diff2/23002:31001/chrome/browser/poli...

[Mattias Nissler (ping if slow), 2012-10-24 11:51:17]

+ben for the gyp changes, ticking the box now.

[commit-bot: I haz the power, 2012-10-24 11:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10928036/32025

[commit-bot: I haz the power, 2012-10-24 14:02:10]

Change committed as 163822