CrOS GMC: Freeze controls when session changes.

This CL extends the freeze timer function to allow the controls
freeze between sessions. This will avoid controls changing
from one session to another when the active session goes next track.
We will now always wait for a brief time to see if the previous session
resumes before we update our controls.

Bug: 1134458

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

ash/system/media/unified_media_controls_controller.cc

@@ -21,7 +21,7 @@ constexpr int kMinimumArtworkSize = 30;
 constexpr int kDisiredArtworkSize = 48;
 
 // Time to wait for new media session.
-constexpr base::TimeDelta kHideControlsDelay =
+constexpr base::TimeDelta kFreezeControlsTime =
     base::TimeDelta::FromMilliseconds(2000);
 
 // Time to wait for new artwork.
@@ -64,11 +64,13 @@ views::View* UnifiedMediaControlsController::CreateView() {
 
 void UnifiedMediaControlsController::MediaSessionInfoChanged(
     media_session::mojom::MediaSessionInfoPtr session_info) {
-  if (hide_controls_timer_->IsRunning())
+  if (!session_info)
     return;
 
-  if (!session_info)
+  if (freeze_session_timer_->IsRunning()) {
+    pending_playback_state_ = session_info->playback_state;
     return;
+  }
 
   media_controls_->SetIsPlaying(
       session_info->playback_state ==
@@ -78,19 +80,23 @@ void UnifiedMediaControlsController::MediaSessionInfoChanged(
 
 void UnifiedMediaControlsController::MediaSessionMetadataChanged(
     const base::Optional<media_session::MediaMetadata>& metadata) {
-  if (hide_controls_timer_->IsRunning())
+  pending_metadata_ = metadata.value_or(media_session::MediaMetadata());
+  if (freeze_session_timer_->IsRunning())
     return;
 
-  media_session::MediaMetadata session_metadata =
-      metadata.value_or(media_session::MediaMetadata());
-  media_controls_->SetTitle(session_metadata.title);
-  media_controls_->SetArtist(session_metadata.artist);
+  media_controls_->SetTitle(pending_metadata_->title);
+  media_controls_->SetArtist(pending_metadata_->artist);
+  pending_metadata_.reset();
 }
 
 void UnifiedMediaControlsController::MediaSessionActionsChanged(
     const std::vector<media_session::mojom::MediaSessionAction>& actions) {
-  if (hide_controls_timer_->IsRunning())
+  if (freeze_session_timer_->IsRunning()) {
+    pending_enabled_actions_ =
+        base::flat_set<media_session::mojom::MediaSessionAction>(
+            actions.begin(), actions.end());
     return;
+  }
 
   enabled_actions_ = base::flat_set<media_session::mojom::MediaSessionAction>(
       actions.begin(), actions.end());
@@ -99,41 +105,90 @@ void UnifiedMediaControlsController::MediaSessionActionsChanged(
 
 void UnifiedMediaControlsController::MediaSessionChanged(
     const base::Optional<base::UnguessableToken>& request_id) {
-  // Stop the timer if we receive a new active sessoin.
-  if (hide_controls_timer_->IsRunning() && request_id.has_value())
-    hide_controls_timer_->Stop();
+  // If previous session resumes, stop freeze timer if necessary and discard
+  // any pending data.
+  if (request_id == media_session_id_) {
+    if (!freeze_session_timer_->IsRunning())
+      return;
+
+    freeze_session_timer_->Stop();
+    ResetPendingData();
+    return;
+  }
 
-  if (request_id == media_session_id_)
+  // If we don't currently have a session, show media controls.
+  if (!media_session_id_.has_value()) {
+    DCHECK(!freeze_session_timer_->IsRunning());
+    media_session_id_ = request_id;
+    delegate_->ShowMediaControls();
+    media_controls_->OnNewMediaSession();
     return;
+  }
 
-  // Start hide controls timer if there is no active session, wait to
-  // see if we will receive a new session.
-  if (!request_id.has_value()) {
+  // If we do currently have a session and received a new session request,
+  // wait to see if the session will resumes. If it is not resumed after
+  // timeout, update session with all pending data.
+  pending_session_id_ = request_id;
+  if (!freeze_session_timer_->IsRunning()) {
     if (hide_artwork_timer_->IsRunning())
       hide_artwork_timer_->Stop();
 
-    hide_controls_timer_->Start(
-        FROM_HERE, kHideControlsDelay,
-        base::BindOnce(&UnifiedMediaControlsController::ShowEmptyState,
+    freeze_session_timer_->Start(
+        FROM_HERE, kFreezeControlsTime,
+        base::BindOnce(&UnifiedMediaControlsController::UpdateSession,
                        base::Unretained(this)));
-    return;
   }
-
-  if (!media_session_id_.has_value())
-    delegate_->ShowMediaControls();
-  media_session_id_ = request_id;
-  media_controls_->OnNewMediaSession();
 }
 
 void UnifiedMediaControlsController::MediaControllerImageChanged(
     media_session::mojom::MediaSessionImageType type,
     const SkBitmap& bitmap) {
-  if (hide_controls_timer_->IsRunning())
+  if (type != media_session::mojom::MediaSessionImageType::kArtwork)
     return;
 
-  if (type != media_session::mojom::MediaSessionImageType::kArtwork)
+  if (freeze_session_timer_->IsRunning()) {
+    pending_artwork_ = bitmap;
+    return;
+  }
+
+  UpdateArtwork(bitmap, true /* should_start_hide_timer */);
+}
+
+void UnifiedMediaControlsController::UpdateSession() {
+  media_session_id_ = pending_session_id_;
+
+  if (media_session_id_ == base::nullopt) {
+    media_controls_->ShowEmptyState();
+    ResetPendingData();
     return;
+  }
+
+  if (pending_playback_state_.has_value()) {
+    media_controls_->SetIsPlaying(
+        pending_playback_state_ ==
+        media_session::mojom::MediaPlaybackState::kPlaying);
+  }
+
+  if (pending_metadata_.has_value()) {
+    media_controls_->SetTitle(pending_metadata_->title);
+    media_controls_->SetArtist(pending_metadata_->artist);
+  }
 
+  if (pending_enabled_actions_.has_value()) {
+    media_controls_->UpdateActionButtonAvailability(*pending_enabled_actions_);
+    enabled_actions_ = base::flat_set<media_session::mojom::MediaSessionAction>(
+        pending_enabled_actions_->begin(), pending_enabled_actions_->end());
+  }
+
+  if (pending_artwork_.has_value())
+    UpdateArtwork(*pending_artwork_, false /* should_start_hide_timer */);
+
+  ResetPendingData();
+}
+
+void UnifiedMediaControlsController::UpdateArtwork(
+    const SkBitmap& bitmap,
+    bool should_start_hide_timer) {
   // Convert the bitmap to kN32_SkColorType if necessary.
   SkBitmap converted_bitmap;
   if (bitmap.colorType() == kN32_SkColorType) {
@@ -160,6 +215,11 @@ void UnifiedMediaControlsController::MediaControllerImageChanged(
   if (media_controls_->artwork_view()->GetImage().isNull())
     return;
 
+  if (!should_start_hide_timer) {
+    media_controls_->SetArtwork(base::nullopt);
+    return;
+  }
+
   // Start |hide_artork_timer_} if not already started and wait for
   // artwork update.
   if (!hide_artwork_timer_->IsRunning()) {
@@ -176,12 +236,16 @@ void UnifiedMediaControlsController::OnMediaControlsViewClicked() {
 
 void UnifiedMediaControlsController::PerformAction(
     media_session::mojom::MediaSessionAction action) {
-  media_session::PerformMediaSessionAction(action, media_controller_remote_);
+  if (!freeze_session_timer_->IsRunning())
+    media_session::PerformMediaSessionAction(action, media_controller_remote_);
 }
 
-void UnifiedMediaControlsController::ShowEmptyState() {
-  media_session_id_ = base::nullopt;
-  media_controls_->ShowEmptyState();
+void UnifiedMediaControlsController::ResetPendingData() {
+  pending_session_id_.reset();
+  pending_playback_state_.reset();
+  pending_metadata_.reset();
+  pending_enabled_actions_.reset();
+  pending_artwork_.reset();
 }
 
 void UnifiedMediaControlsController::FlushForTesting() {

ash/system/media/unified_media_controls_controller.h

@@ -69,7 +69,15 @@ class ASH_EXPORT UnifiedMediaControlsController
   }
 
  private:
-  void ShowEmptyState();
+  // Update view with pending data if necessary. Called when
+  // |freeze_session_timer| is fired.
+  void UpdateSession();
+
+  // Update artwork in media controls view.
+  void UpdateArtwork(const SkBitmap& bitmap, bool should_start_hide_timer);
+
+  // Reset all pending data to empty.
+  void ResetPendingData();
 
   // Weak ptr, owned by view hierarchy.
   UnifiedMediaControlsView* media_controls_ = nullptr;
@@ -85,7 +93,7 @@ class ASH_EXPORT UnifiedMediaControlsController
   mojo::Receiver<media_session::mojom::MediaControllerImageObserver>
       artwork_observer_receiver_{this};
 
-  std::unique_ptr<base::OneShotTimer> hide_controls_timer_ =
+  std::unique_ptr<base::OneShotTimer> freeze_session_timer_ =
       std::make_unique<base::OneShotTimer>();
 
   std::unique_ptr<base::OneShotTimer> hide_artwork_timer_ =
@@ -94,6 +102,15 @@ class ASH_EXPORT UnifiedMediaControlsController
   base::Optional<base::UnguessableToken> media_session_id_;
 
   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::MediaMetadata> pending_metadata_;
+  base::Optional<base::flat_set<media_session::mojom::MediaSessionAction>>
+      pending_enabled_actions_;
+  base::Optional<SkBitmap> pending_artwork_;
 };
 
 }  // namespace ash

ash/system/media/unified_media_controls_controller_unittest.cc

@@ -24,7 +24,7 @@ using media_session::test::TestMediaController;
 
 namespace {
 
-constexpr int kHideControlsDelay = 2000; /* in milliseconds */
+constexpr int kFreezeControlsTime = 2000; /* in milliseconds */
 constexpr int kHideArtworkDelay = 2000;  /* in milliseconds */
 constexpr int kArtworkCornerRadius = 4;
 constexpr int kArtworkHeight = 40;
@@ -380,7 +380,7 @@ TEST_F(UnifiedMediaControlsControllerTest,
 
   // Still in normal state since we are within waiting delay time frame.
   task_environment()->FastForwardBy(
-      base::TimeDelta::FromMilliseconds(kHideControlsDelay - 1));
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime - 1));
   EXPECT_FALSE(IsMediaControlsInEmptyState());
 
   // Session resumes, controls should still be in normal state.
@@ -391,7 +391,7 @@ TEST_F(UnifiedMediaControlsControllerTest,
   // Hide controls timer expired, controls should be in empty state.
   controller()->MediaSessionChanged(base::nullopt);
   task_environment()->FastForwardBy(
-      base::TimeDelta::FromMilliseconds(kHideControlsDelay));
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime));
   EXPECT_TRUE(IsMediaControlsInEmptyState());
   EXPECT_TRUE(delegate()->IsControlsVisible());
 }
@@ -418,7 +418,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyState) {
   // Media controls should be in empty state after getting empty session.
   controller()->MediaSessionChanged(base::nullopt);
   task_environment()->FastForwardBy(
-      base::TimeDelta::FromMilliseconds(kHideControlsDelay));
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime));
 
   EXPECT_TRUE(IsMediaControlsInEmptyState());
 
@@ -471,7 +471,7 @@ TEST_F(UnifiedMediaControlsControllerTest, MediaControlsEmptyStateWithArtwork) {
 
   controller()->MediaSessionChanged(base::nullopt);
   task_environment()->FastForwardBy(
-      base::TimeDelta::FromMilliseconds(kHideControlsDelay));
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime));
 
   // Artwork view should still be visible and have an background in empty state.
   EXPECT_TRUE(IsMediaControlsInEmptyState());
@@ -556,6 +556,78 @@ TEST_F(UnifiedMediaControlsControllerTest, FreezeControlsWhenUpdateSession) {
       GetActionButton(MediaSessionAction::kPreviousTrack)->GetVisible());
 }
 
+TEST_F(UnifiedMediaControlsControllerTest, FreezeControlsBetweenSessions) {
+  auto request_id = base::UnguessableToken::Create();
+  controller()->MediaSessionChanged(request_id);
+
+  EnableAction(MediaSessionAction::kPreviousTrack);
+  SimulateMediaPlaybackStateChanged(
+      media_session::mojom::MediaPlaybackState::kPlaying);
+
+  media_session::MediaMetadata metadata;
+  metadata.title = base::ASCIIToUTF16("title");
+  metadata.artist = base::ASCIIToUTF16("artist");
+  controller()->MediaSessionMetadataChanged(metadata);
+
+  // Verify initial state
+  EXPECT_TRUE(
+      GetActionButton(MediaSessionAction::kPreviousTrack)->GetVisible());
+  EXPECT_NE(GetActionButton(MediaSessionAction::kPause), nullptr);
+  EXPECT_EQ(metadata.title, title_label()->GetText());
+  EXPECT_EQ(metadata.artist, artist_label()->GetText());
+  EXPECT_FALSE(artwork_view()->GetVisible());
+
+  // Receive a new session with new data.
+  auto new_request_id = base::UnguessableToken::Create();
+  controller()->MediaSessionChanged(new_request_id);
+
+  DisableAction(MediaSessionAction::kPreviousTrack);
+  SimulateMediaPlaybackStateChanged(
+      media_session::mojom::MediaPlaybackState::kPaused);
+
+  media_session::MediaMetadata new_metadata;
+  new_metadata.title = base::ASCIIToUTF16("different title");
+  new_metadata.artist = base::ASCIIToUTF16("different artist");
+  controller()->MediaSessionMetadataChanged(new_metadata);
+
+  SkBitmap artwork;
+  artwork.allocN32Pixels(40, 40);
+  controller()->MediaControllerImageChanged(
+      media_session::mojom::MediaSessionImageType::kArtwork, artwork);
+
+  // Session resumes within freezing timeout.
+  task_environment()->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime - 1));
+  controller()->MediaSessionChanged(request_id);
+
+  // Media controls should not be updated.
+  EXPECT_TRUE(
+      GetActionButton(MediaSessionAction::kPreviousTrack)->GetVisible());
+  EXPECT_NE(GetActionButton(MediaSessionAction::kPause), nullptr);
+  EXPECT_EQ(metadata.title, title_label()->GetText());
+  EXPECT_EQ(metadata.artist, artist_label()->GetText());
+  EXPECT_FALSE(artwork_view()->GetVisible());
+
+  // Receive new session and data.
+  controller()->MediaSessionChanged(new_request_id);
+  DisableAction(MediaSessionAction::kPreviousTrack);
+  SimulateMediaPlaybackStateChanged(
+      media_session::mojom::MediaPlaybackState::kPaused);
+  controller()->MediaSessionMetadataChanged(new_metadata);
+  controller()->MediaControllerImageChanged(
+      media_session::mojom::MediaSessionImageType::kArtwork, artwork);
+
+  // Controls should be updated after freeze timeout.
+  task_environment()->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kFreezeControlsTime));
+  EXPECT_FALSE(
+      GetActionButton(MediaSessionAction::kPreviousTrack)->GetVisible());
+  EXPECT_EQ(GetActionButton(MediaSessionAction::kPause), nullptr);
+  EXPECT_EQ(new_metadata.title, title_label()->GetText());
+  EXPECT_EQ(new_metadata.artist, artist_label()->GetText());
+  EXPECT_TRUE(artwork_view()->GetVisible());
+}
+
 TEST_F(UnifiedMediaControlsControllerTest,
        NotifyDelegateWhenMediaControlsViewClicked) {
   CreateWidget();