[Media Router] Do not remove sinks not found by AnyMediaSinksObserver

Until all MRPs support empty-source sink queries, QueryResultManager::
AnyMediaSinksObserver may return an incomplete list of sinks. This CL
makes QueryResultManager keep track of sinks with sources and sources
returned by AnyMediaSinksObserver separate, and take their set union
when notifying observers.

Bug: 929225
Change-Id: I33f8caf789ce8033c5481b9fa86691e9d7f3f537
Reviewed-on: https://chromium-review.googlesource.com/c/1459338
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632299}

chrome/browser/media/router/media_sinks_observer.h

@@ -30,6 +30,9 @@ class MediaSinksObserver {
                      const MediaSource& source,
                      const url::Origin& origin);
   // Constructs an observer for all sinks known to |router|.
+  // NOTE: Not all Media Route Providers support sink queries with an empty
+  // source, so the returned sink list may be incomplete.
+  // TODO(crbug.com/929937): Fix this.
   explicit MediaSinksObserver(MediaRouter* router);
   virtual ~MediaSinksObserver();
 

chrome/browser/ui/media_router/media_sink_with_cast_modes.cc

@@ -9,6 +9,11 @@ namespace media_router {
 MediaSinkWithCastModes::MediaSinkWithCastModes(const MediaSink& sink)
     : sink(sink) {}
 
+MediaSinkWithCastModes::MediaSinkWithCastModes(
+    const MediaSink& sink,
+    std::initializer_list<MediaCastMode> cast_modes)
+    : sink(sink), cast_modes(cast_modes) {}
+
 MediaSinkWithCastModes::MediaSinkWithCastModes(
     const MediaSinkWithCastModes& other) = default;
 

chrome/browser/ui/media_router/media_sink_with_cast_modes.h

@@ -18,8 +18,11 @@ namespace media_router {
 // MediaSource.
 struct MediaSinkWithCastModes {
   explicit MediaSinkWithCastModes(const MediaSink& sink);
+  MediaSinkWithCastModes(const MediaSink& sink,
+                         std::initializer_list<MediaCastMode> cast_modes);
   MediaSinkWithCastModes(const MediaSinkWithCastModes& other);
   ~MediaSinkWithCastModes();
+
   bool operator==(const MediaSinkWithCastModes& other) const;
 
   MediaSink sink;

chrome/browser/ui/media_router/query_result_manager.cc

@@ -142,8 +142,8 @@ std::unique_ptr<MediaSource> QueryResultManager::GetSourceForCastModeAndSink(
     MediaCastMode cast_mode,
     MediaSink::Id sink_id) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  auto sink_entry = all_sinks_.find(sink_id);
-  if (sink_entry == all_sinks_.end())
+  auto sink_entry = sinks_with_sources_.find(sink_id);
+  if (sink_entry == sinks_with_sources_.end())
     return nullptr;
   return GetHighestPrioritySourceForCastModeAndSink(cast_mode,
                                                     sink_entry->second);
@@ -196,44 +196,35 @@ void QueryResultManager::SetSinksCompatibleWithSource(
 
   // (1) Iterate through current sink set, remove cast mode from those that
   // do not appear in latest result.
-  for (auto it = all_sinks_.begin(); it != all_sinks_.end(); ++it) {
+  for (auto it = sinks_with_sources_.begin(); it != sinks_with_sources_.end();
+       /* no-op */) {
     const MediaSink::Id& sink_id = it->first;
     CastModesWithMediaSources& sources_for_sink = it->second;
     if (!base::ContainsKey(new_sink_ids, sink_id))
       sources_for_sink.RemoveSource(cast_mode, source);
+    if (sources_for_sink.IsEmpty()) {
+      sinks_with_sources_.erase(it++);
+    } else {
+      ++it;
+    }
   }
 
   // (2) Add / update sinks with latest result.
   for (const MediaSink& sink : new_sinks) {
-    auto sink_it = all_sinks_.find(sink.id());
-    if (sink_it == all_sinks_.end()) {
-      sink_it =
-          all_sinks_.emplace(sink.id(), CastModesWithMediaSources(sink)).first;
+    auto sink_it = sinks_with_sources_.find(sink.id());
+    if (sink_it == sinks_with_sources_.end()) {
+      sink_it = sinks_with_sources_
+                    .emplace(sink.id(), CastModesWithMediaSources(sink))
+                    .first;
     } else {
       sink_it->second.set_sink(sink);
     }
-
     sink_it->second.AddSource(cast_mode, source);
   }
 }
 
-// A sink is added to |all_sinks_| in SetSinksCompatibleWithSource() when
-// MediaSourceMediaSinksObserver receives it, or in UpdateSinkList() when
-// AnyMediaSinksObserver receives it, whichever comes first.
-// A sink is removed from |all_sinks_| in UpdateSinkList() when
-// AnyMediaSinksObserver receives an update that does not contain it.
 void QueryResultManager::UpdateSinkList(const std::vector<MediaSink>& sinks) {
-  // Erase sinks in |all_sinks_| that do not appear in |sinks|.
-  base::EraseIf(all_sinks_, [&sinks](const auto& sink_pair) {
-    return std::find_if(sinks.begin(), sinks.end(),
-                        [&sink_pair](const MediaSink& sink) {
-                          return sink.id() == sink_pair.first;
-                        }) == sinks.end();
-  });
-  for (const MediaSink& sink : sinks) {
-    if (!base::ContainsKey(all_sinks_, sink.id()))
-      all_sinks_.emplace(sink.id(), sink);
-  }
+  all_sinks_ = sinks;
 }
 
 std::unique_ptr<MediaSource>
@@ -268,11 +259,15 @@ bool QueryResultManager::AreSourcesValidForCastMode(
 
 void QueryResultManager::NotifyOnResultsUpdated() {
   std::vector<MediaSinkWithCastModes> sinks;
-  for (const auto& sink_pair : all_sinks_) {
+  for (const auto& sink_pair : sinks_with_sources_) {
     MediaSinkWithCastModes sink_with_cast_modes(sink_pair.second.sink());
     sink_with_cast_modes.cast_modes = sink_pair.second.GetCastModes();
     sinks.push_back(sink_with_cast_modes);
   }
+  for (const auto& sink : all_sinks_) {
+    if (!base::ContainsKey(sinks_with_sources_, sink.id()))
+      sinks.push_back(MediaSinkWithCastModes(sink));
+  }
   for (QueryResultManager::Observer& observer : observers_)
     observer.OnResultsUpdated(sinks);
 }

chrome/browser/ui/media_router/query_result_manager.h

@@ -168,8 +168,16 @@ class QueryResultManager {
   // Holds registrations of MediaSources for cast modes.
   std::map<MediaCastMode, std::vector<MediaSource>> cast_mode_sources_;
 
-  // Holds all known sinks along with the cast modes and sources they support.
-  std::map<MediaSink::Id, CastModesWithMediaSources> all_sinks_;
+  // Holds sinks along with the cast modes and sources they support.
+  std::map<MediaSink::Id, CastModesWithMediaSources> sinks_with_sources_;
+
+  // Holds all known sinks, including those that do not support the sources in
+  // |cast_mode_sources_|.
+  // NOTE: Not all Media Route Providers support sink queries with an empty
+  // source, so |all_sinks_| may be missing some sinks that
+  // |sinks_with_sources_| has. Therefore the two collections must be tracked
+  // separately for now. crbug.com/929937 tracks this.
+  std::vector<MediaSink> all_sinks_;
 
   // Registered observers.
   base::ObserverList<Observer>::Unchecked observers_;

chrome/browser/ui/media_router/query_result_manager_unittest.cc

@@ -75,14 +75,14 @@ class QueryResultManagerTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(QueryResultManagerTest);
 };
 
-MATCHER_P(VectorEquals, expected, "") {
-  if (expected.size() != arg.size()) {
+// Requires that the elements of |expected| are unique.
+MATCHER_P(VectorSetEquals, expected, "") {
+  if (expected.size() != arg.size())
     return false;
-  }
+
   for (size_t i = 0; i < expected.size(); ++i) {
-    if (!(expected[i] == arg[i])) {
+    if (!base::ContainsValue(arg, expected[i]))
       return false;
-    }
   }
   return true;
 }
@@ -170,121 +170,88 @@ TEST_F(QueryResultManagerTest, MultipleQueries) {
   DiscoverSinks(MediaCastMode::PRESENTATION, presentation_source1);
   DiscoverSinks(MediaCastMode::TAB_MIRROR, tab_source);
 
-  // Scenario (results in this order):
-  // Action: (all sinks) -> [1, 2, 3, 4]
-  // Expected result:
-  // Sinks: [1 -> {}, 2 -> {}, 3 -> {}, 4 -> {}]
-  std::vector<MediaSinkWithCastModes> expected_sinks;
-  expected_sinks.push_back(MediaSinkWithCastModes(sink1));
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  expected_sinks.push_back(MediaSinkWithCastModes(sink4));
-
+  // Action: (all sinks) -> [1, 2, 3, 4].
   std::vector<MediaSink> sinks_query_result = {sink1, sink2, sink3, sink4};
-
+  std::vector<MediaSinkWithCastModes> expected_sinks = {
+      {sink1, {}}, {sink2, {}}, {sink3, {}}, {sink4, {}}};
   const auto& sinks_observers = query_result_manager_.sinks_observers_;
   auto* any_sink_observer = sinks_observers.find(MediaSource())->second.get();
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   any_sink_observer->OnSinksUpdated(sinks_query_result, {});
 
-  // Action: PRESENTATION -> [1, 2, 3]
-  // Expected result:
-  // Sinks: [1 -> {PRESENTATION}, 2 -> {PRESENTATION}, 3 -> {PRESENTATION},
-  //         4 -> {}]
-  expected_sinks.clear();
-  expected_sinks.push_back(MediaSinkWithCastModes(sink1));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink4));
-
+  // Action: PRESENTATION -> [1, 2, 3].
   sinks_query_result = {sink1, sink2, sink3};
+  expected_sinks = {{sink1, {MediaCastMode::PRESENTATION}},
+                    {sink2, {MediaCastMode::PRESENTATION}},
+                    {sink3, {MediaCastMode::PRESENTATION}},
+                    {sink4, {}}};
   auto* presentation1_sinks_observer =
       sinks_observers.find(presentation_source1)->second.get();
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   presentation1_sinks_observer->OnSinksUpdated(sinks_query_result, {});
 
-  // Action: TAB_MIRROR -> [2, 3, 4]
-  // Expected result:
-  // Sinks: [1 -> {PRESENTATION}, 2 -> {PRESENTATION, TAB_MIRROR},
-  //         3 -> {PRESENTATION, TAB_MIRROR}, 4 -> {TAB_MIRROR}]
-  expected_sinks.clear();
-  expected_sinks.push_back(MediaSinkWithCastModes(sink1));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::PRESENTATION);
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink4));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-
+  // Action: TAB_MIRROR -> [2, 3, 4].
   sinks_query_result = {sink2, sink3, sink4};
+  expected_sinks = {
+      {sink1, {MediaCastMode::PRESENTATION}},
+      {sink2, {MediaCastMode::PRESENTATION, MediaCastMode::TAB_MIRROR}},
+      {sink3, {MediaCastMode::PRESENTATION, MediaCastMode::TAB_MIRROR}},
+      {sink4, {MediaCastMode::TAB_MIRROR}}};
   auto* tab_sinks_observer = sinks_observers.find(tab_source)->second.get();
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   tab_sinks_observer->OnSinksUpdated(sinks_query_result,
                                      {url::Origin::Create(GURL(kOrigin))});
 
-  // Action: Update presentation URL
-  // Expected result:
-  // Sinks: [1 -> {}, 2 -> {TAB_MIRROR}, 3 -> {TAB_MIRROR}, 4 -> {TAB_MIRROR}]
-  expected_sinks.clear();
-  expected_sinks.push_back(MediaSinkWithCastModes(sink1));
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink4));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-
+  // Action: Update presentation URL.
+  expected_sinks = {{sink1, {}},
+                    {sink2, {MediaCastMode::TAB_MIRROR}},
+                    {sink3, {MediaCastMode::TAB_MIRROR}},
+                    {sink4, {MediaCastMode::TAB_MIRROR}}};
   // The observer for the old source will be unregistered.
   EXPECT_CALL(mock_router_, UnregisterMediaSinksObserver(_));
   // The observer for the new source will be registered.
   EXPECT_CALL(mock_router_, RegisterMediaSinksObserver(_))
       .WillOnce(Return(true));
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   query_result_manager_.SetSourcesForCastMode(
       MediaCastMode::PRESENTATION, {presentation_source2},
       url::Origin::Create(GURL(kOrigin)));
 
-  // Action: PRESENTATION -> [1], origins don't match
-  // Expected result: [1 -> {}, 2 -> {TAB_MIRROR}, 3 -> {TAB_MIRROR},
-  //                   4 -> {TAB_MIRROR}] (No change)
+  // Action: PRESENTATION -> [1], origins don't match.
+  // Expect no change to the sinks.
   sinks_query_result = {sink1};
   auto* presentation2_sinks_observer =
       sinks_observers.find(presentation_source2)->second.get();
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   presentation2_sinks_observer->OnSinksUpdated(
       sinks_query_result,
       {url::Origin::Create(GURL("https://differentOrigin.com"))});
 
-  // Action: (all sinks) -> [2, 3]
-  // Expected result: [2 -> {TAB_MIRROR}, 3 -> {TAB_MIRROR}]
-  expected_sinks.clear();
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  expected_sinks.back().cast_modes.insert(MediaCastMode::TAB_MIRROR);
-
+  // Action: (all sinks) -> [2, 3].
+  // |sink1| gets removed because none of the sink observers see it.
   sinks_query_result = {sink2, sink3};
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  expected_sinks = {{sink2, {MediaCastMode::TAB_MIRROR}},
+                    {sink3, {MediaCastMode::TAB_MIRROR}},
+                    {sink4, {MediaCastMode::TAB_MIRROR}}};
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   any_sink_observer->OnSinksUpdated(sinks_query_result, {});
 
-  // Action: Remove TAB_MIRROR observer
-  // Expected result:
-  // Sinks: [2 -> {}, 3 -> {}]
-  expected_sinks.clear();
-  expected_sinks.push_back(MediaSinkWithCastModes(sink2));
-  expected_sinks.push_back(MediaSinkWithCastModes(sink3));
-  EXPECT_CALL(mock_observer_, OnResultsUpdated(VectorEquals(expected_sinks)));
+  // Action: Remove TAB_MIRROR observer.
+  // |sink4| gets removed because none of the sink observers see it.
+  expected_sinks = {{sink2, {}}, {sink3, {}}};
+  EXPECT_CALL(mock_observer_,
+              OnResultsUpdated(VectorSetEquals(expected_sinks)));
   EXPECT_CALL(mock_router_, UnregisterMediaSinksObserver(_));
   query_result_manager_.RemoveSourcesForCastMode(MediaCastMode::TAB_MIRROR);
 
   // Remaining observers: PRESENTATION observer, which will be removed on
-  // destruction
+  // destruction.
   EXPECT_CALL(mock_router_, UnregisterMediaSinksObserver(_));
 }