[Media Router] Fix RecordPresentationRequestUrlBySink.

When the native Cast MRP was disabled, the FixProviderId() hack was
mapping all Chromecast media sources to ProviderId::EXTENSION, which
is not expected by RecordPresentationRequestUrlBySink.

This patch records the histogram before invoking FixProviderId().

It also restores the unittest for RecordPresentationRequestUrlBySink,
which was deleted in http://crrev.com/c/2405519

Bug: 1112866
Change-Id: Iff2ea815c18458d6dcccb00974524c0ad382d461
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2468788
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: John Williams <jrw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818525}

chrome/browser/media/router/mojo/media_router_mojo_impl.cc

@@ -117,63 +117,6 @@ DesktopMediaPickerController::Params MakeDesktopPickerParams(
   return params;
 }
 
-// Records the possible ways a Presentation URL can be used to start a
-// presentation, both by the kind of URL and the type of the sink the URL will
-// be presented on.  "Normal" (https:, file:, or chrome-extension:) URLs are
-// typically implemented by loading them into an offscreen tab for streaming,
-// while Cast and DIAL URLs are sent directly to a compatible device.
-enum class PresentationUrlBySink {
-  kUnknown = 0,
-  kNormalUrlToChromecast = 1,
-  kNormalUrlToExtension = 2,
-  kNormalUrlToWiredDisplay = 3,
-  kCastUrlToChromecast = 4,
-  kDialUrlToDial = 5,
-  // Add new values immediately above this line.  Also update kMaxValue below
-  // and the enum of the same name in tools/metrics/histograms/enums.xml.
-  kMaxValue = kDialUrlToDial,
-};
-
-// NOTE: To record this on Android, will need to move to
-// //components/media_router and refactor to avoid the extensions dependency.
-void RecordPresentationRequestUrlBySink(const MediaSource& source,
-                                        MediaRouteProviderId provider_id) {
-  PresentationUrlBySink value = PresentationUrlBySink::kUnknown;
-  // URLs that can be rendered in offscreen tabs (for cloud or Chromecast
-  // sinks), or on a wired display.
-  bool is_normal_url = source.url().SchemeIs(url::kHttpsScheme) ||
-                       source.url().SchemeIs(extensions::kExtensionScheme) ||
-                       source.url().SchemeIs(url::kFileScheme);
-  switch (provider_id) {
-    case MediaRouteProviderId::EXTENSION:
-      if (is_normal_url) {
-        value = PresentationUrlBySink::kNormalUrlToExtension;
-      }
-      break;
-    case MediaRouteProviderId::WIRED_DISPLAY:
-      if (is_normal_url) {
-        value = PresentationUrlBySink::kNormalUrlToWiredDisplay;
-      }
-      break;
-    case MediaRouteProviderId::CAST:
-      if (source.IsCastPresentationUrl()) {
-        value = PresentationUrlBySink::kCastUrlToChromecast;
-      } else if (is_normal_url) {
-        value = PresentationUrlBySink::kNormalUrlToChromecast;
-      }
-      break;
-    case MediaRouteProviderId::DIAL:
-      if (source.IsDialSource()) {
-        value = PresentationUrlBySink::kDialUrlToDial;
-      }
-      break;
-    case MediaRouteProviderId::UNKNOWN:
-      break;
-  }
-  base::UmaHistogramEnumeration("MediaRouter.PresentationRequest.UrlBySink",
-                                value);
-}
-
 }  // namespace
 
 using SinkAvailability = mojom::MediaRouter::SinkAvailability;
@@ -329,13 +272,14 @@ void MediaRouterMojoImpl::CreateRoute(const MediaSource::Id& source_id,
   }
 
   MediaRouterMetrics::RecordMediaSinkType(sink->icon_type());
-  const MediaRouteProviderId provider_id = FixProviderId(sink->provider_id());
   // Record which of the possible ways the sink may render the source's
   // presentation URL (if it has one).
   if (source.url().is_valid()) {
-    RecordPresentationRequestUrlBySink(source, provider_id);
+    RecordPresentationRequestUrlBySink(source, sink->provider_id());
   }
 
+  const MediaRouteProviderId provider_id = FixProviderId(sink->provider_id());
+
   const std::string presentation_id = MediaRouterBase::CreatePresentationId();
   auto mr_callback = base::BindOnce(
       &MediaRouterMojoImpl::RouteResponseReceived, weak_factory_.GetWeakPtr(),
@@ -1104,6 +1048,51 @@ const MediaSink* MediaRouterMojoImpl::GetSinkById(
   return nullptr;
 }
 
+// NOTE: To record this on Android, will need to move to
+// //components/media_router and refactor to avoid the extensions dependency.
+void MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+    const MediaSource& source,
+    MediaRouteProviderId provider_id) {
+  PresentationUrlBySink value = PresentationUrlBySink::kUnknown;
+  // URLs that can be rendered in offscreen tabs (for cloud or Chromecast
+  // sinks), or on a wired display.
+  bool is_normal_url = source.url().SchemeIs(url::kHttpsScheme) ||
+                       source.url().SchemeIs(extensions::kExtensionScheme) ||
+                       source.url().SchemeIs(url::kFileScheme);
+  switch (provider_id) {
+    case MediaRouteProviderId::EXTENSION:
+      if (source.IsCastPresentationUrl()) {
+        // This "should not happen," but the code that creates media routes is
+        // tricky and we want to catch all possible cases.
+        value = PresentationUrlBySink::kCastUrlToChromecast;
+      } else if (is_normal_url) {
+        value = PresentationUrlBySink::kNormalUrlToExtension;
+      }
+      break;
+    case MediaRouteProviderId::WIRED_DISPLAY:
+      if (is_normal_url) {
+        value = PresentationUrlBySink::kNormalUrlToWiredDisplay;
+      }
+      break;
+    case MediaRouteProviderId::CAST:
+      if (source.IsCastPresentationUrl()) {
+        value = PresentationUrlBySink::kCastUrlToChromecast;
+      } else if (is_normal_url) {
+        value = PresentationUrlBySink::kNormalUrlToChromecast;
+      }
+      break;
+    case MediaRouteProviderId::DIAL:
+      if (source.IsDialSource()) {
+        value = PresentationUrlBySink::kDialUrlToDial;
+      }
+      break;
+    case MediaRouteProviderId::UNKNOWN:
+      break;
+  }
+  base::UmaHistogramEnumeration("MediaRouter.PresentationRequest.UrlBySink",
+                                value);
+}
+
 void MediaRouterMojoImpl::CreateRouteWithSelectedDesktop(
     MediaRouteProviderId provider_id,
     const std::string& sink_id,

chrome/browser/media/router/mojo/media_router_mojo_impl.h

@@ -20,6 +20,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/media/webrtc/desktop_media_picker_controller.h"
 #include "components/media_router/browser/issue_manager.h"
@@ -163,6 +164,8 @@ class MediaRouterMojoImpl : public MediaRouterBase, public mojom::MediaRouter {
                            PresentationConnectionStateChangedCallback);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
                            PresentationConnectionStateChangedCallbackRemoved);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImpl,
