Recategorize ARC auth sign in errors.

chrome/browser/chromeos/arc/arc_auth_service.cc

 // Copyright 2015 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/arc/arc_auth_service.h"
 
 #include <utility>
 
 #include "ash/shelf/shelf_delegate.h"
 #include "ash/shell.h"
@@ skipping 69 matching lines @@
 }
 
 ash::ShelfDelegate* GetShelfDelegate() {
   if (g_shelf_delegate_for_testing)
     return g_shelf_delegate_for_testing;
   if (ash::Shell::HasInstance())
     return ash::Shell::GetInstance()->GetShelfDelegate();
   return nullptr;
 }
 
+ProvisioningResult ConvertArcSignInFailureReasonToProvisioningResult(
+    arc::mojom::ArcSignInFailureReason reason) {
+  using ArcSignInFailureReason = arc::mojom::ArcSignInFailureReason;
+
+  switch (reason) {
+    case ArcSignInFailureReason::UNKNOWN_ERROR:
+      return ProvisioningResult::UNKNOWN_ERROR;
+    case ArcSignInFailureReason::MOJO_VERSION_MISMATCH:
+      return ProvisioningResult::MOJO_VERSION_MISMATCH;
+    case ArcSignInFailureReason::MOJO_CALL_TIMEOUT:
+      return ProvisioningResult::MOJO_CALL_TIMEOUT;
+    case ArcSignInFailureReason::DEVICE_CHECK_IN_FAILED:
+      return ProvisioningResult::DEVICE_CHECK_IN_FAILED;
+    case ArcSignInFailureReason::GMS_SIGN_IN_FAILED:
+      return ProvisioningResult::GMS_SIGN_IN_FAILED;
+    case ArcSignInFailureReason::GMS_SIGN_IN_TIMEOUT:
+      return ProvisioningResult::GMS_SIGN_IN_TIMEOUT;
+    case ArcSignInFailureReason::GMS_NETWORK_ERROR:
+      return ProvisioningResult::GMS_NETWORK_ERROR;
+    case ArcSignInFailureReason::GMS_SERVICE_UNAVAILABLE:
+      return ProvisioningResult::GMS_SERVICE_UNAVAILABLE;
+    case ArcSignInFailureReason::GMS_BAD_AUTHENTICATION:
+      return ProvisioningResult::GMS_BAD_AUTHENTICATION;
+    case ArcSignInFailureReason::GMS_EMPTY_CONSUMER_PKG_OR_SIG:
+      return ProvisioningResult::GMS_EMPTY_CONSUMER_PKG_OR_SIG;
+    case ArcSignInFailureReason::GMS_NEEDS_2F:
+      return ProvisioningResult::GMS_NEEDS_2F;
+    case ArcSignInFailureReason::GMS_NEEDS_POST_SIGN_IN_FLOW:
+      return ProvisioningResult::GMS_NEEDS_POST_SIGN_IN_FLOW;
+    case ArcSignInFailureReason::GMS_NEEDS_BROWSER:
+      return ProvisioningResult::GMS_NEEDS_BROWSER;
+    case ArcSignInFailureReason::CLOUD_PROVISION_FLOW_FAILED:
+      return ProvisioningResult::CLOUD_PROVISION_FLOW_FAILED;
+    case ArcSignInFailureReason::CLOUD_PROVISION_FLOW_TIMEOUT:
+      return ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT;

[Junichi Uekawa, 2016/07/26 06:22:27]

my eyes hurt trying to compare and find a typo / copy-paste error 
in the list. We should let the compiler do such a job.

Some suggestions:

 1. You could create a map and do a map.find()
 2. #define MAP_ERROR_CODE(something) that generates case/return pair (or the
data).

[hidehiko, 2016/07/26 13:02:58]

Replaced by a macro.

+    default:
+      LOG(ERROR) << "unknown reason: " << static_cast<int>(reason);
+      return ProvisioningResult::UNKNOWN_ERROR;
+  }
+}
+
 }  // namespace
 
 ArcAuthService::ArcAuthService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service), binding_(this), weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_auth_service);
 
   g_arc_auth_service = this;
 
   arc_bridge_service()->AddObserver(this);
@@ skipping 91 matching lines @@
   arc_bridge_service()->auth()->instance()->Init(
       binding_.CreateInterfacePtrAndBind());
 }
 
 void ArcAuthService::OnBridgeStopped(ArcBridgeService::StopReason reason) {
   // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (waiting_for_reply_) {
     // Using SERVICE_UNAVAILABLE instead of UNKNOWN_ERROR, since the latter
     // causes this code to not try to stop ARC, so it would retry without the
     // user noticing.
-    OnSignInFailed(mojom::ArcSignInFailureReason::SERVICE_UNAVAILABLE);
+    OnSignInFailedInternal(ProvisioningResult::ARC_STOPPED);
   }
   if (!clear_required_)
     return;
   clear_required_ = false;
   chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->RemoveArcData(
       cryptohome::Identification(
           multi_user_util::GetAccountIdFromProfile(profile_)));
 }
 
 std::string ArcAuthService::GetAndResetAuthCode() {
@@ skipping 39 matching lines @@
   }
 
   profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
   CloseUI();
   UpdateProvisioningTiming(base::Time::Now() - sign_in_time_, true,
                            IsAccountManaged(profile_));
   UpdateProvisioningResultUMA(ProvisioningResult::SUCCESS);
 }
 
 void ArcAuthService::OnSignInFailed(arc::mojom::ArcSignInFailureReason reason) {
+  OnSignInFailedInternal(
+      ConvertArcSignInFailureReasonToProvisioningResult(reason));
+}
+
+void ArcAuthService::OnSignInFailedInternal(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::ACTIVE);
   DCHECK(!sign_in_time_.is_null());
 
   waiting_for_reply_ = false;
 
   UpdateProvisioningTiming(base::Time::Now() - sign_in_time_, false,
                            IsAccountManaged(profile_));
