[Cast MRP] Use app availability caching when adding sink query.

The availability caching will now be checked when a new sink query is
added. Before that, it is only checked during Refresh() (user gesture)
and when sink is added. This prevents a request from being re-sent if
a query was removed and re-added while the sink is still on the list.
In addition, the cache rule now applies when a sink is added or
updated. This means a request will be re-sent (subject to timing
threshold) if its cached availability is kUnavailable.

Note the rule to always use cache if the availability value is
kAvailable hasn't changed. An app is considered to be available until
it is offline, at which point the cache value will be invalidated.

Bug: 809249
Change-Id: Iadcee4953b51464211c3ee91777683a640f1291a
Reviewed-on: https://chromium-review.googlesource.com/1173477
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588947}

chrome/browser/media/router/providers/cast/cast_app_discovery_service.cc

@@ -19,22 +19,6 @@ namespace {
 static constexpr base::TimeDelta kRefreshThreshold =
     base::TimeDelta::FromMinutes(1);
 
-bool ShouldRefreshAppAvailability(
-    const CastAppAvailabilityTracker::AppAvailability& availability,
-    base::TimeTicks now) {
-  switch (availability.first) {
-    case cast_channel::GetAppAvailabilityResult::kAvailable:
-      return false;
-    case cast_channel::GetAppAvailabilityResult::kUnavailable:
-      return now - availability.second > kRefreshThreshold;
-    case cast_channel::GetAppAvailabilityResult::kUnknown:
-      return true;
-  }
-
-  NOTREACHED();
-  return false;
-}
-
 }  // namespace
 
 CastAppDiscoveryServiceImpl::CastAppDiscoveryServiceImpl(
@@ -82,9 +66,8 @@ CastAppDiscoveryServiceImpl::StartObservingMediaSinks(
         base::Unretained(this), source));
 
     // Note: even though we retain availability results for an app unregistered
-    // from the tracker, we will send app availability requests again when it
-    // is re-registered. This gives us a chance to refresh the results in case
-    // it changed.
+    // from the tracker, we will refresh the results when the app is
+    // re-registered.
     base::flat_set<std::string> new_app_ids =
         availability_tracker_.RegisterSource(source);
     const auto& sinks = media_sink_service_->GetSinks();
@@ -104,21 +87,17 @@ CastAppDiscoveryServiceImpl::StartObservingMediaSinks(
       }
     }
   }
-
   return callback_list->Add(callback);
 }
 
 void CastAppDiscoveryServiceImpl::Refresh() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   const auto app_ids = availability_tracker_.GetRegisteredApps();
-  base::TimeTicks now = clock_->NowTicks();
   const auto& sinks = media_sink_service_->GetSinks();
   // Note: The following logic assumes |sinks| will not change as it is
   // being iterated.
   for (const auto& sink : sinks) {
     for (const auto& app_id : app_ids) {
-      if (ShouldRefreshAppAvailability(
-              availability_tracker_.GetAvailability(sink.first, app_id), now)) {
         int channel_id = sink.second.cast_data().cast_channel_id;
         cast_channel::CastSocket* socket =
             socket_service_->GetSocket(channel_id);
@@ -126,11 +105,9 @@ void CastAppDiscoveryServiceImpl::Refresh() {
           DVLOG(1) << "Socket not found for id " << channel_id;
           continue;
         }
-
         RequestAppAvailability(socket, app_id, sink.first);
       }
   }
-  }
 }
 
 void CastAppDiscoveryServiceImpl::MaybeRemoveSinkQueryEntry(
@@ -164,13 +141,8 @@ void CastAppDiscoveryServiceImpl::OnSinkAddedOrUpdated(
   // Any queries that currently contains this sink should be updated.
   UpdateSinkQueries(availability_tracker_.GetSupportedSources(sink_id));
 
-  for (const std::string& app_id : availability_tracker_.GetRegisteredApps()) {
-    auto availability = availability_tracker_.GetAvailability(sink_id, app_id);
-    if (availability.first != cast_channel::GetAppAvailabilityResult::kUnknown)
-      continue;
-
+  for (const std::string& app_id : availability_tracker_.GetRegisteredApps())
     RequestAppAvailability(socket, app_id, sink_id);
-  }
 }
 
 void CastAppDiscoveryServiceImpl::OnSinkRemoved(const MediaSinkInternal& sink) {
@@ -183,11 +155,13 @@ void CastAppDiscoveryServiceImpl::RequestAppAvailability(
     cast_channel::CastSocket* socket,
     const std::string& app_id,
     const MediaSink::Id& sink_id) {
+  base::TimeTicks now = clock_->NowTicks();
+  if (ShouldRefreshAppAvailability(sink_id, app_id, now)) {
     message_handler_->RequestAppAvailability(
         socket, app_id,
         base::BindOnce(&CastAppDiscoveryServiceImpl::UpdateAppAvailability,
-                     weak_ptr_factory_.GetWeakPtr(), clock_->NowTicks(),
-                     sink_id));
+                       weak_ptr_factory_.GetWeakPtr(), now, sink_id));
+  }
 }
 
 void CastAppDiscoveryServiceImpl::UpdateAppAvailability(
@@ -231,4 +205,22 @@ std::vector<MediaSinkInternal> CastAppDiscoveryServiceImpl::GetSinksByIds(
   return sinks;
 }
 
+bool CastAppDiscoveryServiceImpl::ShouldRefreshAppAvailability(
+    const MediaSink::Id& sink_id,
+    const std::string& app_id,
+    base::TimeTicks now) const {
+  auto availability = availability_tracker_.GetAvailability(sink_id, app_id);
+  switch (availability.first) {
+    case cast_channel::GetAppAvailabilityResult::kAvailable:
+      return false;
+    case cast_channel::GetAppAvailabilityResult::kUnavailable:
+      return now - availability.second > kRefreshThreshold;
+    case cast_channel::GetAppAvailabilityResult::kUnknown:
+      return true;
+  }
+
+  NOTREACHED();
+  return false;
+}
+
 }  // namespace media_router

