[Cast MRP] Closing a tab or window terminates mirroring.

Bug: 1095222, b/159069899
Change-Id: I98eefb2eaa7b7086a26590a09bfc811b854e8e33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264497
Commit-Queue: John Williams <jrw@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783092}

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

@@ -68,6 +68,7 @@ void ActivityRecord::SetOrUpdateSession(const CastSession& session,
   DVLOG(2) << "SetOrUpdateSession old session_id = "
            << session_id_.value_or("<missing>")
            << ", new session_id = " << session.session_id();
+  DCHECK(sink.is_cast_sink());
   route_.set_description(session.GetRouteDescription());
   sink_ = sink;
   if (session_id_) {

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

@@ -37,7 +37,8 @@ class ActivityRecordFactoryForTest {
       const std::string& app_id) = 0;
   virtual std::unique_ptr<MirroringActivityRecord> MakeMirroringActivityRecord(
       const MediaRoute& route,
-      const std::string& app_id) = 0;
+      const std::string& app_id,
+      base::OnceClosure on_stop) = 0;
 };
 
 class ActivityRecord {
@@ -143,6 +144,13 @@ class ActivityRecord {
     client_factory_for_test_ = factory;
   }
 
+  void SetSessionAndSinkForTest(const CastSession& session,
+                                const MediaSinkInternal& sink,
+                                const std::string& hash_code) {
+    session_id_ = session.session_id();
+    sink_ = sink;
+  }
+
  protected:
   using ClientMap =
       base::flat_map<std::string, std::unique_ptr<CastSessionClient>>;
@@ -178,8 +186,6 @@ class ActivityRecord {
   ClientMap connected_clients_;
 
  private:
-  friend class CastActivityRecordTest;
-
   static CastSessionClientFactoryForTest* client_factory_for_test_;
 };
 

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

@@ -338,13 +338,8 @@ void CastActivityManager::JoinSession(
                           base::nullopt, RouteRequestResult::ResultCode::OK);
 }
 
