Minor cleanup of EnrollmentHandlerChromeOS.

chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc

Index: chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
diff --git a/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc b/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
index 5e447e94972fa68ca7133a10fd5e956cb1f3b61c..1070d56a75194069a08ccf7a64ea0fcccab9d5f9 100644
--- a/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
+++ b/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
@@ -89,7 +89,7 @@ void EnrollmentHandlerChromeOS::StartEnrollment() {
   CHECK_EQ(STEP_PENDING, enrollment_step_);
   enrollment_step_ = STEP_STATE_KEYS;
   state_keys_broker_->RequestStateKeys(
-      base::Bind(&EnrollmentHandlerChromeOS::CheckStateKeys,
+      base::Bind(&EnrollmentHandlerChromeOS::HandleStateKeysResult,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ -140,7 +140,7 @@ void EnrollmentHandlerChromeOS::OnPolicyFetched(CloudPolicyClient* client) {
   // from that username to validate the key below (http://crbug.com/343074).
   validator->ValidateInitialKey(GetPolicyVerificationKey(), domain);
   validator.release()->StartValidation(
-      base::Bind(&EnrollmentHandlerChromeOS::PolicyValidated,
+      base::Bind(&EnrollmentHandlerChromeOS::HandlePolicyValidationResult,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ -162,7 +162,7 @@ void EnrollmentHandlerChromeOS::OnRegistrationStateChanged(
     client_->FetchPolicy();
   } else {
     LOG(FATAL) << "Registration state changed to " << client_->is_registered()
-               << " in step " << enrollment_step_;
+               << " in step " << enrollment_step_ << ".";
   }
 }
 
@@ -184,10 +184,10 @@ void EnrollmentHandlerChromeOS::OnStoreLoaded(CloudPolicyStore* store) {
   DCHECK_EQ(store_, store);
 
   if (enrollment_step_ == STEP_LOADING_STORE) {
-    // If the |store_| wasn't initialized when StartEnrollment() was
-    // called, then AttemptRegistration() bails silently.  This gets
-    // registration rolling again after the store finishes loading.
-    AttemptRegistration();
+    // If the |store_| wasn't initialized when StartEnrollment() was called,
+    // then StartRegistration() bails silently.  This gets registration rolling
+    // again after the store finishes loading.
+    StartRegistration();
   } else if (enrollment_step_ == STEP_STORE_POLICY) {
     ReportResult(EnrollmentStatus::ForStatus(EnrollmentStatus::STATUS_SUCCESS));
   }
@@ -206,7 +206,7 @@ void EnrollmentHandlerChromeOS::OnStoreError(CloudPolicyStore* store) {
                                                store_->validation_status()));
 }
 
-void EnrollmentHandlerChromeOS::CheckStateKeys(
+void EnrollmentHandlerChromeOS::HandleStateKeysResult(
     const std::vector<std::string>& state_keys) {
   CHECK_EQ(STEP_STATE_KEYS, enrollment_step_);
 
@@ -219,24 +219,27 @@ void EnrollmentHandlerChromeOS::CheckStateKeys(
       return;
     }
     client_->SetStateKeysToUpload(state_keys);
-    current_state_key_ = state_keys_broker_->current_state_key();
+    current_state_key_ = state_keys.front();

[Joao da Silva, 2014/08/14 12:11:10]

Why? What if the logic for current_state_key() changes?

[Thiemo Nagel, 2014/08/14 14:52:37]

With the current implementation, we can't be sure that current_state_key_ is
non-empty without making assumptions on StateKeysBroker behaviour.  Anyways,
you're right, my proposal is too brittle, I'm keeping the redundant call to
StateKeysBroker but check the return value.

   }
 
   enrollment_step_ = STEP_LOADING_STORE;
-  AttemptRegistration();
+  StartRegistration();
 }
 
-void EnrollmentHandlerChromeOS::AttemptRegistration() {
+void EnrollmentHandlerChromeOS::StartRegistration() {
   CHECK_EQ(STEP_LOADING_STORE, enrollment_step_);
   if (store_->is_initialized()) {
     enrollment_step_ = STEP_REGISTRATION;
     client_->Register(em::DeviceRegisterRequest::DEVICE,
                       auth_token_, client_id_, is_auto_enrollment_,
                       requisition_, current_state_key_);
+  } else {
+    // Do nothing.  StartRegistration() will be called again from

[Joao da Silva, 2014/08/14 12:11:10]

nit: single space after "." (I've seen other files with double spaces, but we
use single spaces consistently)

[Thiemo Nagel, 2014/08/14 14:52:37]

I don't agree.  The sample comments in the style guide use double space:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

[Joao da Silva, 2014/08/20 12:32:58]

Look around this file and other policy/ files. We use single spaces in our
comments, and many other pieces of the codebase do that too.

I see it's against the style guide, but I'd favor consistency within a file.

[Thiemo Nagel, 2014/08/20 13:26:15]

Done.

+    // OnStoreLoaded() after the CloudPolicyStore has initialized.
   }
 }
 
-void EnrollmentHandlerChromeOS::PolicyValidated(
+void EnrollmentHandlerChromeOS::HandlePolicyValidationResult(
     DeviceCloudPolicyValidator* validator) {
   CHECK_EQ(STEP_VALIDATION, enrollment_step_);
   if (validator->success()) {
@@ -302,7 +305,7 @@ void EnrollmentHandlerChromeOS::OnRefreshTokenResponse(
     const std::string& access_token,
     int expires_in_seconds) {
   // We never use the code that should trigger this callback.
-  LOG(FATAL) << "Unexpected callback invoked";
+  LOG(FATAL) << "Unexpected callback invoked.";
 }
 
 // GaiaOAuthClient::Delegate OAuth2 error when fetching refresh token request.
@@ -335,7 +338,7 @@ void EnrollmentHandlerChromeOS::StartLockDevice() {
     enrollment_step_ = STEP_STORE_TOKEN_AND_ID;
     device_settings_service_->SetManagementSettings(
         management_mode_, request_token_, device_id_,
-        base::Bind(&EnrollmentHandlerChromeOS::OnSetManagementSettingsDone,
+        base::Bind(&EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone,
                    weak_ptr_factory_.GetWeakPtr()));
   } else {
     install_attributes_->LockDevice(
@@ -345,7 +348,7 @@ void EnrollmentHandlerChromeOS::StartLockDevice() {
   }
 }
 
-void EnrollmentHandlerChromeOS::OnSetManagementSettingsDone() {
+void EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone() {
   CHECK_EQ(STEP_STORE_TOKEN_AND_ID, enrollment_step_);
   if (device_settings_service_->status() !=
       chromeos::DeviceSettingsService::STORE_SUCCESS) {
@@ -354,7 +357,7 @@ void EnrollmentHandlerChromeOS::OnSetManagementSettingsDone() {
     return;
   }
 
-  StoreRobotAuth();
+  StartStoreRobotAuth();
 }
 
 void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
@@ -362,7 +365,7 @@ void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
   CHECK_EQ(STEP_LOCK_DEVICE, enrollment_step_);
   switch (lock_result) {
     case EnterpriseInstallAttributes::LOCK_SUCCESS:
-      StoreRobotAuth();
+      StartStoreRobotAuth();
       return;
     case EnterpriseInstallAttributes::LOCK_NOT_READY:
       // We wait up to |kLockRetryTimeoutMs| milliseconds and if it hasn't
@@ -393,22 +396,19 @@ void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
           EnrollmentStatus::STATUS_LOCK_WRONG_USER));
       return;
   }
-
-  NOTREACHED() << "Invalid lock result " << lock_result;
-  ReportResult(EnrollmentStatus::ForStatus(
-      EnrollmentStatus::STATUS_LOCK_ERROR));

[Joao da Silva, 2014/08/14 12:11:10]

Don't remove this. If this flow enters somehow then we need to call
ReportResult, otherwise the device will just get stuck. If we report this error
then the user has something specific to send to support, and we can debug the
problem.

[Thiemo Nagel, 2014/08/14 14:52:37]

Control cannot get there because each of the switch cases return.  The style
guide explictly forbids code after NOTREACHED():

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

+  NOTREACHED();
 }
 
-void EnrollmentHandlerChromeOS::StoreRobotAuth() {
+void EnrollmentHandlerChromeOS::StartStoreRobotAuth() {
   // Get the token service so we can store our robot refresh token.
   enrollment_step_ = STEP_STORE_ROBOT_AUTH;
   chromeos::DeviceOAuth2TokenServiceFactory::Get()->SetAndSaveRefreshToken(
       refresh_token_,
-      base::Bind(&EnrollmentHandlerChromeOS::HandleRobotAuthTokenStored,
+      base::Bind(&EnrollmentHandlerChromeOS::HandleStoreRobotAuthTokenResult,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void EnrollmentHandlerChromeOS::HandleRobotAuthTokenStored(bool result) {
+void EnrollmentHandlerChromeOS::HandleStoreRobotAuthTokenResult(bool result) {
   CHECK_EQ(STEP_STORE_ROBOT_AUTH, enrollment_step_);
 
   if (!result) {
@@ -442,9 +442,9 @@ void EnrollmentHandlerChromeOS::ReportResult(EnrollmentStatus status) {
 
   if (status.status() != EnrollmentStatus::STATUS_SUCCESS) {
     LOG(WARNING) << "Enrollment failed: " << status.status()
-                 << " " << status.client_status()
-                 << " " << status.validation_status()
-                 << " " << status.store_status();
+                 << ", client: " << status.client_status()
+                 << ", validation: " << status.validation_status()
+                 << ", store: " << status.store_status();
   }
 
   if (!callback.is_null())