Improve error metric for ChromeOS.Apps.IntentPickerAction

Dividing the intent picker error metric between before and after picker
shown. This allows us to better understand the error metrics to isolate where
errors are occurring.

BUG=983408

Test: Manual testing, unit_tests
Change-Id: I25c0db3531cbe08ebf5df282a40ca3244e4d8eb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1701023Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Melissa Zhang <melzhang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678054}

chrome/browser/apps/intent_helper/apps_navigation_throttle.cc

@@ -284,7 +284,8 @@ AppsNavigationThrottle::Platform AppsNavigationThrottle::GetDestinationPlatform(
       return Platform::ARC;
     case PickerAction::PWA_APP_PRESSED:
       return Platform::PWA;
-    case PickerAction::PICKER_ERROR:
+    case PickerAction::ERROR_BEFORE_PICKER:
+    case PickerAction::ERROR_AFTER_PICKER:
     case PickerAction::DIALOG_DEACTIVATED:
     case PickerAction::CHROME_PRESSED:
     case PickerAction::CHROME_PREFERRED_PRESSED:
@@ -414,8 +415,10 @@ AppsNavigationThrottle::PickerAction AppsNavigationThrottle::GetPickerAction(
     IntentPickerCloseReason close_reason,
     bool should_persist) {
   switch (close_reason) {
-    case IntentPickerCloseReason::PICKER_ERROR:
-      return PickerAction::PICKER_ERROR;
+    case IntentPickerCloseReason::ERROR_BEFORE_PICKER:
+      return PickerAction::ERROR_BEFORE_PICKER;
+    case IntentPickerCloseReason::ERROR_AFTER_PICKER:
+      return PickerAction::ERROR_AFTER_PICKER;
     case IntentPickerCloseReason::DIALOG_DEACTIVATED:
       return PickerAction::DIALOG_DEACTIVATED;
     case IntentPickerCloseReason::PREFERRED_APP_FOUND:

chrome/browser/apps/intent_helper/apps_navigation_throttle.h

@@ -104,7 +104,8 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
   // and need to be synced with histograms.xml. This enum class should also be
   // treated as append-only.
   enum class PickerAction : int {
-    PICKER_ERROR = 0,
+    // Picker errors occurring after the picker is shown.
+    ERROR_AFTER_PICKER = 0,
     // DIALOG_DEACTIVATED keeps track of the user dismissing the UI via clicking
     // the close button or clicking outside of the IntentPickerBubbleView
     // surface. As with CHROME_PRESSED, the user stays in Chrome, however we
@@ -124,7 +125,9 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
     ARC_APP_PRESSED = 7,
     ARC_APP_PREFERRED_PRESSED = 8,
     PWA_APP_PRESSED = 9,
-    INVALID = 10,
+    // Picker errors occurring before the picker is shown.
+    ERROR_BEFORE_PICKER = 10,
+    INVALID = 11,
     kMaxValue = INVALID,
   };
 

chrome/browser/apps/intent_helper/apps_navigation_throttle_unittest.cc

@@ -104,30 +104,53 @@ TEST(AppsNavigationThrottleTest, TestShouldOverrideUrlLoading) {
 }
 
 TEST(AppsNavigationThrottleTest, TestGetPickerAction) {
-  // Expect PickerAction::PICKER_ERROR if the close_reason is ERROR.
-  EXPECT_EQ(
-      AppsNavigationThrottle::PickerAction::PICKER_ERROR,
-      AppsNavigationThrottle::GetPickerAction(
-          apps::mojom::AppType::kUnknown, IntentPickerCloseReason::PICKER_ERROR,
-          /*should_persist=*/true));
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kUnknown,
+                IntentPickerCloseReason::ERROR_BEFORE_PICKER,
+                /*should_persist=*/true));
 
-  EXPECT_EQ(
-      AppsNavigationThrottle::PickerAction::PICKER_ERROR,
-      AppsNavigationThrottle::GetPickerAction(
-          apps::mojom::AppType::kArc, IntentPickerCloseReason::PICKER_ERROR,
-          /*should_persist=*/true));
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kArc,
+                IntentPickerCloseReason::ERROR_BEFORE_PICKER,
+                /*should_persist=*/true));
 
-  EXPECT_EQ(
-      AppsNavigationThrottle::PickerAction::PICKER_ERROR,
-      AppsNavigationThrottle::GetPickerAction(
-          apps::mojom::AppType::kUnknown, IntentPickerCloseReason::PICKER_ERROR,
-          /*should_persist=*/false));
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kUnknown,
+                IntentPickerCloseReason::ERROR_BEFORE_PICKER,
+                /*should_persist=*/false));
 
-  EXPECT_EQ(
-      AppsNavigationThrottle::PickerAction::PICKER_ERROR,
-      AppsNavigationThrottle::GetPickerAction(
-          apps::mojom::AppType::kArc, IntentPickerCloseReason::PICKER_ERROR,
-          /*should_persist=*/false));
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kArc,
+                IntentPickerCloseReason::ERROR_BEFORE_PICKER,
+                /*should_persist=*/false));
+
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kUnknown,
+                IntentPickerCloseReason::ERROR_AFTER_PICKER,
+                /*should_persist=*/true));
+
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kArc,
+                IntentPickerCloseReason::ERROR_AFTER_PICKER,
+                /*should_persist=*/true));
+
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kUnknown,
+                IntentPickerCloseReason::ERROR_AFTER_PICKER,
+                /*should_persist=*/false));
+
+  EXPECT_EQ(AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER,
+            AppsNavigationThrottle::GetPickerAction(
+                apps::mojom::AppType::kArc,
+                IntentPickerCloseReason::ERROR_AFTER_PICKER,
+                /*should_persist=*/false));
 
   // Expect PickerAction::DIALOG_DEACTIVATED if the close_reason is
   // DIALOG_DEACTIVATED.
@@ -237,12 +260,22 @@ TEST(AppsNavigationThrottleTest, TestGetDestinationPlatform) {
 
   // When the PickerAction is either ERROR or DIALOG_DEACTIVATED we MUST stay in
   // Chrome not taking into account the selected_app_package.
-  EXPECT_EQ(AppsNavigationThrottle::Platform::CHROME,
-            AppsNavigationThrottle::GetDestinationPlatform(
-                app_id, AppsNavigationThrottle::PickerAction::PICKER_ERROR));
-  EXPECT_EQ(AppsNavigationThrottle::Platform::CHROME,
-            AppsNavigationThrottle::GetDestinationPlatform(
-                app_id, AppsNavigationThrottle::PickerAction::PICKER_ERROR));
+  EXPECT_EQ(
+      AppsNavigationThrottle::Platform::CHROME,
+      AppsNavigationThrottle::GetDestinationPlatform(
+          app_id, AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER));
+  EXPECT_EQ(
+      AppsNavigationThrottle::Platform::CHROME,
+      AppsNavigationThrottle::GetDestinationPlatform(
+          app_id, AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER));
+  EXPECT_EQ(
+      AppsNavigationThrottle::Platform::CHROME,
+      AppsNavigationThrottle::GetDestinationPlatform(
+          app_id, AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER));
+  EXPECT_EQ(
+      AppsNavigationThrottle::Platform::CHROME,
+      AppsNavigationThrottle::GetDestinationPlatform(
+          app_id, AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER));
   EXPECT_EQ(
       AppsNavigationThrottle::Platform::CHROME,
       AppsNavigationThrottle::GetDestinationPlatform(

chrome/browser/apps/intent_helper/apps_navigation_types.h

@@ -17,8 +17,11 @@ namespace apps {
 
 // Describes the possible ways for the intent picker to be closed.
 enum class IntentPickerCloseReason {
-  // There was an error in showing the intent picker.
-  PICKER_ERROR,
+  // An error occurred in the intent picker before it could be displayed.
+  ERROR_BEFORE_PICKER,
+
+  // An error occurred in the intent picker after it was displayed.
+  ERROR_AFTER_PICKER,
 
   // The user dismissed the picker without making a choice.
   DIALOG_DEACTIVATED,

chrome/browser/chromeos/apps/intent_helper/chromeos_apps_navigation_throttle.cc

@@ -74,7 +74,7 @@ void ChromeOsAppsNavigationThrottle::OnIntentPickerClosed(
               url, launch_name, should_launch_app, should_persist)) {
         CloseOrGoBack(web_contents);
       } else {
-        close_reason = apps::IntentPickerCloseReason::PICKER_ERROR;
+        close_reason = apps::IntentPickerCloseReason::ERROR_AFTER_PICKER;
       }
       return;
     case apps::mojom::AppType::kUnknown:
@@ -154,7 +154,8 @@ ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
     case PickerAction::ARC_APP_PRESSED:
     case PickerAction::ARC_APP_PREFERRED_PRESSED:
     case PickerAction::PWA_APP_PRESSED:
-    case PickerAction::PICKER_ERROR:
+    case PickerAction::ERROR_BEFORE_PICKER:
+    case PickerAction::ERROR_AFTER_PICKER:
     case PickerAction::DIALOG_DEACTIVATED:
     case PickerAction::CHROME_PRESSED:
     case PickerAction::CHROME_PREFERRED_PRESSED:

chrome/browser/chromeos/apps/intent_helper/chromeos_apps_navigation_throttle_unittest.cc

@@ -17,14 +17,26 @@ TEST(ChromeOsAppsNavigationThrottleTest, TestGetDestinationPlatform) {
 
   // When the PickerAction is either ERROR or DIALOG_DEACTIVATED we MUST stay in
   // Chrome not taking into account the selected_app_package.
-  EXPECT_EQ(apps::AppsNavigationThrottle::Platform::CHROME,
-            ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
-                chrome_launch_name,
-                apps::AppsNavigationThrottle::PickerAction::PICKER_ERROR));
-  EXPECT_EQ(apps::AppsNavigationThrottle::Platform::CHROME,
-            ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
-                app_launch_name,
-                apps::AppsNavigationThrottle::PickerAction::PICKER_ERROR));
+  EXPECT_EQ(
+      apps::AppsNavigationThrottle::Platform::CHROME,
+      ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
+          chrome_launch_name,
+          apps::AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER));
+  EXPECT_EQ(
+      apps::AppsNavigationThrottle::Platform::CHROME,
+      ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
+          app_launch_name,
+          apps::AppsNavigationThrottle::PickerAction::ERROR_BEFORE_PICKER));
+  EXPECT_EQ(
+      apps::AppsNavigationThrottle::Platform::CHROME,
+      ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
+          chrome_launch_name,
+          apps::AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER));
+  EXPECT_EQ(
+      apps::AppsNavigationThrottle::Platform::CHROME,
+      ChromeOsAppsNavigationThrottle::GetDestinationPlatform(
+          app_launch_name,
+          apps::AppsNavigationThrottle::PickerAction::ERROR_AFTER_PICKER));
   EXPECT_EQ(
       apps::AppsNavigationThrottle::Platform::CHROME,
       ChromeOsAppsNavigationThrottle::GetDestinationPlatform(

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc

@@ -386,12 +386,13 @@ void OnIntentPickerClosed(int render_process_host_id,
   }
 
   if (!instance)
-    reason = apps::IntentPickerCloseReason::PICKER_ERROR;
+    reason = apps::IntentPickerCloseReason::ERROR_AFTER_PICKER;
 
   if (reason == apps::IntentPickerCloseReason::OPEN_APP ||
       reason == apps::IntentPickerCloseReason::STAY_IN_CHROME) {
     if (selected_app_index == handlers.size()) {
-      reason = apps::IntentPickerCloseReason::PICKER_ERROR;
+      // Selected app does not exist.
+      reason = apps::IntentPickerCloseReason::ERROR_AFTER_PICKER;
     }
   }
 
@@ -423,7 +424,11 @@ void OnIntentPickerClosed(int render_process_host_id,
       LOG(ERROR) << "Chrome is not a valid option for external protocol URLs";
       NOTREACHED();
       return;  // no UMA recording.
-    case apps::IntentPickerCloseReason::PICKER_ERROR:
+    case apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER:
+      // This can happen since an error could occur right before invoking
+      // Show() on the bubble's UI code.
+      FALLTHROUGH;
+    case apps::IntentPickerCloseReason::ERROR_AFTER_PICKER:
       LOG(ERROR) << "IntentPickerBubbleView returned CloseReason::ERROR: "
                  << "instance=" << instance
                  << ", selected_app_index=" << selected_app_index

chrome/browser/chromeos/arc/intent_helper/arc_intent_picker_app_fetcher.cc

@@ -236,7 +236,7 @@ void ArcIntentPickerAppFetcher::OnAppCandidatesReceivedForNavigation(
     DVLOG(1) << "There are no app candidates for this URL: " << url;
     chromeos::ChromeOsAppsNavigationThrottle::RecordUma(
         /*selected_app_package=*/std::string(), apps::mojom::AppType::kUnknown,
-        apps::IntentPickerCloseReason::PICKER_ERROR,
+        apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER,
         /*should_persist=*/false);
     std::move(callback).Run(apps::AppsNavigationAction::RESUME, {});
     return;
@@ -310,7 +310,7 @@ apps::PreferredPlatform ArcIntentPickerAppFetcher::DidLaunchPreferredArcApp(
     }
 
     if (!instance) {
-      close_reason = apps::IntentPickerCloseReason::PICKER_ERROR;
+      close_reason = apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER;
     } else if (ArcIntentHelperBridge::IsIntentHelperPackage(package_name)) {
       IntentPickerTabHelper::SetShouldShowIcon(web_contents(), true);
       preferred_platform = apps::PreferredPlatform::NATIVE_CHROME;
@@ -339,7 +339,7 @@ void ArcIntentPickerAppFetcher::GetArcAppIcons(
     LOG(ERROR) << "Cannot get an instance of ArcIntentHelperBridge";
     chromeos::ChromeOsAppsNavigationThrottle::RecordUma(
         /*selected_app_package=*/std::string(), apps::mojom::AppType::kUnknown,
-        apps::IntentPickerCloseReason::PICKER_ERROR,
+        apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER,
         /*should_persist=*/false);
     std::move(callback).Run({});
     return;

chrome/browser/ui/views/intent_picker_bubble_view.cc

@@ -134,7 +134,7 @@ views::Widget* IntentPickerBubbleView::ShowBubble(
   if (!browser || !BrowserView::GetBrowserViewForBrowser(browser)) {
     std::move(intent_picker_cb)
         .Run(kInvalidLaunchName, apps::mojom::AppType::kUnknown,
-             apps::IntentPickerCloseReason::PICKER_ERROR, false);
+             apps::IntentPickerCloseReason::ERROR_BEFORE_PICKER, false);
     return nullptr;
   }
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);

tools/metrics/histograms/enums.xml

@@ -2386,11 +2386,8 @@ Unknown properties are collapsed to zero. -->
 </enum>
 
 <enum name="ArcIntentHandlerAction">
-  <obsolete>
-    Deprecated as of 9/2018 (M71).
-  </obsolete>
   <summary>Defines Arc intent handler actions</summary>
-  <int value="0" label="Error"/>
+  <int value="0" label="Error after showing picker"/>
   <int value="1" label="Dialog deactivated"/>
   <int value="2" label="DEPRECATED: Always"/>
   <int value="3" label="DEPRECATED: Just once"/>
@@ -2400,6 +2397,7 @@ Unknown properties are collapsed to zero. -->
   <int value="7" label="ARC app pressed"/>
   <int value="8" label="ARC app preferred and pressed"/>
   <int value="9" label="Progressive web app pressed"/>
+  <int value="10" label="Error before showing picker"/>
 </enum>
 
 <enum name="ArcIntentHandlerDestinationPlatform">