[Cast MRP] Refactored to streamline unit tests.

This patch as a number of changes to streamline unit tests; the main
on is that it names ActivityRecord::SetOrUpdateSession() a nonvirtual
method and delegates subclass-specific functionality to the new
OnSessionSet() and OnSessionUpdated() methods.

Change-Id: I3ce0c0ced968d3e3945ecd215dfe9a25959a49a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2278138
Commit-Queue: John Williams <jrw@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786504}

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

@@ -73,10 +73,10 @@ void ActivityRecord::SetOrUpdateSession(const CastSession& session,
   sink_ = sink;
   if (session_id_) {
     DCHECK_EQ(*session_id_, session.session_id());
+    OnSessionUpdated(session, hash_token);
   } else {
     session_id_ = session.session_id();
-    if (on_session_set_)
-      std::move(on_session_set_).Run();
+    OnSessionSet(session);
   }
 }
 
@@ -175,6 +175,9 @@ void ActivityRecord::HandleLeaveSession(const std::string& client_id) {
   }
 }
 
+void ActivityRecord::OnSessionUpdated(const CastSession& session,
+                                      const std::string& hash_token) {}
+
 CastSessionClientFactoryForTest* ActivityRecord::client_factory_for_test_ =
     nullptr;
 

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

@@ -74,9 +74,9 @@ class ActivityRecord {
   //
   // The |hash_token| parameter is used for hashing receiver IDs in messages
   // sent to the Cast SDK, and |sink| is the sink associated with |session|.
-  virtual void SetOrUpdateSession(const CastSession& session,
-                                  const MediaSinkInternal& sink,
-                                  const std::string& hash_token);
+  void SetOrUpdateSession(const CastSession& session,
+                          const MediaSinkInternal& sink,
+                          const std::string& hash_token);
 
   virtual void SendStopSessionMessageToClients(const std::string& hash_token);
 
@@ -144,20 +144,25 @@ 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;
+  void SetSessionIdForTest(const std::string& session_id) {
+    session_id_ = session_id;
   }
 
  protected:
   using ClientMap =
       base::flat_map<std::string, std::unique_ptr<CastSessionClient>>;
 
-  // Gets the session associated with this activity.  May return nullptr.
+  // Gets the session based on its ID.  May return null.
   CastSession* GetSession() const;
 
+  // Called after the session has been set by SetOrUpdateSession.  The |session|
+  // parameters are somewhat redundant because the same information is available
+  // using the GetSession() method, but passing the parameter avoids some
+  // unnecessary lookups and eliminates the need to a null check.
+  virtual void OnSessionSet(const CastSession& session) = 0;
+  virtual void OnSessionUpdated(const CastSession& session,
+                                const std::string& hash_token);
+
   CastSessionClient* GetClient(const std::string& client_id) {
     auto it = connected_clients_.find(client_id);
     return it == connected_clients_.end() ? nullptr : it->second.get();
@@ -169,9 +174,6 @@ class ActivityRecord {
   std::string app_id_;
   base::Optional<int> mirroring_tab_id_;
 
-  // Called when a session is initially set from SetOrUpdateSession().
-  base::OnceCallback<void()> on_session_set_;
-
   // TODO(https://crbug.com/809249): Consider wrapping CastMessageHandler with
   // known parameters (sink, client ID, session transport ID) and passing them
   // to objects that need to send messages to the receiver.

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

@@ -161,8 +161,8 @@ void CastActivityManager::LaunchSessionParsed(
         activity_it->second->route().media_route_id();
     // We cannot launch the new session in the TerminateSession() callback
     // because if we create a session there, then it may get deleted when
-    // OnSessionRemoved() is called to notify that the previous session was
-    // removed on the receiver.
+    // OnSessionRemoved() is called to notify that the previous session
+    // was removed on the receiver.
     TerminateSession(existing_route_id, base::DoNothing());
     // The new session will be launched when OnSessionRemoved() is called for
     // the old session.
@@ -487,8 +487,7 @@ ActivityRecord* CastActivityManager::AddMirroringActivityRecord(
           : std::make_unique<MirroringActivityRecord>(
                 route, app_id, message_handler_, session_tracker_, tab_id,
                 cast_data, std::move(on_stop));
-  if (route.is_local())
-    activity->CreateMojoBindings(media_router_);
+  activity->CreateMojoBindings(media_router_);
   auto* const activity_ptr = activity.get();
   activities_.emplace(route.media_route_id(), std::move(activity));
   return activity_ptr;

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

@@ -29,15 +29,16 @@ CastActivityRecord::CastActivityRecord(
 
 CastActivityRecord::~CastActivityRecord() = default;
 
-void CastActivityRecord::SetOrUpdateSession(const CastSession& session,
-                                            const MediaSinkInternal& sink,
-                                            const std::string& hash_token) {
-  bool had_session_id = session_id_.has_value();
-  ActivityRecord::SetOrUpdateSession(session, sink, hash_token);
-  if (had_session_id) {
-    for (auto& client : connected_clients_)
-      client.second->SendMessageToClient(
-          CreateUpdateSessionMessage(session, client.first, sink, hash_token));
+void CastActivityRecord::OnSessionSet(const CastSession& session) {
+  if (media_controller_)
+    media_controller_->SetSession(session);
+}
+
+void CastActivityRecord::OnSessionUpdated(const CastSession& session,
+                                          const std::string& hash_token) {
+  for (auto& client : connected_clients_) {
+    client.second->SendMessageToClient(
+        CreateUpdateSessionMessage(session, client.first, sink_, hash_token));
   }
   if (media_controller_)
     media_controller_->SetSession(session);

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

@@ -40,10 +40,6 @@ class CastActivityRecord : public ActivityRecord {
                      CastSessionTracker* session_tracker);
   ~CastActivityRecord() override;
 
-  // ActivityRecord implementation
-  void SetOrUpdateSession(const CastSession& session,
-                          const MediaSinkInternal& sink,
-                          const std::string& hash_token) override;
   void SendMediaStatusToClients(const base::Value& media_status,
                                 base::Optional<int> request_id) override;
   void OnAppMessage(const cast::channel::CastMessage& message) override;
@@ -64,6 +60,9 @@ class CastActivityRecord : public ActivityRecord {
   bool HasJoinableClient(AutoJoinPolicy policy,
                          const url::Origin& origin,
                          int tab_id) const;
+  void OnSessionSet(const CastSession& session) override;
+  void OnSessionUpdated(const CastSession& session,
+                        const std::string& hash_token) override;
 
  private:
   friend class CastSessionClientImpl;

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

@@ -71,24 +71,21 @@ class MockPresentationConnection : public blink::mojom::PresentationConnection {
 
 }  // namespace
 
+#define EXPECT_ERROR_LOG(matcher)                                \
+  if (DLOG_IS_ON(ERROR)) {                                       \
+    EXPECT_CALL(log_, Log(logging::LOG_ERROR, _, _, _, matcher)) \
+        .WillOnce(Return(true)); /* suppress logging */          \
+  }
+
 class CastSessionClientImplTest : public testing::Test {
  public:
-  CastSessionClientImplTest() { activity_.set_session_id("theSessionId"); }
+  CastSessionClientImplTest() { activity_.SetSessionIdForTest("theSessionId"); }
 
   ~CastSessionClientImplTest() override { RunUntilIdle(); }
 
  protected:
   void RunUntilIdle() { task_environment_.RunUntilIdle(); }
 
-  template <typename T>
-  void ExpectErrorLog(const T& matcher) {
-    if (DLOG_IS_ON(ERROR)) {
-      EXPECT_CALL(log_, Log(logging::LOG_ERROR, _, _, _,
-                            matcher))
-          .WillOnce(Return(true));  // suppress logging
-    }
-  }
-
   content::BrowserTaskEnvironment task_environment_;
   data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
   cast_channel::MockCastSocketService socket_service_{
@@ -111,7 +108,7 @@ class CastSessionClientImplTest : public testing::Test {
 TEST_F(CastSessionClientImplTest, OnInvalidJson) {
   // TODO(crbug.com/905002): Check UMA calls instead of logging (here and
   // below).
-  ExpectErrorLog(HasSubstr("Failed to parse Cast client message"));
+  EXPECT_ERROR_LOG(HasSubstr("Failed to parse Cast client message"));
 
   log_.StartCapturingLogs();
   client_->OnMessage(
@@ -119,8 +116,8 @@ TEST_F(CastSessionClientImplTest, OnInvalidJson) {
 }
 
 TEST_F(CastSessionClientImplTest, OnInvalidMessage) {
-  ExpectErrorLog(AllOf(HasSubstr("Failed to parse Cast client message"),
-                       HasSubstr("Not a Cast message")));
+  EXPECT_ERROR_LOG(AllOf(HasSubstr("Failed to parse Cast client message"),
+                         HasSubstr("Not a Cast message")));
 
   log_.StartCapturingLogs();
   client_->OnMessage(
@@ -128,9 +125,9 @@ TEST_F(CastSessionClientImplTest, OnInvalidMessage) {
 }
 
 TEST_F(CastSessionClientImplTest, OnMessageWrongClientId) {
-  ExpectErrorLog(AllOf(HasSubstr("Client ID mismatch"),
-                       HasSubstr("theClientId"),
-                       HasSubstr("theWrongClientId")));
+  EXPECT_ERROR_LOG(AllOf(HasSubstr("Client ID mismatch"),
+                         HasSubstr("theClientId"),
+                         HasSubstr("theWrongClientId")));
 
   log_.StartCapturingLogs();
   client_->OnMessage(
@@ -145,9 +142,9 @@ TEST_F(CastSessionClientImplTest, OnMessageWrongClientId) {
 }
 
 TEST_F(CastSessionClientImplTest, OnMessageWrongSessionId) {
-  ExpectErrorLog(AllOf(HasSubstr("Session ID mismatch"),
-                       HasSubstr("theSessionId"),
-                       HasSubstr("theWrongSessionId")));
+  EXPECT_ERROR_LOG(AllOf(HasSubstr("Session ID mismatch"),
+                         HasSubstr("theSessionId"),
+                         HasSubstr("theWrongSessionId")));
 
   log_.StartCapturingLogs();
   client_->OnMessage(

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

@@ -142,7 +142,8 @@ MirroringActivityRecord::~MirroringActivityRecord() {
 
 void MirroringActivityRecord::CreateMojoBindings(
     mojom::MediaRouter* media_router) {
-  DCHECK(mirroring_type_);
+  if (!mirroring_type_)
+    return;
 
   // Get a reference to the mirroring service host.
   switch (*mirroring_type_) {
@@ -165,28 +166,9 @@ void MirroringActivityRecord::CreateMojoBindings(
       break;
   }
 
-  auto cast_source = CastMediaSource::FromMediaSource(route_.media_source());
-  DCHECK(cast_source);
-
-  // Derive session type from capabilities and media source.
-  const bool has_audio = (cast_data_.capabilities &
-                          static_cast<uint8_t>(cast_channel::AUDIO_OUT)) != 0 &&
-                         cast_source->allow_audio_capture();
-  const bool has_video = (cast_data_.capabilities &
-                          static_cast<uint8_t>(cast_channel::VIDEO_OUT)) != 0;
-  DCHECK(has_audio || has_video);
-  const SessionType session_type =
-      has_audio && has_video
-          ? SessionType::AUDIO_AND_VIDEO
-          : has_audio ? SessionType::AUDIO_ONLY : SessionType::VIDEO_ONLY;
-
-  // Arrange to start mirroring once the session is set.
-  on_session_set_ = base::BindOnce(
-      &MirroringActivityRecord::StartMirroring, base::Unretained(this),
-      SessionParameters::New(session_type, cast_data_.ip_endpoint.address(),
-                             cast_data_.model_name,
-                             cast_source->target_playout_delay()),
-      channel_to_service_.BindNewPipeAndPassReceiver());
+  DCHECK(!channel_to_service_receiver_);
+  channel_to_service_receiver_ =
+      channel_to_service_.BindNewPipeAndPassReceiver();
 }
 
 void MirroringActivityRecord::OnError(SessionError error) {
@@ -293,9 +275,25 @@ void MirroringActivityRecord::HandleParseJsonResult(
   message_handler_->SendCastMessage(cast_data_.cast_channel_id, cast_message);
 }
 
-void MirroringActivityRecord::StartMirroring(
-    mirroring::mojom::SessionParametersPtr session_params,
-    mojo::PendingReceiver<CastMessageChannel> channel_to_service) {
+void MirroringActivityRecord::OnSessionSet(const CastSession& session) {
+  if (!mirroring_type_)
+    return;
+
+  auto cast_source = CastMediaSource::FromMediaSource(route_.media_source());
+  DCHECK(cast_source);
+
+  // Derive session type from capabilities and media source.
+  const bool has_audio = (cast_data_.capabilities &
+                          static_cast<uint8_t>(cast_channel::AUDIO_OUT)) != 0 &&
+                         cast_source->allow_audio_capture();
+  const bool has_video = (cast_data_.capabilities &
+                          static_cast<uint8_t>(cast_channel::VIDEO_OUT)) != 0;
+  DCHECK(has_audio || has_video);
+  const SessionType session_type =
+      has_audio && has_video
+          ? SessionType::AUDIO_AND_VIDEO
+          : has_audio ? SessionType::AUDIO_ONLY : SessionType::VIDEO_ONLY;
+
   will_start_mirroring_timestamp_ = base::Time::Now();
 
   // Bind Mojo receivers for the interfaces this object implements.
@@ -304,8 +302,15 @@ void MirroringActivityRecord::StartMirroring(
   mojo::PendingRemote<mirroring::mojom::CastMessageChannel> channel_remote;
   channel_receiver_.Bind(channel_remote.InitWithNewPipeAndPassReceiver());
 
-  host_->Start(std::move(session_params), std::move(observer_remote),
-               std::move(channel_remote), std::move(channel_to_service));
+  // If this fails, it's probably because CreateMojoBindings() hasn't been
+  // called.
+  DCHECK(channel_to_service_receiver_);
+
+  host_->Start(SessionParameters::New(
+                   session_type, cast_data_.ip_endpoint.address(),
+                   cast_data_.model_name, cast_source->target_playout_delay()),
+               std::move(observer_remote), std::move(channel_remote),
+               std::move(channel_to_service_receiver_));
 }
 
 void MirroringActivityRecord::StopMirroring() {

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

@@ -66,6 +66,7 @@ class MirroringActivityRecord : public ActivityRecord,
   void OnInternalMessage(const cast_channel::InternalMessage& message) override;
 
  protected:
+  void OnSessionSet(const CastSession& session) override;
   void CreateMediaController(
       mojo::PendingReceiver<mojom::MediaController> media_controller,
       mojo::PendingRemote<mojom::MediaStatusObserver> observer) override;
@@ -74,9 +75,6 @@ class MirroringActivityRecord : public ActivityRecord,
   void HandleParseJsonResult(const std::string& route_id,
                              data_decoder::DataDecoder::ValueOrError result);
 
-  void StartMirroring(
-      mirroring::mojom::SessionParametersPtr session_params,
-      mojo::PendingReceiver<CastMessageChannel> channel_to_service);
   void StopMirroring();
 
   mojo::Remote<mirroring::mojom::MirroringServiceHost> host_;
@@ -84,6 +82,11 @@ class MirroringActivityRecord : public ActivityRecord,
   // Sends Cast messages from the mirroring receiver to the mirroring service.
   mojo::Remote<mirroring::mojom::CastMessageChannel> channel_to_service_;
 
+  // Only used to store pending CastMessageChannel receiver while waiting for
+  // OnSessionSet() to be called.
+  mojo::PendingReceiver<mirroring::mojom::CastMessageChannel>
+      channel_to_service_receiver_;
+
   mojo::Receiver<mirroring::mojom::SessionObserver> observer_receiver_{this};
 
   // To handle Cast messages from the mirroring service to the mirroring

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

@@ -21,12 +21,6 @@ class MockCastActivityRecord : public CastActivityRecord {
   MockCastActivityRecord(const MediaRoute& route, const std::string& app_id);
   ~MockCastActivityRecord() override;
 
-  void set_session_id(const std::string& new_id) {
-    if (!session_id_)
-      session_id_ = new_id;
-    ASSERT_EQ(session_id_, new_id);
-  }
-
   MOCK_METHOD1(SendAppMessageToReceiver,
                cast_channel::Result(const CastInternalMessage& cast_message));
   MOCK_METHOD1(SendMediaRequestToReceiver,
@@ -47,10 +41,9 @@ class MockCastActivityRecord : public CastActivityRecord {
                                             const url::Origin& origin,
                                             int tab_id));
   MOCK_METHOD1(RemoveClient, void(const std::string& client_id));
-  MOCK_METHOD3(SetOrUpdateSession,
-               void(const CastSession& session,
-                    const MediaSinkInternal& sink,
-                    const std::string& hash_token));
+  MOCK_METHOD1(OnSessionSet, void(const CastSession& session));
+  MOCK_METHOD2(OnSessionUpdated,
+               void(const CastSession& session, const std::string& hash_token));
   MOCK_METHOD2(SendMessageToClient,
                void(const std::string& client_id,
                     blink::mojom::PresentationConnectionMessagePtr message));