Minor cleanup of EnrollmentHandlerChromeOS.

Rename private methods for consistency: Start*() for something that invokes
an async operation and Handle*() for something that gets called back by such
an operation.  That leaves On*() reserved for observers.  Also update some
comments.  Lastly, use the current state key from the parameter passed to
HandleStateKeysResult() instead of requesting it again from the broker (line
222).

EnrollmentHandlerChromeOS and EnrollmentScreen: Don't report metrics on
unreached code paths.  Generally, remove some obsolete instances of
NOTREACHED() after switches over enums:  The compiler ensures that all enum
values are covered, there's no need to complicate the code by checking the
same thing again.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290841

Review URL: https://codereview.chromium.org/465413002

Cr-Commit-Position: refs/heads/master@{#293640}

chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc

@@ -180,10 +180,11 @@ bool AutoEnrollmentCheckScreen::UpdateCaptivePortalStatus(
       ShowErrorScreen(ErrorScreen::ERROR_STATE_PROXY);
       return true;
     case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT:
-      // Trigger NOTREACHED() below.
-      break;
+      NOTREACHED() << "Bad status: CAPTIVE_PORTAL_STATUS_COUNT";
+      return false;
   }
 
+  // Return is required to avoid compiler warning.
   NOTREACHED() << "Bad status " << new_captive_portal_status;
   return false;
 }
@@ -205,6 +206,7 @@ bool AutoEnrollmentCheckScreen::UpdateAutoEnrollmentState(
       return true;
   }
 
+  // Return is required to avoid compiler warning.
   NOTREACHED() << "bad state " << new_auto_enrollment_state;
   return false;
 }

chrome/browser/chromeos/login/enrollment/enrollment_screen.cc

