Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

 // Copyright 2016 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_navigation_throttle.h"
 
 #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"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 
 namespace arc {
 
 namespace {
 
@@ skipping 136 matching lines @@
 }
 
 void ArcNavigationThrottle::OnDisambigDialogClosed(
     mojo::Array<mojom::UrlHandlerInfoPtr> handlers,
     size_t selected_app_index,
     CloseReason close_reason) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   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 || selected_app_index >= handlers.size()) {
+    close_reason = CloseReason::REASON_ERROR;
   }
-  mojom::IntentHelperInstance* bridge = GetIntentHelper();
-  if (!bridge) {
-    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;
   }
-  if (close_reason == CloseReason::REASON_ALWAYS_PRESSED) {
-    bridge->AddPreferredPackage(handlers[selected_app_index]->package_name);
-  }
-  if (ArcIntentHelperBridge::IsIntentHelperPackage(
-          handlers[selected_app_index]->package_name)) {
-    handle->Resume();
-    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