Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

Index: chrome/browser/chromeos/arc/arc_navigation_throttle.cc
diff --git a/chrome/browser/chromeos/arc/arc_navigation_throttle.cc b/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
index e63e77f4aae133af43bd5b4716a1e21783c64af1..0c1944c5043e1d34103cd7789560e3e82f07aac1 100644
--- a/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
+++ b/chrome/browser/chromeos/arc/arc_navigation_throttle.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/histogram_macros.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
@@ -163,34 +164,44 @@ void ArcNavigationThrottle::OnDisambigDialogClosed(
   const GURL& url = navigation_handle()->GetURL();
   content::NavigationHandle* handle = navigation_handle();
 
-  // TODO(djacobo): Record UMA
-  // If the user fails to select an option from the list, or the UI returned an
-  // error or if |selected_app_index| is not a valid index, then resume the
-  // navigation in Chrome. Otherwise store the preferred app (if any) and start
-  // the selected app, either Chrome Browser or ARC app.
-  if (close_reason == CloseReason::REASON_DIALOG_DEACTIVATED ||
-      close_reason == CloseReason::REASON_ERROR ||
-      selected_app_index >= handlers.size()) {
-    DVLOG(1) << "User didn't select a valid option, resuming navigation.";
-    handle->Resume();
-    return;
-  }
   mojom::IntentHelperInstance* bridge = GetIntentHelper();
-  if (!bridge) {
-    handle->Resume();
-    return;
-  }
-  if (close_reason == CloseReason::REASON_ALWAYS_PRESSED) {
-    bridge->AddPreferredPackage(handlers[selected_app_index]->package_name);
+  if (!bridge || selected_app_index >= handlers.size()) {
+    close_reason = CloseReason::REASON_ERROR;
   }
-  if (ArcIntentHelperBridge::IsIntentHelperPackage(
-          handlers[selected_app_index]->package_name)) {
-    handle->Resume();
-    return;
+
+  switch (close_reason) {
+    case CloseReason::REASON_ALWAYS_PRESSED:
+      bridge->AddPreferredPackage(handlers[selected_app_index]->package_name);
+    // fall through

[Luis Héctor Chávez, 2016/06/27 16:01:11]

nit: Can you run git cl format? I suspect that this should be aligned with the
line above (which also matches other "fall through" comments in
chrome/browser/chromeos)

[Yusuke Sato, 2016/06/27 16:19:32]

git cl format did this actually.. and I don't like the formatting either. Added
{} to force the indent level.

+    case CloseReason::REASON_JUST_ONCE_PRESSED:
+    case CloseReason::REASON_PREFERRED_ACTIVITY_FOUND:
+      if (ArcIntentHelperBridge::IsIntentHelperPackage(
+              handlers[selected_app_index]->package_name)) {
+        handle->Resume();
+      } else {
+        bridge->HandleUrl(url.spec(),
+                          handlers[selected_app_index]->package_name);
+        handle->CancelDeferredNavigation(
+            content::NavigationThrottle::CANCEL_AND_IGNORE);
+      }
+      break;
+    case CloseReason::REASON_DIALOG_DEACTIVATED:
+    case CloseReason::REASON_ERROR:
+      // If the user fails to select an option from the list, or the UI returned
+      // an error or if |selected_app_index| is not a valid index, then resume
+      // the navigation in Chrome. Otherwise store the preferred app (if any)

[Luis Héctor Chávez, 2016/06/27 16:01:11]

The "Otherwise ..." part of this comment does not apply anymore since that
appears before in the code. Please remove it or move it to where the code for
that part is.

[Yusuke Sato, 2016/06/27 16:19:33]

Good catch, removed. The comment was added when |close_reason| was a boolean.
Now that it is an enum, the comment is probably redundant. Done.

+      // and start the selected app, either Chrome Browser or ARC app.
+      DVLOG(1) << "User didn't select a valid option, resuming navigation.";
+      handle->Resume();
+      break;
+    case CloseReason::SIZE:

[Luis Héctor Chávez, 2016/06/27 16:01:11]

nit: default instead of an explicit case.

[Yusuke Sato, 2016/06/27 16:19:32]

I tend to avoid having default: when using switch-case for enums because clang
automatically detects and warns missing case:s when new values are added to the
enum class. Using default: disables the compile time check which seems to be
more error prone. WDYT?

+      NOTREACHED();
+      return;
   }
-  bridge->HandleUrl(url.spec(), handlers[selected_app_index]->package_name);
-  handle->CancelDeferredNavigation(
-      content::NavigationThrottle::CANCEL_AND_IGNORE);
+
+  UMA_HISTOGRAM_ENUMERATION("Arc.IntentHandlerAction",
+                            static_cast<int>(close_reason),
+                            static_cast<int>(CloseReason::SIZE));
 }
 
 }  // namespace arc