@@ -110,9 +110,6 @@ void EnrollmentScreen::OnLoginDone(const std::string& user) {
 }
 
 void EnrollmentScreen::OnAuthError(const GoogleServiceAuthError& error) {
-  enrollment_failed_once_ = true;
-  actor_->ShowAuthError(error);
-
   switch (error.state()) {
     case GoogleServiceAuthError::NONE:
     case GoogleServiceAuthError::CAPTCHA_REQUIRED:
@@ -124,24 +121,25 @@ void EnrollmentScreen::OnAuthError(const GoogleServiceAuthError& error) {
     case GoogleServiceAuthError::SERVICE_ERROR:
       UMAFailure(policy::kMetricEnrollmentLoginFailed);
       LOG(ERROR) << "Auth error " << error.state();
-      return;
+      break;
     case GoogleServiceAuthError::USER_NOT_SIGNED_UP:
     case GoogleServiceAuthError::ACCOUNT_DELETED:
     case GoogleServiceAuthError::ACCOUNT_DISABLED:
       UMAFailure(policy::kMetricEnrollmentNotSupported);
       LOG(ERROR) << "Account error " << error.state();
-      return;
+      break;
     case GoogleServiceAuthError::CONNECTION_FAILED:
     case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
       UMAFailure(policy::kMetricEnrollmentNetworkFailed);
       LOG(WARNING) << "Network error " << error.state();
-      return;
+      break;
     case GoogleServiceAuthError::NUM_STATES:
+      NOTREACHED();
       break;
   }
 
-  NOTREACHED();
-  UMAFailure(policy::kMetricEnrollmentOtherFailed);
+  enrollment_failed_once_ = true;
+  actor_->ShowAuthError(error);
 }
 
 void EnrollmentScreen::OnOAuthTokenAvailable(const std::string& token) {
@@ -251,20 +249,15 @@ void EnrollmentScreen::ShowEnrollmentStatusOnSuccess(
 }
 
 void EnrollmentScreen::ReportEnrollmentStatus(policy::EnrollmentStatus status) {
-  if (status.status() == policy::EnrollmentStatus::STATUS_SUCCESS) {
-    StartupUtils::MarkDeviceRegistered(
-        base::Bind(&EnrollmentScreen::ShowEnrollmentStatusOnSuccess,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   status));
-    UMA(is_auto_enrollment() ? policy::kMetricEnrollmentAutoOK
-                             : policy::kMetricEnrollmentOK);
-    return;
-  } else {
-    enrollment_failed_once_ = true;
-  }
-  actor_->ShowEnrollmentStatus(status);
-
   switch (status.status()) {
+    case policy::EnrollmentStatus::STATUS_SUCCESS:
+      StartupUtils::MarkDeviceRegistered(
+          base::Bind(&EnrollmentScreen::ShowEnrollmentStatusOnSuccess,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     status));
+      UMA(is_auto_enrollment() ? policy::kMetricEnrollmentAutoOK
+                               : policy::kMetricEnrollmentOK);
+      return;
     case policy::EnrollmentStatus::STATUS_REGISTRATION_FAILED:
     case policy::EnrollmentStatus::STATUS_POLICY_FETCH_FAILED:
       switch (status.client_status()) {
@@ -276,72 +269,70 @@ void EnrollmentScreen::ReportEnrollmentStatus(policy::EnrollmentStatus status) {
         case policy::DM_STATUS_SERVICE_DEVICE_ID_CONFLICT:
         case policy::DM_STATUS_SERVICE_POLICY_NOT_FOUND:
           UMAFailure(policy::kMetricEnrollmentOtherFailed);
-          return;
+          break;
         case policy::DM_STATUS_REQUEST_FAILED:
         case policy::DM_STATUS_TEMPORARY_UNAVAILABLE:
         case policy::DM_STATUS_HTTP_STATUS_ERROR:
         case policy::DM_STATUS_RESPONSE_DECODING_ERROR:
           UMAFailure(policy::kMetricEnrollmentNetworkFailed);
-          return;
+          break;
         case policy::DM_STATUS_SERVICE_MANAGEMENT_NOT_SUPPORTED:
           UMAFailure(policy::kMetricEnrollmentNotSupported);
-          return;
+          break;
         case policy::DM_STATUS_SERVICE_INVALID_SERIAL_NUMBER:
           UMAFailure(policy::kMetricEnrollmentInvalidSerialNumber);
-          return;
+          break;
         case policy::DM_STATUS_SERVICE_MISSING_LICENSES:
           UMAFailure(policy::kMetricMissingLicensesError);
-          return;
+          break;
         case policy::DM_STATUS_SERVICE_DEPROVISIONED:
           UMAFailure(policy::kMetricEnrollmentDeprovisioned);
-          return;
+          break;
         case policy::DM_STATUS_SERVICE_DOMAIN_MISMATCH:
           UMAFailure(policy::kMetricEnrollmentDomainMismatch);
-          return;
+          break;
       }
       break;
     case policy::EnrollmentStatus::STATUS_REGISTRATION_BAD_MODE:
       UMAFailure(policy::kMetricEnrollmentInvalidEnrollmentMode);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_LOCK_TIMEOUT:
       UMAFailure(policy::kMetricLockboxTimeoutError);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_LOCK_WRONG_USER:
       UMAFailure(policy::kMetricEnrollmentWrongUserError);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_NO_STATE_KEYS:
       UMAFailure(policy::kMetricEnrollmentNoStateKeys);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_VALIDATION_FAILED:
       UMAFailure(policy::kMetricEnrollmentPolicyValidationFailed);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_STORE_ERROR:
       UMAFailure(policy::kMetricEnrollmentCloudPolicyStoreError);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_LOCK_ERROR:
       UMAFailure(policy::kMetricEnrollmentLockBackendError);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_ROBOT_AUTH_FETCH_FAILED:
       UMAFailure(policy::kMetricEnrollmentRobotAuthCodeFetchFailed);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_ROBOT_REFRESH_FETCH_FAILED:
       UMAFailure(policy::kMetricEnrollmentRobotRefreshTokenFetchFailed);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_ROBOT_REFRESH_STORE_FAILED:
       UMAFailure(policy::kMetricEnrollmentRobotRefreshTokenStoreFailed);
-      return;
+      break;
     case policy::EnrollmentStatus::STATUS_STORE_TOKEN_AND_ID_FAILED:
-      // This error should not happen for enterprise enrollment.
+      // This error should not happen for enterprise enrollment, it only affects
+      // consumer enrollment.
       UMAFailure(policy::kMetricEnrollmentStoreTokenAndIdFailed);
       NOTREACHED();
-      return;
-    case policy::EnrollmentStatus::STATUS_SUCCESS:
-      NOTREACHED();
-      return;
+      break;
   }
 
-  NOTREACHED();
-  UMAFailure(policy::kMetricEnrollmentOtherFailed);
+  enrollment_failed_once_ = true;
+  actor_->ShowEnrollmentStatus(status);
 }
 
 void EnrollmentScreen::UMA(policy::MetricEnrollment sample) {

chrome/browser/chromeos/policy/auto_enrollment_client.cc

@@ -76,6 +76,7 @@ std::string ConvertRestoreMode(
       return kDeviceStateRestoreModeReEnrollmentEnforced;
   }
 
+  // Return is required to avoid compiler warning.
   NOTREACHED() << "Bad restore mode " << restore_mode;
   return std::string();
 }

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,37 +206,40 @@ void EnrollmentHandlerChromeOS::OnStoreError(CloudPolicyStore* store) {
                                                store_->validation_status()));
 }
 
-void EnrollmentHandlerChromeOS::CheckStateKeys(
+void EnrollmentHandlerChromeOS::HandleStateKeysResult(
     const std::vector<std::string>& state_keys, bool /* first_boot */) {
   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()) {
+    client_->SetStateKeysToUpload(state_keys);
+    current_state_key_ = state_keys_broker_->current_state_key();
+    if (state_keys.empty() || current_state_key_.empty()) {
       ReportResult(
           EnrollmentStatus::ForStatus(EnrollmentStatus::STATUS_NO_STATE_KEYS));
       return;
     }
-    client_->SetStateKeysToUpload(state_keys);
-    current_state_key_ = state_keys_broker_->current_state_key();
   }
 
   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 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,8 +365,8 @@ void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
   CHECK_EQ(STEP_LOCK_DEVICE, enrollment_step_);
   switch (lock_result) {
     case EnterpriseInstallAttributes::LOCK_SUCCESS:
-      StoreRobotAuth();
-      return;
+      StartStoreRobotAuth();
+      break;
     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.
@@ -381,34 +384,30 @@ void EnrollmentHandlerChromeOS::HandleLockDeviceResult(
         ReportResult(EnrollmentStatus::ForStatus(
             EnrollmentStatus::STATUS_LOCK_TIMEOUT));
       }
-      return;
+      break;
     case EnterpriseInstallAttributes::LOCK_BACKEND_ERROR:
       ReportResult(EnrollmentStatus::ForStatus(
           EnrollmentStatus::STATUS_LOCK_ERROR));
-      return;
+      break;
     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;
+      break;
   }
-
-  NOTREACHED() << "Invalid lock result " << lock_result;
-  ReportResult(EnrollmentStatus::ForStatus(
-      EnrollmentStatus::STATUS_LOCK_ERROR));
 }
 
-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 +441,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())

