CrOS GMC: Fix a bug where controls can be empty in quick settings.

This CL changes the logic for displaying controls in quick settings
such that we will only show controls if the title of the session is
not empty and the session is controllable. This also matches the logic
for showing notification in Chrome global media controls.

Bug: 1142625
Change-Id: I91d7f8279b8602d89af50bdf7c891b3440abd9ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523735
Commit-Queue: Jazz Xu <jazzhsu@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825455}

ash/system/media/unified_media_controls_controller.cc

@@ -64,16 +64,20 @@ views::View* UnifiedMediaControlsController::CreateView() {
 
 void UnifiedMediaControlsController::MediaSessionInfoChanged(
     media_session::mojom::MediaSessionInfoPtr session_info) {
-  if (!session_info)
-    return;
-
   if (freeze_session_timer_->IsRunning()) {
-    pending_playback_state_ = session_info->playback_state;
+    if (session_info)
+      pending_session_info_ = std::move(session_info);
     return;
   }
 
+  session_info_ = std::move(session_info);
+  MaybeShowMediaControlsOrEmptyState();
+
+  if (!session_info_)
+    return;
+
   media_controls_->SetIsPlaying(
-      session_info->playback_state ==
+      session_info_->playback_state ==
       media_session::mojom::MediaPlaybackState::kPlaying);
   media_controls_->UpdateActionButtonAvailability(enabled_actions_);
 }
@@ -84,9 +88,11 @@ void UnifiedMediaControlsController::MediaSessionMetadataChanged(
   if (freeze_session_timer_->IsRunning())
     return;
 
+  session_metadata_ = *pending_metadata_;
   media_controls_->SetTitle(pending_metadata_->title);
   media_controls_->SetArtist(pending_metadata_->artist);
   pending_metadata_.reset();
+  MaybeShowMediaControlsOrEmptyState();
 }
 
 void UnifiedMediaControlsController::MediaSessionActionsChanged(
@@ -116,12 +122,10 @@ void UnifiedMediaControlsController::MediaSessionChanged(
     return;
   }
 
-  // If we don't currently have a session, show media controls.
+  // If we don't currently have a session, update session id.
   if (!media_session_id_.has_value()) {
     DCHECK(!freeze_session_timer_->IsRunning());
     media_session_id_ = request_id;
-    delegate_->ShowMediaControls();
-    media_controls_->OnNewMediaSession();
     return;
   }
 
@@ -157,22 +161,24 @@ void UnifiedMediaControlsController::MediaControllerImageChanged(
 void UnifiedMediaControlsController::UpdateSession() {
   media_session_id_ = pending_session_id_;
 
-  if (media_session_id_ == base::nullopt) {
-    media_controls_->ShowEmptyState();
+  if (media_session_id_ == base::nullopt)
     ResetPendingData();
-    return;
-  }
 
-  if (pending_playback_state_.has_value()) {
+  if (pending_session_info_.has_value()) {
     media_controls_->SetIsPlaying(
-        pending_playback_state_ ==
+        (*pending_session_info_)->playback_state ==
         media_session::mojom::MediaPlaybackState::kPlaying);
+    session_info_ = std::move(*pending_session_info_);
+  } else {
+    session_info_ = nullptr;
   }
 
   if (pending_metadata_.has_value()) {
     media_controls_->SetTitle(pending_metadata_->title);
     media_controls_->SetArtist(pending_metadata_->artist);
   }
+  session_metadata_ =
+      pending_metadata_.value_or(media_session::MediaMetadata());
 
   if (pending_enabled_actions_.has_value()) {
     media_controls_->UpdateActionButtonAvailability(*pending_enabled_actions_);
@@ -183,6 +189,7 @@ void UnifiedMediaControlsController::UpdateSession() {
   if (pending_artwork_.has_value())
     UpdateArtwork(*pending_artwork_, false /* should_start_hide_timer */);
 
+  MaybeShowMediaControlsOrEmptyState();
   ResetPendingData();
 }
 
@@ -220,7 +227,7 @@ void UnifiedMediaControlsController::UpdateArtwork(
     return;
   }
 
-  // Start |hide_artork_timer_} if not already started and wait for
+  // Start |hide_artork_timer_| if not already started and wait for
   // artwork update.
   if (!hide_artwork_timer_->IsRunning()) {
     hide_artwork_timer_->Start(
@@ -246,12 +253,31 @@ void UnifiedMediaControlsController::PerformAction(
 
 void UnifiedMediaControlsController::ResetPendingData() {
   pending_session_id_.reset();
-  pending_playback_state_.reset();
+  pending_session_info_.reset();
   pending_metadata_.reset();
   pending_enabled_actions_.reset();
   pending_artwork_.reset();
 }
 
+bool UnifiedMediaControlsController::ShouldShowMediaControls() const {
+  if (!session_info_ || !session_info_->is_controllable)
+    return false;
+
+  if (session_metadata_.title.empty())
+    return false;
+
+  return true;
+}
+
+void UnifiedMediaControlsController::MaybeShowMediaControlsOrEmptyState() {
+  if (ShouldShowMediaControls()) {
+    delegate_->ShowMediaControls();
+    media_controls_->OnNewMediaSession();
+  } else {
+    media_controls_->ShowEmptyState();
+  }
+}
+
 void UnifiedMediaControlsController::FlushForTesting() {
   media_controller_remote_.FlushForTesting();  // IN-TEST
 }

ash/system/media/unified_media_controls_controller.h

@@ -79,6 +79,10 @@ class ASH_EXPORT UnifiedMediaControlsController
   // Reset all pending data to empty.
   void ResetPendingData();
 
+  bool ShouldShowMediaControls() const;
+
+  void MaybeShowMediaControlsOrEmptyState();
+
   // Weak ptr, owned by view hierarchy.
   UnifiedMediaControlsView* media_controls_ = nullptr;
 
@@ -101,12 +105,16 @@ class ASH_EXPORT UnifiedMediaControlsController
 
   base::Optional<base::UnguessableToken> media_session_id_;
 
+  media_session::mojom::MediaSessionInfoPtr session_info_;
+
+  media_session::MediaMetadata session_metadata_;
+
   base::flat_set<media_session::mojom::MediaSessionAction> enabled_actions_;
 
   // Pending data to update when |freeze_session_tmier_| fired.
   base::Optional<base::UnguessableToken> pending_session_id_;
-  base::Optional<media_session::mojom::MediaPlaybackState>
-      pending_playback_state_;
+  base::Optional<media_session::mojom::MediaSessionInfoPtr>
+      pending_session_info_;
   base::Optional<media_session::MediaMetadata> pending_metadata_;
   base::Optional<base::flat_set<media_session::mojom::MediaSessionAction>>
       pending_enabled_actions_;

ash/system/media/unified_media_controls_controller_unittest.cc

@@ -97,11 +97,25 @@ class UnifiedMediaControlsControllerTest : public AshTestBase {
     NotifyActionsChanged();
   }
 
+  void SimulateNewMediaSessionWithData(base::UnguessableToken request_id) {
+    controller()->MediaSessionChanged(request_id);
+
+    media_session::mojom::MediaSessionInfoPtr session_info(
+        media_session::mojom::MediaSessionInfo::New());
+    session_info->is_controllable = true;
+    controller()->MediaSessionInfoChanged(session_info.Clone());
+
+    media_session::MediaMetadata metadata;
+    metadata.title = base::ASCIIToUTF16("foo");
+    controller()->MediaSessionMetadataChanged(metadata);
+  }
+
   void SimulateMediaPlaybackStateChanged(
       media_session::mojom::MediaPlaybackState playback_state) {
     media_session::mojom::MediaSessionInfoPtr session_info(
         media_session::mojom::MediaSessionInfo::New());
     session_info->playback_state = playback_state;
+    session_info->is_controllable = true;
     controller()->MediaSessionInfoChanged(session_info.Clone());
   }
 
@@ -172,6 +186,8 @@ class UnifiedMediaControlsControllerTest : public AshTestBase {
 
 TEST_F(UnifiedMediaControlsControllerTest, ActionButtonsTest) {
   CreateWidget();
+  SimulateNewMediaSessionWithData(base::UnguessableToken::Create());
+
   EnableAction(MediaSessionAction::kPreviousTrack);
   EnableAction(MediaSessionAction::kNextTrack);
   EnableAction(MediaSessionAction::kPlay);
@@ -237,6 +253,8 @@ TEST_F(UnifiedMediaControlsControllerTest, ActionButtonVisibility) {
 }
 
 TEST_F(UnifiedMediaControlsControllerTest, MetadataUpdate) {
+  SimulateNewMediaSessionWithData(base::UnguessableToken::Create());
+
   media_session::MediaMetadata metadata;
   metadata.title = base::ASCIIToUTF16("title");
   metadata.artist = base::ASCIIToUTF16("artist");
@@ -362,7 +380,28 @@ TEST_F(UnifiedMediaControlsControllerTest,
   auto request_id = base::UnguessableToken::Create();
 
   EXPECT_FALSE(delegate()->IsControlsVisible());
+
+  // Don't show controls if we don't have metadata and session info.
   controller()->MediaSessionChanged(request_id);
+  EXPECT_FALSE(delegate()->IsControlsVisible());
+
+  // Test that we need to have both media title and session info
+  // to display the controls.
+  media_session::mojom::MediaSessionInfoPtr session_info(
+      media_session::mojom::MediaSessionInfo::New());
+  session_info->is_controllable = true;
+  controller()->MediaSessionInfoChanged(session_info.Clone());
+  EXPECT_FALSE(delegate()->IsControlsVisible());
+
+  session_info->is_controllable = false;
+  controller()->MediaSessionInfoChanged(session_info.Clone());
+  media_session::MediaMetadata metadata;
+  metadata.title = base::ASCIIToUTF16("foo");
+  controller()->MediaSessionMetadataChanged(metadata);
+  EXPECT_FALSE(delegate()->IsControlsVisible());
+
+  // Controls should show with metadata and controllable session.
+  SimulateNewMediaSessionWithData(request_id);
   EXPECT_TRUE(delegate()->IsControlsVisible());
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
@@ -375,7 +414,7 @@ TEST_F(UnifiedMediaControlsControllerTest,
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
   // Session resumes, controls should still be in normal state.
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   task_environment()->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
@@ -392,7 +431,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyState) {
 
   // Show media controls.
   auto request_id = base::UnguessableToken::Create();
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   EXPECT_TRUE(delegate()->IsControlsVisible());
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
@@ -430,7 +469,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyState) {
   generator->ClickLeftButton();
 
   // Media controls should get back to normal state.
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
   EXPECT_TRUE(artist_label()->GetVisible());
@@ -447,7 +486,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyStateWithArtwork) {
   auto request_id = base::UnguessableToken::Create();
 
   EXPECT_FALSE(delegate()->IsControlsVisible());
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   EXPECT_TRUE(delegate()->IsControlsVisible());
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
@@ -470,7 +509,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyStateWithArtwork) {
   EXPECT_NE(artwork_view()->background(), nullptr);
 
   // Session and artwork updated, artwotk view should be back in normal state.
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   controller()->MediaControllerImageChanged(
       media_session::mojom::MediaSessionImageType::kArtwork, artwork);
   EXPECT_TRUE(artwork_view()->GetVisible());
@@ -479,18 +518,23 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyStateWithArtwork) {
 
 TEST_F(UnifiedMediaControlsControllerTest, FreezeControlsWhenUpdateSession) {
   auto request_id = base::UnguessableToken::Create();
-  controller()->MediaSessionChanged(request_id);
+  SimulateNewMediaSessionWithData(request_id);
   EnableAction(MediaSessionAction::kPlay);
   EnableAction(MediaSessionAction::kPause);
   SimulateMediaPlaybackStateChanged(
       media_session::mojom::MediaPlaybackState::kPlaying);
 
+  media_session::MediaMetadata init_metadata;
+  init_metadata.title = base::ASCIIToUTF16("init_title");
+  init_metadata.artist = base::ASCIIToUTF16("init_artist");
+  controller()->MediaSessionMetadataChanged(init_metadata);
+
   // Initial state of media controls.
   EXPECT_TRUE(GetActionButton(MediaSessionAction::kPause)->GetVisible());
   EXPECT_FALSE(
       GetActionButton(MediaSessionAction::kPreviousTrack)->GetVisible());
-  EXPECT_EQ(title_label()->GetText(), base::ASCIIToUTF16(""));
-  EXPECT_EQ(artist_label()->GetText(), base::ASCIIToUTF16(""));
+  EXPECT_EQ(title_label()->GetText(), init_metadata.title);
+  EXPECT_EQ(artist_label()->GetText(), init_metadata.artist);
   EXPECT_FALSE(artwork_view()->GetVisible());
 
   controller()->MediaSessionChanged(base::nullopt);
@@ -501,8 +545,8 @@ TEST_F(UnifiedMediaControlsControllerTest, FreezeControlsWhenUpdateSession) {
   metadata.artist = base::ASCIIToUTF16("artist");
   controller()->MediaSessionMetadataChanged(metadata);
 
-  EXPECT_EQ(title_label()->GetText(), base::ASCIIToUTF16(""));
-  EXPECT_EQ(artist_label()->GetText(), base::ASCIIToUTF16(""));
+  EXPECT_EQ(title_label()->GetText(), init_metadata.title);
+  EXPECT_EQ(artist_label()->GetText(), init_metadata.artist);
 
   // Test that media seeesion info update is ignored when we waiting for new
   // session.

ash/system/media/unified_media_controls_view.cc

@@ -328,6 +328,9 @@ void UnifiedMediaControlsView::OnThemeChanged() {
 }
 
 void UnifiedMediaControlsView::ShowEmptyState() {
+  if (is_in_empty_state_)
+    return;
+
   is_in_empty_state_ = true;
 
   title_label_->SetText(