chrome/browser/media/router/providers/cast/cast_app_discovery_service.h

@@ -118,6 +118,13 @@ class CastAppDiscoveryServiceImpl : public CastAppDiscoveryService,
   std::vector<MediaSinkInternal> GetSinksByIds(
       const base::flat_set<MediaSink::Id>& sink_ids) const;
 
+  // Returns true if an app availability request should be issued for |sink_id|
+  // and |app_id|. |now| is used for checking whether previously cached results
+  // should be refreshed.
+  bool ShouldRefreshAppAvailability(const MediaSink::Id& sink_id,
+                                    const std::string& app_id,
+                                    base::TimeTicks now) const;
+
   // Registered sink queries and their associated callbacks.
   base::flat_map<MediaSource::Id, std::unique_ptr<SinkQueryCallbackList>>
       sink_queries_;

chrome/browser/media/router/providers/cast/cast_app_discovery_service_unittest.cc

@@ -116,6 +116,33 @@ TEST_F(CastAppDiscoveryServiceTest, StartObservingMediaSinks) {
   RemoveSink(sink1);
 }
 
+TEST_F(CastAppDiscoveryServiceTest, ReAddSinkQueryUsesCachedValue) {
+  auto subscription1 = StartObservingMediaSinksInitially(source_a_1_);
+
+  // Adding a sink after app registered causes app availability request to be
+  // sent.
+  MediaSinkInternal sink1 = CreateCastSink(1);
+  cast_channel::GetAppAvailabilityCallback cb;
+  EXPECT_CALL(message_handler_, DoRequestAppAvailability(_, "AAAAAAAA", _))
+      .WillOnce([&cb](cast_channel::CastSocket*, const std::string&,
+                      cast_channel::GetAppAvailabilityCallback& callback) {
+        cb = std::move(callback);
+      });
+
+  AddOrUpdateSink(sink1);
+
+  std::vector<MediaSinkInternal> sinks_1 = {sink1};
+  EXPECT_CALL(*this, OnSinkQueryUpdated(source_a_1_.source_id(), sinks_1));
+  std::move(cb).Run("AAAAAAAA", GetAppAvailabilityResult::kAvailable);
+
+  subscription1.reset();
+
+  // Request not re-sent; cached kAvailable value is used.
+  EXPECT_CALL(message_handler_, DoRequestAppAvailability(_, _, _)).Times(0);
+  EXPECT_CALL(*this, OnSinkQueryUpdated(source_a_1_.source_id(), sinks_1));
+  subscription1 = StartObservingMediaSinksInitially(source_a_1_);
+}
+
 TEST_F(CastAppDiscoveryServiceTest, SinkQueryUpdatedOnSinkUpdate) {
   auto subscription1 = StartObservingMediaSinksInitially(source_a_1_);