Media Router: Simplify app availability

This patch fixes some issues with tab sink availability by modifying the
MediaRouterMojoImpl to have a single SinksQuery for all tabs,
tabs_sinks_query_, instead of registering a unique sinks query for each
unique tab media source id.

In places where a media source id is expected, specifically for the
media router providers themselves, a hardcoded media source id for tab 0
is passed.

Bug: 1013769, b/154115531
Change-Id: I8cdb05fba9aa813153e66bf3af8e5f3d17581903
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2200015
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771963}

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

@@ -205,7 +205,7 @@ void MediaRouterMojoImpl::OnSinksReceived(
     const std::vector<url::Origin>& origins) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DVLOG_WITH_INSTANCE(1) << "OnSinksReceived";
-  auto it = sinks_queries_.find(media_source);
+  auto it = sinks_queries_.find(MediaSinksQuery::GetKey(media_source).id());
   if (it == sinks_queries_.end()) {
     DVLOG_WITH_INSTANCE(1) << "Received sink list without MediaSinksQuery.";
     return;
@@ -486,6 +486,25 @@ void MediaRouterMojoImpl::GetMediaController(
       std::move(callback));
 }
 
+// static
+MediaSource MediaRouterMojoImpl::MediaSinksQuery::GetKey(
+    const MediaSource::Id& id) {
+  MediaSource source(id);
+  if (source.IsTabMirroringSource()) {
+    return MediaSource::ForAnyTab();
+  }
+  return source;
+}
+
+// static
+MediaSource MediaRouterMojoImpl::MediaSinksQuery::GetKey(
+    const MediaSinksObserver& observer) {
+  if (!observer.source()) {
+    return MediaSource{""};
+  }
+  return GetKey(observer.source()->id());
+}
+
 void MediaRouterMojoImpl::MediaSinksQuery::SetSinksForProvider(
     MediaRouteProviderId provider_id,
     const std::vector<MediaSink>& sinks) {
@@ -652,12 +671,11 @@ void MediaRouterMojoImpl::ProviderSinkAvailability::
 bool MediaRouterMojoImpl::RegisterMediaSinksObserver(
     MediaSinksObserver* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
   // Create an observer list for the media source and add |observer|
   // to it. Fail if |observer| is already registered.
-  const std::string& source_id =
-      observer->source() ? observer->source()->id() : "";
-  std::unique_ptr<MediaSinksQuery>& sinks_query = sinks_queries_[source_id];
+
+  const MediaSource source = MediaSinksQuery::GetKey(*observer);
+  std::unique_ptr<MediaSinksQuery>& sinks_query = sinks_queries_[source.id()];
   bool is_new_query = false;
   if (!sinks_query) {
     is_new_query = true;
@@ -672,7 +690,7 @@ bool MediaRouterMojoImpl::RegisterMediaSinksObserver(
   if (is_new_query) {
     for (const auto& provider : media_route_providers_) {
       if (sink_availability_.IsAvailableForProvider(provider.first)) {
-        provider.second->StartObservingMediaSinks(source_id);
+        provider.second->StartObservingMediaSinks(source.id());
       }
     }
   }
@@ -683,9 +701,8 @@ void MediaRouterMojoImpl::UnregisterMediaSinksObserver(
     MediaSinksObserver* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  const std::string& source_id =
-      observer->source() ? observer->source()->id() : "";
-  auto it = sinks_queries_.find(source_id);
+  const MediaSource source = MediaSinksQuery::GetKey(*observer);
+  auto it = sinks_queries_.find(source.id());
   if (it == sinks_queries_.end() || !it->second->HasObserver(observer))
     return;
 
@@ -694,15 +711,17 @@ void MediaRouterMojoImpl::UnregisterMediaSinksObserver(
   // HasObservers() is reliable here on the assumption that this call
   // is not inside the ObserverList iteration.
   it->second->RemoveObserver(observer);
-  if (!it->second->HasObservers()) {
+  // Since all tabs share the tab sinks query, we don't want to delete it
+  // here.
+  if (!it->second->HasObservers() && !source.IsTabMirroringSource()) {
     // Only ask MRPs to stop observing media sinks if there are sinks available.
     // Otherwise, the MRPs would have discarded the queries already.
     for (const auto& provider : media_route_providers_) {
       if (sink_availability_.IsAvailableForProvider(provider.first)) {
-        provider.second->StopObservingMediaSinks(source_id);
+        provider.second->StopObservingMediaSinks(source.id());
       }
     }
-    sinks_queries_.erase(source_id);
+    sinks_queries_.erase(source.id());
   }
 }
 

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

@@ -150,43 +150,14 @@ class MediaRouterMojoImpl : public MediaRouterBase, public mojom::MediaRouter {
   friend class MediaRouterFactory;
   friend class MediaRouterMojoImplTest;
   friend class MediaRouterMojoTest;
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest, JoinRoute);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest, JoinRouteTimedOutFails);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
                            JoinRouteIncognitoMismatchFails);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           IncognitoRoutesTerminatedOnProfileShutdown);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RegisterAndUnregisterMediaSinksObserver);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RegisterMediaSinksObserverWithAvailabilityChange);
-  FRIEND_TEST_ALL_PREFIXES(
-      MediaRouterMojoImplTest,
-      RegisterAndUnregisterMediaSinksObserverWithAvailabilityChange);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RegisterAndUnregisterMediaRoutesObserver);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RouteMessagesSingleObserver);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RouteMessagesMultipleObservers);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest, HandleIssue);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest, GetMediaController);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           FailToCreateRouteController);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           RegisterMediaRoutesObserver_DedupingWithCache);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
                            PresentationConnectionStateChangedCallback);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
                            PresentationConnectionStateChangedCallbackRemoved);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           SendSinkRequestsToMultipleProviders);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           SendRouteRequestsToMultipleProviders);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           ObserveSinksFromMultipleProviders);
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterMojoImplTest,
-                           ObserveRoutesFromMultipleProviders);
   FRIEND_TEST_ALL_PREFIXES(MediaRouterDesktopTest,
                            SyncStateToMediaRouteProvider);
   FRIEND_TEST_ALL_PREFIXES(ExtensionMediaRouteProviderProxyTest,
@@ -199,6 +170,9 @@ class MediaRouterMojoImpl : public MediaRouterBase, public mojom::MediaRouter {
     MediaSinksQuery();
     ~MediaSinksQuery();
 
+    static MediaSource GetKey(const MediaSource::Id& source_id);
+    static MediaSource GetKey(const MediaSinksObserver& observer);
+
     // Caches the list of sinks for the provider returned from the query.
     void SetSinksForProvider(MediaRouteProviderId provider_id,
                              const std::vector<MediaSink>& sinks);

chrome/common/media_router/media_source.cc

@@ -21,6 +21,7 @@ namespace {
 
 // Prefixes used to format and detect various protocols' media source URNs.
 // See: https://www.ietf.org/rfc/rfc3406.txt
+constexpr char kAnyTabMediaUrn[] = "urn:x-org.chromium.media:source:tab:*";
 constexpr char kTabMediaUrnFormat[] = "urn:x-org.chromium.media:source:tab:%d";
 constexpr base::StringPiece kDesktopMediaUrnPrefix =
     "urn:x-org.chromium.media:source:desktop:";
@@ -67,6 +68,11 @@ MediaSource::MediaSource(const GURL& presentation_url)
 
 MediaSource::~MediaSource() = default;
 
+// static
+MediaSource MediaSource::ForAnyTab() {
+  return MediaSource(std::string(kAnyTabMediaUrn));
+}
+
 // static
 MediaSource MediaSource::ForTab(int tab_id) {
   return MediaSource(base::StringPrintf(kTabMediaUrnFormat, tab_id));
@@ -87,18 +93,19 @@ MediaSource MediaSource::ForPresentationUrl(const GURL& presentation_url) {
   return MediaSource(presentation_url);
 }
 
+bool MediaSource::IsTabMirroringSource() const {
+  int tab_id;
+  return id() == kAnyTabMediaUrn ||
+         (std::sscanf(id().c_str(), kTabMediaUrnFormat, &tab_id) == 1 &&
+          tab_id > 0);
+}
+
 bool MediaSource::IsDesktopMirroringSource() const {
   return id() == kUnknownDesktopMediaUrn ||
          base::StartsWith(id(), kDesktopMediaUrnPrefix,
                           base::CompareCase::SENSITIVE);
 }
 
-bool MediaSource::IsTabMirroringSource() const {
-  int tab_id;
-  return std::sscanf(id_.c_str(), kTabMediaUrnFormat, &tab_id) == 1 &&
-         tab_id > 0;
-}
-
 bool MediaSource::IsMirroringSource() const {
   return IsDesktopMirroringSource() || IsTabMirroringSource();
 }
@@ -125,7 +132,7 @@ base::Optional<std::string> MediaSource::DesktopStreamId() const {
 }
 
 bool MediaSource::IsValid() const {
-  return TabId() > 0 || IsDesktopMirroringSource() ||
+  return IsTabMirroringSource() || IsDesktopMirroringSource() ||
          IsValidPresentationUrl(GURL(id_));
 }
 

chrome/common/media_router/media_source.h

@@ -90,6 +90,7 @@ class MediaSource {
 
   // Protocol-specific media source object creation.
   // Returns MediaSource URI depending on the type of source.
+  static MediaSource ForAnyTab();
   static MediaSource ForTab(int tab_id);
   static MediaSource ForPresentationUrl(const GURL& presentation_url);
 
@@ -107,9 +108,8 @@ class MediaSource {
   // extension-based Cast MRP is removed.
   static MediaSource ForDesktop();
 
-  // Returns true if source outputs its content via mirroring.
-  bool IsDesktopMirroringSource() const;
   bool IsTabMirroringSource() const;
+  bool IsDesktopMirroringSource() const;
   bool IsMirroringSource() const;
 
   // Returns true if this is represents a Cast Presentation URL.

chrome/common/media_router/media_source_unittest.cc

@@ -54,6 +54,18 @@ TEST(MediaSourceTest, ConstructorWithURLString) {
   EXPECT_EQ(test_url, source1.url());
 }
 
+TEST(MediaSourceTest, ForAnyTab) {
+  auto source = MediaSource::ForAnyTab();
+  EXPECT_EQ("urn:x-org.chromium.media:source:tab:*", source.id());
+  EXPECT_EQ(-1, source.TabId());
+  EXPECT_TRUE(source.IsValid());
+  EXPECT_FALSE(source.IsDesktopMirroringSource());
+  EXPECT_TRUE(source.IsTabMirroringSource());
+  EXPECT_TRUE(source.IsMirroringSource());
+  EXPECT_FALSE(source.IsCastPresentationUrl());
+  EXPECT_FALSE(source.IsDialSource());
+}
+
 TEST(MediaSourceTest, ForTab) {
   auto source = MediaSource::ForTab(123);
   EXPECT_EQ("urn:x-org.chromium.media:source:tab:123", source.id());