Recategorize ARC auth sign in errors.

chrome/browser/chromeos/arc/arc_auth_service.cc

Index: chrome/browser/chromeos/arc/arc_auth_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_auth_service.cc b/chrome/browser/chromeos/arc/arc_auth_service.cc
index c20b7401e98994688138b9c3ba3d02006c883521..643086e4e790a875a8e1d846479fbcae35678592 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -87,6 +87,47 @@ ash::ShelfDelegate* 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)
@@ -198,7 +239,7 @@ void ArcAuthService::OnBridgeStopped(ArcBridgeService::StopReason reason) {
     // 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;
@@ -258,6 +299,11 @@ void ArcAuthService::OnSignInComplete() {
 }
 
 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());
@@ -266,42 +312,41 @@ void ArcAuthService::OnSignInFailed(arc::mojom::ArcSignInFailureReason reason) {
 
   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.