+  UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL);

[victorhsieh, 2016/07/25 20:30:41]

Why it's always CLOUD_PROVISION_FLOW_FAIL now?

[hidehiko, 2016/07/26 13:02:58]

The previous code is sending dupped error status via OptInCancelUMA and
ProvisiningResultUMA. Also, UpdateOptInCancelUMA is called in other places, so
the log is mixed.

With this, CLOUD_PROVISION_FLOW_FAIL means "sign-in failure", and provisioning
UMA stats is something drill-down. I'm not very sure if we should keep using the
same value or deprecate it and introduce another value, though.

+  UpdateProvisioningResultUMA(result);
+
   int error_message_id;
-  switch (reason) {
-    case mojom::ArcSignInFailureReason::NETWORK_ERROR:
+  switch (result) {
+    case ProvisioningResult::GMS_NETWORK_ERROR:
       error_message_id = IDS_ARC_SIGN_IN_NETWORK_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
-      UpdateProvisioningResultUMA(ProvisioningResult::NETWORK_ERROR);
       break;
-    case mojom::ArcSignInFailureReason::SERVICE_UNAVAILABLE:
+    case ProvisioningResult::GMS_SERVICE_UNAVAILABLE:

[victorhsieh, 2016/07/25 20:30:41]

FWIW, this SERVICE_UNAVAILABLE is just an edge case when it failed to contact
the server for unexpected reason.  I was confused to see this case not returning
IDS_..._GMS_NOT_AVIALABLE at first.

[hidehiko, 2016/07/26 13:02:58]

SERVICE_UNAVAILABLE was used various purpose in the client code,
so it was difficult to see what was happened via UMA.
Now, GMS_SERVICE_UNAVAILABLE is returned in case that Android GMS services tells
us it is unavailable.

As for message, changing UI is out of scope in this CL,
so let me keep it as is.
Let's revisit UI later.

+    case ProvisioningResult::GMS_SIGN_IN_FAILED:
+    case ProvisioningResult::GMS_SIGN_IN_TIMEOUT:
       error_message_id = IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::SERVICE_UNAVAILABLE);
-      UpdateProvisioningResultUMA(ProvisioningResult::SERVICE_UNAVAILABLE);
       break;
-    case mojom::ArcSignInFailureReason::BAD_AUTHENTICATION:
+    case ProvisioningResult::GMS_BAD_AUTHENTICATION:
+    case ProvisioningResult::GMS_EMPTY_CONSUMER_PKG_OR_SIG:
+    case ProvisioningResult::GMS_NEEDS_2F:
+    case ProvisioningResult::GMS_NEEDS_POST_SIGN_IN_FLOW:
+    case ProvisioningResult::GMS_NEEDS_BROWSER:
       error_message_id = IDS_ARC_SIGN_IN_BAD_AUTHENTICATION_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::BAD_AUTHENTICATION);
-      UpdateProvisioningResultUMA(ProvisioningResult::BAD_AUTHENTICATION);
       break;
-    case mojom::ArcSignInFailureReason::GMS_CORE_NOT_AVAILABLE:
+    case ProvisioningResult::DEVICE_CHECK_IN_FAILED:
       error_message_id = IDS_ARC_SIGN_IN_GMS_NOT_AVAILABLE_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::GMS_CORE_NOT_AVAILABLE);
-      UpdateProvisioningResultUMA(ProvisioningResult::GMS_CORE_NOT_AVAILABLE);
       break;
-    case mojom::ArcSignInFailureReason::CLOUD_PROVISION_FLOW_FAIL:
+    case ProvisioningResult::CLOUD_PROVISION_FLOW_FAILED:
+    case ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT:
       error_message_id = IDS_ARC_SIGN_IN_CLOUD_PROVISION_FLOW_FAIL_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL);
-      UpdateProvisioningResultUMA(
-          ProvisioningResult::CLOUD_PROVISION_FLOW_FAIL);
       break;
     default:
       error_message_id = IDS_ARC_SIGN_IN_UNKNOWN_ERROR;
-      UpdateOptInCancelUMA(OptInCancelReason::UNKNOWN_ERROR);
-      UpdateProvisioningResultUMA(ProvisioningResult::UNKNOWN_ERROR);
+      break;
   }
 
-  if (reason == mojom::ArcSignInFailureReason::CLOUD_PROVISION_FLOW_FAIL ||
-      reason == mojom::ArcSignInFailureReason::UNKNOWN_ERROR) {
+  if (result == ProvisioningResult::CLOUD_PROVISION_FLOW_FAILED ||
+      result == ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT ||
+      result == ProvisioningResult::UNKNOWN_ERROR) {
     clear_required_ = true;
     // We'll delay shutting down the bridge in this case to allow people to send
     // feedback.
     ShowUI(UIPage::ERROR_WITH_FEEDBACK,
            l10n_util::GetStringUTF16(error_message_id));
     return;
   }
 
   if (profile_->GetPrefs()->HasPrefPath(prefs::kArcSignedIn))
     profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
@@ skipping 447 matching lines @@
       return os << kStateFetchingCode;
     case ArcAuthService::State::ACTIVE:
       return os << kStateActive;
     default:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc