Adding a destination platform histogram for UMA.

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

Index: chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
diff --git a/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc b/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
index f49f39f6e5b238741031a5bb3addcbba703ad0c2..f7010b60386db176a6ac8dbc4d2d0d294a78c786 100644
--- a/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
+++ b/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
@@ -40,10 +40,15 @@ constexpr uint32_t kMinVersionForHandleUrl = 2;
 constexpr uint32_t kMinVersionForRequestUrlHandlerList = 2;
 constexpr uint32_t kMinVersionForAddPreferredPackage = 7;
 
-void RecordUma(ArcNavigationThrottle::CloseReason close_reason) {
+void RecordUma(ArcNavigationThrottle::CloseReason close_reason,
+               ArcNavigationThrottle::Platform platform) {
   UMA_HISTOGRAM_ENUMERATION(
       "Arc.IntentHandlerAction", static_cast<int>(close_reason),
       static_cast<int>(ArcNavigationThrottle::CloseReason::SIZE));
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "Arc.IntentHandlerDestinationPlatform", static_cast<int>(platform),
+      static_cast<int>(ArcNavigationThrottle::Platform::SIZE));
 }
 
 // Shows the Chrome OS' original external protocol dialog as a fallback.
@@ -271,7 +276,11 @@ void OnIntentPickerClosed(int render_process_host_id,
       break;
     }
   }
-  RecordUma(close_reason);
+
+  ArcNavigationThrottle::Platform platform =
+      ArcNavigationThrottle::GetDestinationPlatform(selected_app_package,
+                                                    close_reason);
+  RecordUma(close_reason, platform);
 }
 
 // Called when ARC returned activity icons for the |handlers|.
@@ -324,8 +333,10 @@ void OnUrlHandlerList(int render_process_host_id,
   GetActionResult result;
   if (HandleUrl(render_process_host_id, routing_id, url, handlers,
                 handlers.size(), &result)) {
-    if (result == GetActionResult::HANDLE_URL_IN_ARC)
-      RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND);
+    RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND,

[Yusuke Sato, 2016/11/04 00:16:14]

When there's no app that can handle the custom |url|,
GetActionResult::SHOW_CHROME_OS_DIALOG is returned. In that case, we should not
record any UMA because neither ARC nor Chrome has handled the URL.

[djacobo_, 2016/11/04 05:53:50]

Makes sense, let me rollback the comments on histograms.xml, also putting back
line 327. Thanks for the clarification!!! 

+              result == GetActionResult::HANDLE_URL_IN_ARC

[Yusuke Sato, 2016/11/04 00:16:14]

Sorry that the enum does not have descriptions (please check the unit tests
instead that are actually very extensive...), but OPEN_URL_IN_CHROME is probably
not what you want to record UMA for.

In short, HANDLE_URL_IN_ARC is returned when there are one or more apps that can
handle the custom |url| _and_ one of them is a preferred one.

OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent:

 (e.g.
"intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end"
)

and 2) the package specified in the intent: |url| is not found in ARC++, and 3)
the intent: |url| has http or https S.browser_fallback_url.

In this case, HandleUrl() silently opens S.browser_fallback_url without asking
the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND.
OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed "Always" for
Chrome before, for example.

This is the reason why the current code only call RecordUma() for
HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) as
this file is for handling external protocols like intent: and
app-specific-custom-uri:.

[djacobo_, 2016/11/04 05:53:50]

Ok, so if I understand correctly I could Use the code as it was before in line
328, and I can still add ARC as the platform for those, but none of the
CloseReason entries matches the case you describe (keeping the traffic in Chrome
by using a fallback string).

I think we can still record this for the new histogram, but we need another
entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought about
APP_NOT_INSTALLED, but naming things is difficult to me), WDYT?
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe...

[Yusuke Sato, 2016/11/04 16:18:06]

Re FALLBACK_STRING_USED, I'd defer the decision to Elijah. Adding it in a
separate CL later seems also fine because Chrome for Chrome OS rarely handles
intent: anyway.

[djacobo_, 2016/11/04 17:43:43]

Acknowledged.

[elijahtaylor (use chromium), 2016/11/04 20:26:19]

fallback string in a different CL sgtm

Just to clarify, we are also setting PREFERRED_ACTIVITY_FOUND when chrome is
used as the default from an "always" choice?

[Yusuke Sato, 2016/11/04 20:42:33]

If Chrome is used as the default (aka Always) when clicking an http(s) link,
yes, ArcNavigationThrottle::OnIntentPickerClosed() records
PREFERRED_ACTIVITY_FOUND with Platform::CHROME.

+                  ? ArcNavigationThrottle::Platform::ARC
+                  : ArcNavigationThrottle::Platform::CHROME);
     return;  // the |url| has been handled.
   }