-// TODO(jrw): Can this be merged with HandleStopSessionResponse?
-void CastActivityManager::RemoveActivityByRouteId(const std::string& route_id) {
-  auto it = activities_.find(route_id);
-  if (it != activities_.end()) {
-    RemoveActivity(it, PresentationConnectionState::TERMINATED,
-                   PresentationConnectionCloseReason::CLOSED);
-  }
+void CastActivityManager::OnActivityStopped(const std::string& route_id) {
+  TerminateSession(route_id, base::DoNothing());
 }
 
 void CastActivityManager::RemoveActivity(
@@ -449,8 +444,9 @@ CastActivityRecord* CastActivityManager::AddCastActivityRecord(
     const MediaRoute& route,
     const std::string& app_id) {
   std::unique_ptr<CastActivityRecord> activity(
-      activity_record_factory_
-          ? activity_record_factory_->MakeCastActivityRecord(route, app_id)
+      activity_record_factory_for_test_
+          ? activity_record_factory_for_test_->MakeCastActivityRecord(route,
+                                                                      app_id)
           : std::make_unique<CastActivityRecord>(
                 route, app_id, message_handler_, session_tracker_));
   auto* const activity_ptr = activity.get();
@@ -464,18 +460,18 @@ ActivityRecord* CastActivityManager::AddMirroringActivityRecord(
     const std::string& app_id,
     const int tab_id,
     const CastSinkExtraData& cast_data) {
+  // NOTE(jrw): We could theoretically use base::Unretained() below instead of
+  // GetWeakPtr(), but that seems like an unnecessary optimization here.
+  auto on_stop =
+      base::BindOnce(&CastActivityManager::OnActivityStopped,
+                     weak_ptr_factory_.GetWeakPtr(), route.media_route_id());
   auto activity =
-      activity_record_factory_
-          ? activity_record_factory_->MakeMirroringActivityRecord(route, app_id)
+      activity_record_factory_for_test_
+          ? activity_record_factory_for_test_->MakeMirroringActivityRecord(
+                route, app_id, std::move(on_stop))
           : std::make_unique<MirroringActivityRecord>(
                 route, app_id, message_handler_, session_tracker_, tab_id,
-                cast_data,
-                // NOTE(jrw): We could theoretically use base::Unretained()
-                // below instead of GetWeakPtr(), but that seems like an
-                // unnecessary optimization here.
-                base::BindOnce(&CastActivityManager::RemoveActivityByRouteId,
-                               weak_ptr_factory_.GetWeakPtr(),
-                               route.media_route_id()));
+                cast_data, std::move(on_stop));
   if (route.is_local())
     activity->CreateMojoBindings(media_router_);
   auto* const activity_ptr = activity.get();
@@ -834,7 +830,7 @@ CastActivityManager::DoLaunchSessionParams::DoLaunchSessionParams(
 CastActivityManager::DoLaunchSessionParams::~DoLaunchSessionParams() = default;
 
 // static
-ActivityRecordFactoryForTest* CastActivityManager::activity_record_factory_ =
-    nullptr;
+ActivityRecordFactoryForTest*
+    CastActivityManager::activity_record_factory_for_test_ = nullptr;
 
 }  // namespace media_router

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

@@ -128,7 +128,7 @@ class CastActivityManager : public CastActivityManagerBase,
 
   static void SetActitivyRecordFactoryForTest(
       ActivityRecordFactoryForTest* factory) {
-    activity_record_factory_ = factory;
+    activity_record_factory_for_test_ = factory;
   }
 
   cast_channel::ResultCallback MakeResultCallbackForRoute(
@@ -204,8 +204,7 @@ class CastActivityManager : public CastActivityManagerBase,
   };
 
   void DoLaunchSession(DoLaunchSessionParams params);
-
-  void RemoveActivityByRouteId(const std::string& route_id);
+  void OnActivityStopped(const std::string& route_id);
 
   // Removes an activity, terminating any associated connections, then
   // notifies the media router that routes have been updated.
@@ -275,7 +274,7 @@ class CastActivityManager : public CastActivityManagerBase,
   std::string ChooseAppId(const CastMediaSource& source,
                           const MediaSinkInternal& sink) const;
 
-  static ActivityRecordFactoryForTest* activity_record_factory_;
+  static ActivityRecordFactoryForTest* activity_record_factory_for_test_;
 
   base::flat_set<MediaSource::Id> route_queries_;
 

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

@@ -45,6 +45,7 @@ using testing::_;
 using testing::AnyNumber;
 using testing::ByRef;
 using testing::ElementsAre;
+using testing::Invoke;
 using testing::IsEmpty;
 using testing::Not;
 using testing::Return;
@@ -102,14 +103,15 @@ using MockCastActivityRecordCallback =
 class MockMirroringActivityRecord : public MirroringActivityRecord {
  public:
   MockMirroringActivityRecord(const MediaRoute& route,
-                              const std::string& app_id)
+                              const std::string& app_id,
+                              OnStopCallback on_stop)
       : MirroringActivityRecord(route,
                                 app_id,
                                 nullptr,
                                 nullptr,
                                 0,
                                 CastSinkExtraData(),
-                                base::DoNothing()) {}
+                                std::move(on_stop)) {}
 
   void set_session_id(const std::string& new_id) {
     if (!session_id_)
@@ -179,12 +181,10 @@ class CastActivityManagerTest : public testing::Test,
       const MediaRoute& route,
       const std::string& app_id) override {
     auto activity = std::make_unique<MockCastActivityRecord>(route, app_id);
-    auto* activity_ptr = activity.get();
-    std::string route_id = route.media_route_id();
     ON_CALL(*activity, SetOrUpdateSession)
-        .WillByDefault(WithArg<0>([activity_ptr](const auto& session) {
-          activity_ptr->set_session_id(session.session_id());
-        }));
+        .WillByDefault(
+            Invoke(activity.get(), &ActivityRecord::SetSessionAndSinkForTest));
+    auto* activity_ptr = activity.get();
     cast_activities_.push_back(activity_ptr);
     activity_record_callback_.Run(activity_ptr);
     return activity;
@@ -193,14 +193,13 @@ class CastActivityManagerTest : public testing::Test,
   // from ActivityRecordFactoryForTest
   std::unique_ptr<MirroringActivityRecord> MakeMirroringActivityRecord(
       const MediaRoute& route,
-      const std::string& app_id) override {
-    auto activity =
-        std::make_unique<MockMirroringActivityRecord>(route, app_id);
-    auto* activity_ptr = activity.get();
+      const std::string& app_id,
+      MirroringActivityRecord::OnStopCallback on_stop) override {
+    auto activity = std::make_unique<MockMirroringActivityRecord>(
+        route, app_id, std::move(on_stop));
     ON_CALL(*activity, SetOrUpdateSession)
-        .WillByDefault(WithArg<0>([activity_ptr](const auto& session) {
-          activity_ptr->set_session_id(session.session_id());
-        }));
+        .WillByDefault(
+            Invoke(activity.get(), &ActivityRecord::SetSessionAndSinkForTest));
     mirroring_activity_ = activity.get();
     return activity;
   }
@@ -423,6 +422,20 @@ TEST_F(CastActivityManagerTest, LaunchMirroringSession) {
   EXPECT_EQ(RouteControllerType::kMirroring, route_->controller_type());
 }
 
+TEST_F(CastActivityManagerTest, MirroringSessionStopped) {
+  CallLaunchSession(kCastStreamingAppId);
+  auto response = GetSuccessLaunchResponse();
+
+  SetSessionForTest(route_->media_sink_id(),
+                    CastSession::From(sink_, *response.receiver_status));
+  std::move(launch_session_callback_).Run(std::move(response));
+  RunUntilIdle();
+
+  ASSERT_TRUE(mirroring_activity_);
+  EXPECT_CALL(message_handler_, StopSession).Times(1);
+  mirroring_activity_->DidStop();
+}
+
 TEST_F(CastActivityManagerTest, LaunchSessionFails) {
   // 3 things will happen:
   // (1) Route is removed