Minor cleanup of EnrollmentHandlerChromeOS.

chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/enrollment_handler_chromeos.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 71 matching lines @@
 
 EnrollmentHandlerChromeOS::~EnrollmentHandlerChromeOS() {
   Stop();
   store_->RemoveObserver(this);
 }
 
 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()));
 }
 
 scoped_ptr<CloudPolicyClient> EnrollmentHandlerChromeOS::ReleaseClient() {
   Stop();
   return client_.Pass();
 }
 
 void EnrollmentHandlerChromeOS::OnPolicyFetched(CloudPolicyClient* client) {
   DCHECK_EQ(client_.get(), client);
@@ skipping 30 matching lines @@
                              CloudPolicyValidatorBase::DM_TOKEN_REQUIRED);
   validator->ValidatePolicyType(dm_protocol::kChromeDevicePolicyType);
   validator->ValidatePayload();
   // If |domain| is empty here, the policy validation code will just use the
   // domain from the username field in the policy itself to do key validation.
   // TODO(mnissler): Plumb the enrolling user's username into this object so
   // we can validate the username on the resulting policy, and use the domain
   // 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()));
 }
 
 void EnrollmentHandlerChromeOS::OnRegistrationStateChanged(
     CloudPolicyClient* client) {
   DCHECK_EQ(client_.get(), client);
 
   if (enrollment_step_ == STEP_REGISTRATION && client_->is_registered()) {
     enrollment_step_ = STEP_POLICY_FETCH,
     device_mode_ = client_->device_mode();
     if (device_mode_ == DEVICE_MODE_NOT_SET)
       device_mode_ = DEVICE_MODE_ENTERPRISE;
     if (!allowed_device_modes_.test(device_mode_)) {
       LOG(ERROR) << "Bad device mode " << device_mode_;
       ReportResult(EnrollmentStatus::ForStatus(
           EnrollmentStatus::STATUS_REGISTRATION_BAD_MODE));
       return;
     }
     client_->FetchPolicy();
   } else {
     LOG(FATAL) << "Registration state changed to " << client_->is_registered()
-               << " in step " << enrollment_step_;
+               << " in step " << enrollment_step_ << ".";
   }
 }
 
 void EnrollmentHandlerChromeOS::OnClientError(CloudPolicyClient* client) {
   DCHECK_EQ(client_.get(), client);
 
   if (enrollment_step_ == STEP_ROBOT_AUTH_FETCH) {
     LOG(ERROR) << "API authentication code fetch failed: "
                << client_->status();
     ReportResult(EnrollmentStatus::ForRobotAuthFetchError(client_->status()));
   } else if (enrollment_step_ < STEP_POLICY_FETCH) {
     ReportResult(EnrollmentStatus::ForRegistrationError(client_->status()));
   } else {
     ReportResult(EnrollmentStatus::ForFetchError(client_->status()));
   }
 }
 
 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));
   }
 }
 
 void EnrollmentHandlerChromeOS::OnStoreError(CloudPolicyStore* store) {
   DCHECK_EQ(store_, store);
   if (enrollment_step_ == STEP_STORE_TOKEN_AND_ID) {
     // Calling DeviceSettingsService::SetManagementSettings() on a non-
     // enterprise-managed device will trigger OnStoreError(), as
     // DeviceCloudPolicyStore listens to all changes on DeviceSettingsService,
     // and it calls OnStoreError() when the device is not enterprise-managed.
     return;
   }
   ReportResult(EnrollmentStatus::ForStoreError(store_->status(),
                                                store_->validation_status()));
 }
 
-void EnrollmentHandlerChromeOS::CheckStateKeys(
+void EnrollmentHandlerChromeOS::HandleStateKeysResult(
     const std::vector<std::string>& state_keys) {
   CHECK_EQ(STEP_STATE_KEYS, enrollment_step_);
 
   // Make sure state keys are available if forced re-enrollment is on.
   if (chromeos::AutoEnrollmentController::GetMode() ==
       chromeos::AutoEnrollmentController::MODE_FORCED_RE_ENROLLMENT) {
     if (state_keys.empty()) {
       ReportResult(
           EnrollmentStatus::ForStatus(EnrollmentStatus::STATUS_NO_STATE_KEYS));
       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()) {
     policy_ = validator->policy().Pass();
     username_ = validator->policy_data()->username();
     device_id_ = validator->policy_data()->device_id();
     request_token_ = validator->policy_data()->request_token();
 
     if (CommandLine::ForCurrentProcess()->HasSwitch(
             chromeos::switches::kEnterpriseEnrollmentSkipRobotAuth)) {
@@ skipping 45 matching lines @@
 
   enrollment_step_ = STEP_LOCK_DEVICE;
   StartLockDevice();
 }
 
 // GaiaOAuthClient::Delegate
 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.
 void EnrollmentHandlerChromeOS::OnOAuthError() {
   CHECK_EQ(STEP_ROBOT_AUTH_REFRESH, enrollment_step_);
   // OnOAuthError is only called if the request is bad (malformed) or the
   // response is bad (empty access token returned).
   LOG(ERROR) << "OAuth protocol error while fetching API refresh token.";
   ReportResult(
       EnrollmentStatus::ForRobotRefreshFetchError(net::HTTP_BAD_REQUEST));
@@ skipping 12 matching lines @@
   CHECK_EQ(STEP_LOCK_DEVICE, enrollment_step_);
   // Since this method is also called directly.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   if (management_mode_ == em::PolicyData::CONSUMER_MANAGED) {
     // Consumer device enrollment doesn't use install attributes. Instead,
     // we put the information in the owners settings.
     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(
         username_, device_mode_, device_id_,
         base::Bind(&EnrollmentHandlerChromeOS::HandleLockDeviceResult,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
-void EnrollmentHandlerChromeOS::OnSetManagementSettingsDone() {
+void EnrollmentHandlerChromeOS::HandleSetManagementSettingsDone() {
   CHECK_EQ(STEP_STORE_TOKEN_AND_ID, enrollment_step_);
   if (device_settings_service_->status() !=
       chromeos::DeviceSettingsService::STORE_SUCCESS) {
     ReportResult(EnrollmentStatus::ForStatus(
         EnrollmentStatus::STATUS_STORE_TOKEN_AND_ID_FAILED));
     return;
   }
 
-  StoreRobotAuth();
+  StartStoreRobotAuth();
 }
 
 void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
     EnterpriseInstallAttributes::LockResult lock_result) {
   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
       // succeeded by then show an error to the user and stop the enrollment.
       if (lockbox_init_duration_ < kLockRetryTimeoutMs) {
         // InstallAttributes not ready yet, retry later.
         LOG(WARNING) << "Install Attributes not ready yet will retry in "
                      << kLockRetryIntervalMs << "ms.";
         base::MessageLoop::current()->PostDelayedTask(
             FROM_HERE,
@@ skipping 10 matching lines @@
       ReportResult(EnrollmentStatus::ForStatus(
           EnrollmentStatus::STATUS_LOCK_ERROR));
       return;
     case EnterpriseInstallAttributes::LOCK_WRONG_USER:
       LOG(ERROR) << "Enrollment cannot proceed because the InstallAttrs "
                  << "has been locked already!";
       ReportResult(EnrollmentStatus::ForStatus(
           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) {
     LOG(ERROR) << "Failed to store API refresh token.";
     ReportResult(EnrollmentStatus::ForStatus(
         EnrollmentStatus::STATUS_ROBOT_REFRESH_STORE_FAILED));
     return;
   }
 
   if (management_mode_ == em::PolicyData::CONSUMER_MANAGED) {
@@ skipping 13 matching lines @@
   weak_ptr_factory_.InvalidateWeakPtrs();
   completion_callback_.Reset();
 }
 
 void EnrollmentHandlerChromeOS::ReportResult(EnrollmentStatus status) {
   EnrollmentCallback callback = completion_callback_;
   Stop();
 
   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())
     callback.Run(status);
 }
 
 }  // namespace policy