+                           TestRecordPresentationRequestUrlBySink);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterDesktopTest,
                            SyncStateToMediaRouteProvider);
   FRIEND_TEST_ALL_PREFIXES(ExtensionMediaRouteProviderProxyTest,
@@ -392,6 +395,28 @@ class MediaRouterMojoImpl : public MediaRouterBase, public mojom::MediaRouter {
   // Returns a nullptr if none is found.
   const MediaSink* GetSinkById(const MediaSink::Id& sink_id) const;
 
+  // Used by RecordPresentationRequestUrlBySink to record the possible ways a
+  // Presentation URL can be used to start a presentation, both by the kind of
+  // URL and the type of the sink the URL will be presented on.  "Normal"
+  // (https:, file:, or chrome-extension:) URLs are typically implemented by
+  // loading them into an offscreen tab for streaming, while Cast and DIAL URLs
+  // are sent directly to a compatible device.
+  enum class PresentationUrlBySink {
+    kUnknown = 0,
+    kNormalUrlToChromecast = 1,
+    kNormalUrlToExtension = 2,
+    kNormalUrlToWiredDisplay = 3,
+    kCastUrlToChromecast = 4,
+    kDialUrlToDial = 5,
+    // Add new values immediately above this line.  Also update kMaxValue below
+    // and the enum of the same name in tools/metrics/histograms/enums.xml.
+    kMaxValue = kDialUrlToDial,
+  };
+
+  static void RecordPresentationRequestUrlBySink(
+      const MediaSource& source,
+      MediaRouteProviderId provider_id);
+
   base::flat_map<MediaSource::Id, std::unique_ptr<MediaSinksQuery>>
       sinks_queries_;
 

chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
+#include "base/strings/strcat.h"
 #include "base/test/gmock_callback_support.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
@@ -44,6 +45,7 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 using blink::mojom::PresentationConnectionCloseReason;
 using blink::mojom::PresentationConnectionState;
@@ -1264,4 +1266,50 @@ TEST_F(MediaRouterMojoImplTest, ObserveRoutesFromMultipleProviders) {
   UpdateRoutes(MediaRouteProviderId::WIRED_DISPLAY, {}, kSource, {});
 }
 
+TEST(MediaRouterMojoImpl, TestRecordPresentationRequestUrlBySink) {
+  using base::Bucket;
+  using PresentationUrlBySink = MediaRouterMojoImpl::PresentationUrlBySink;
+
+  MediaSource cast_source("cast:ABCD1234");
+  MediaSource dial_source(
+      GURL(base::StrCat({kCastDialPresentationUrlScheme, ":YouTube"})));
+  MediaSource presentation_url(GURL("https://www.example.com"));
+
+  base::HistogramTester tester;
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      cast_source, MediaRouteProviderId::CAST);
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      dial_source, MediaRouteProviderId::DIAL);
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      presentation_url, MediaRouteProviderId::CAST);
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      presentation_url, MediaRouteProviderId::WIRED_DISPLAY);
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      presentation_url, MediaRouteProviderId::EXTENSION);
+  // DIAL devices don't support normal URLs, so this will get logged as
+  // kUnknown.
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      presentation_url, MediaRouteProviderId::DIAL);
+  // Normally, Cast sources are sent to the CAST provider, but in case it gets
+  // routed to the EXTENSION provider instead.
+  MediaRouterMojoImpl::RecordPresentationRequestUrlBySink(
+      cast_source, MediaRouteProviderId::EXTENSION);
+
+  EXPECT_THAT(
+      tester.GetAllSamples("MediaRouter.PresentationRequest.UrlBySink"),
+      testing::UnorderedElementsAre(
+          Bucket(static_cast<int>(PresentationUrlBySink::kUnknown), 1),
+          Bucket(
+              static_cast<int>(PresentationUrlBySink::kNormalUrlToChromecast),
+              1),
+          Bucket(static_cast<int>(PresentationUrlBySink::kNormalUrlToExtension),
+                 1),
+          Bucket(
+              static_cast<int>(PresentationUrlBySink::kNormalUrlToWiredDisplay),
+              1),
+          Bucket(static_cast<int>(PresentationUrlBySink::kCastUrlToChromecast),
+                 2),
+          Bucket(static_cast<int>(PresentationUrlBySink::kDialUrlToDial), 1)));
+}
+
 }  // namespace media_router