chrome/browser/chromeos/policy/enrollment_handler_chromeos.h

@@ -121,35 +121,34 @@ class EnrollmentHandlerChromeOS : public CloudPolicyClient::Observer,
   };
 
   // Handles the response to a request for server-backed state keys.
-  void CheckStateKeys(const std::vector<std::string>& state_keys,
-                      bool first_boot);
+  void HandleStateKeysResult(const std::vector<std::string>& state_keys,
+                             bool first_boot);
 
   // Starts registration if the store is initialized.
-  void AttemptRegistration();
+  void StartRegistration();
 
-  // Handles the policy validation result, proceeding with installation-time
-  // attributes locking if successful.
-  void PolicyValidated(DeviceCloudPolicyValidator* validator);
+  // Handles the policy validation result, proceeding with device lock if
+  // successful.
+  void HandlePolicyValidationResult(DeviceCloudPolicyValidator* validator);
 
-  // Calls LockDevice() and proceeds to policy installation. If unsuccessful,
-  // reports the result. Actual installation or error report will be done in
-  // HandleLockDeviceResult().
+  // Calls InstallAttributes::LockDevice() for enterprise enrollment and
+  // DeviceSettingsService::SetManagementSettings() for consumer
+  // enrollment.
   void StartLockDevice();
 
   // Checks the status after SetManagementSettings() is done. Proceeds to
   // robot auth code storing if successful.
-  void OnSetManagementSettingsDone();
+  void HandleSetManagementSettingsDone();
 
-  // Helper for StartLockDevice(). It performs the actual action based on
-  // the result of LockDevice.
+  // Handle callback from InstallAttributes::LockDevice() and retry on failure.
   void HandleLockDeviceResult(
       EnterpriseInstallAttributes::LockResult lock_result);
 
-  // Stores robot auth token.
-  void StoreRobotAuth();
+  // Initiates storing of robot auth token.
+  void StartStoreRobotAuth();
 
   // Handles completion of the robot token store operation.
-  void HandleRobotAuthTokenStored(bool result);
+  void HandleStoreRobotAuthTokenResult(bool result);
 
   // Drops any ongoing actions.
   void Stop();

chrome/browser/chromeos/policy/server_backed_state_keys_broker.h

@@ -24,9 +24,9 @@ class SessionManagerClient;
 
 namespace policy {
 
-// Brokers server-backed state keys for the device. Retrieves them from session
-// manager via DBus and refreshes them periodically. Consumers can register
-// callbacks to invoke when the state keys change.
+// Brokers server-backed FRE state keys for the device. Retrieves them from
+// session manager via DBus and refreshes them periodically. Consumers can
+// register callbacks to invoke when the state keys change.
 class ServerBackedStateKeysBroker {
  public:
   typedef scoped_ptr<base::CallbackList<void()>::Subscription> Subscription;