[Media Notification] Move actions to enum

Move the action buttons to using the MediaSessionAction
enum now that it is available.

BUG=893296

Change-Id: I174d7f9aadc0b1cf89b0b1a2f01d8e2a5269aead
Reviewed-on: https://chromium-review.googlesource.com/c/1320653
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611274}

ash/media/media_notification_controller.cc

@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "services/media_session/public/mojom/constants.mojom.h"
 #include "services/media_session/public/mojom/media_controller.mojom.h"
+#include "services/media_session/public/mojom/media_session.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "ui/gfx/image/image.h"
 #include "ui/message_center/public/cpp/notification.h"
@@ -21,6 +22,8 @@
 
 namespace ash {
 
+using media_session::mojom::MediaSessionAction;
+
 namespace {
 
 constexpr base::TimeDelta kDefaultSeekTime =
@@ -138,25 +141,25 @@ void MediaNotificationController::OnNotificationClicked(
     base::Optional<int> button_id) {
   DCHECK(button_id.has_value());
 
-  // TODO(beccahughes): Replace with MediaSessionAction enum when moved.
-  switch (*button_id) {
-    case 0:
+  switch (static_cast<MediaSessionAction>(*button_id)) {
+    case MediaSessionAction::kPreviousTrack:
       media_controller_ptr_->PreviousTrack();
       break;
-    case 1:
+    case MediaSessionAction::kSeekBackward:
       media_controller_ptr_->Seek(kDefaultSeekTime * -1);
       break;
-    case 2:
-      media_controller_ptr_->ToggleSuspendResume();
+    case MediaSessionAction::kPlay:
+      media_controller_ptr_->Resume();
+      break;
+    case MediaSessionAction::kPause:
+      media_controller_ptr_->Suspend();
       break;
-    case 3:
+    case MediaSessionAction::kSeekForward:
       media_controller_ptr_->Seek(kDefaultSeekTime);
       break;
-    case 4:
+    case MediaSessionAction::kNextTrack:
       media_controller_ptr_->NextTrack();
       break;
-    default:
-      NOTREACHED();
   }
 }
 

ash/media/media_notification_view.cc

@@ -8,6 +8,7 @@
 #include "ash/media/media_notification_controller.h"
 #include "ash/shell.h"
 #include "components/vector_icons/vector_icons.h"
+#include "services/media_session/public/mojom/media_session.mojom.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/message_center_constants.h"
 #include "ui/message_center/views/notification_control_buttons_view.h"
@@ -18,6 +19,8 @@
 
 namespace ash {
 
+using media_session::mojom::MediaSessionAction;
+
 namespace {
 
 // Dimensions.
@@ -61,11 +64,14 @@ MediaNotificationView::MediaNotificationView(
       views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
   AddChildView(button_row_);
 
-  CreateMediaButton(vector_icons::kMediaPreviousTrackIcon);
-  CreateMediaButton(vector_icons::kMediaSeekBackwardIcon);
+  CreateMediaButton(vector_icons::kMediaPreviousTrackIcon,
+                    MediaSessionAction::kPreviousTrack);
+  CreateMediaButton(vector_icons::kMediaSeekBackwardIcon,
+                    MediaSessionAction::kSeekBackward);
 
   // |play_pause_button_| toggles playback.
   play_pause_button_ = views::CreateVectorToggleImageButton(this);
+  play_pause_button_->set_tag(static_cast<int>(MediaSessionAction::kPlay));
   SkColor play_button_color = GetMediaNotificationColor(*play_pause_button_);
   views::SetImageFromVectorIcon(play_pause_button_,
                                 vector_icons::kPlayArrowIcon,
@@ -75,8 +81,10 @@ MediaNotificationView::MediaNotificationView(
                                        kMediaButtonIconSize, play_button_color);
   button_row_->AddChildView(play_pause_button_);
 
-  CreateMediaButton(vector_icons::kMediaSeekForwardIcon);
-  CreateMediaButton(vector_icons::kMediaNextTrackIcon);
+  CreateMediaButton(vector_icons::kMediaSeekForwardIcon,
+                    MediaSessionAction::kSeekForward);
+  CreateMediaButton(vector_icons::kMediaNextTrackIcon,
+                    MediaSessionAction::kNextTrack);
 
   // TODO(beccahughes): Add remaining UI for notification.
 
@@ -131,15 +139,18 @@ void MediaNotificationView::ButtonPressed(views::Button* sender,
                                           const ui::Event& event) {
   if (sender->parent() == button_row_) {
     message_center::MessageCenter::Get()->ClickOnNotificationButton(
-        notification_id(), sender->parent()->GetIndexOf(sender));
+        notification_id(), sender->tag());
   }
 }
 
 void MediaNotificationView::UpdateWithMediaSessionInfo(
     const media_session::mojom::MediaSessionInfoPtr& session_info) {
-  play_pause_button_->SetToggled(
-      session_info->playback_state ==
-      media_session::mojom::MediaPlaybackState::kPlaying);
+  bool playing = session_info->playback_state ==
+                 media_session::mojom::MediaPlaybackState::kPlaying;
+  play_pause_button_->SetToggled(playing);
+  play_pause_button_->set_tag(
+      playing ? static_cast<int>(MediaSessionAction::kPause)
+              : static_cast<int>(MediaSessionAction::kPlay));
 }
 
 void MediaNotificationView::UpdateControlButtonsVisibilityWithNotification(
@@ -152,8 +163,10 @@ void MediaNotificationView::UpdateControlButtonsVisibilityWithNotification(
   UpdateControlButtonsVisibility();
 }
 
-void MediaNotificationView::CreateMediaButton(const gfx::VectorIcon& icon) {
+void MediaNotificationView::CreateMediaButton(const gfx::VectorIcon& icon,
+                                              MediaSessionAction action) {
   views::ImageButton* button = views::CreateVectorImageButton(this);
+  button->set_tag(static_cast<int>(action));
   views::SetImageFromVectorIcon(button, icon, kMediaButtonIconSize,
                                 GetMediaNotificationColor(*button));
   button_row_->AddChildView(button);

ash/media/media_notification_view.h

@@ -11,6 +11,10 @@
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/image_button.h"
 
+namespace media_session {
+enum class MediaSessionAction;
+}  // namespace media_session
+
 namespace message_center {
 class NotificationHeaderView;
 }  // namespace message_center
@@ -55,8 +59,10 @@ class ASH_EXPORT MediaNotificationView : public message_center::MessageView,
   void UpdateControlButtonsVisibilityWithNotification(
       const message_center::Notification& notification);
 
-  // Creates an image button with |icon| and adds it to |button_row_|.
-  void CreateMediaButton(const gfx::VectorIcon& icon);
+  // Creates an image button with |icon| and adds it to |button_row_|. When
+  // clicked it will trigger |action| on the sesssion.
+  void CreateMediaButton(const gfx::VectorIcon& icon,
+                         media_session::mojom::MediaSessionAction action);
 
   // View containing close and settings buttons.
   std::unique_ptr<message_center::NotificationControlButtonsView>

ash/media/media_notification_view_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "services/media_session/public/cpp/test/test_media_controller.h"
 #include "services/media_session/public/mojom/audio_focus.mojom.h"
+#include "services/media_session/public/mojom/media_session.mojom.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/message_center/message_center.h"
@@ -27,6 +28,7 @@
 
 namespace ash {
 
+using media_session::mojom::MediaSessionAction;
 using media_session::test::TestMediaController;
 
 namespace {
@@ -124,6 +126,17 @@ class MediaNotificationViewTest : public AshTestBase {
 
   views::View* button_row() const { return view_->button_row_; }
 
+  views::Button* GetButtonForAction(MediaSessionAction action) const {
+    for (int i = 0; i < button_row()->child_count(); ++i) {
+      views::Button* child = views::Button::AsButton(button_row()->child_at(i));
+
+      if (child->tag() == static_cast<int>(action))
+        return child;
+    }
+
+    return nullptr;
+  }
+
  private:
   std::unique_ptr<message_center::MessageView> CreateAndCaptureCustomView(
       const message_center::Notification& notification) {
@@ -179,14 +192,23 @@ TEST_F(MediaNotificationViewTest, ButtonsSanityCheck) {
     EXPECT_EQ(kMediaButtonIconSize, child->width());
     EXPECT_EQ(kMediaButtonIconSize, child->height());
   }
+
+  EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kPlay));
+  EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kPreviousTrack));
+  EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kNextTrack));
+  EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kSeekBackward));
+  EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kSeekForward));
+
+  // |kPause| cannot be present if |kPlay| is.
+  EXPECT_FALSE(GetButtonForAction(MediaSessionAction::kPause));
 }
 
 TEST_F(MediaNotificationViewTest, NextTrackButtonClick) {
   EXPECT_EQ(0, media_controller()->next_track_count());
 
   gfx::Point cursor_location(1, 1);
-  views::View::ConvertPointToScreen(button_row()->child_at(4),
-                                    &cursor_location);
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kNextTrack), &cursor_location);
   GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
@@ -194,25 +216,45 @@ TEST_F(MediaNotificationViewTest, NextTrackButtonClick) {
   EXPECT_EQ(1, media_controller()->next_track_count());
 }
 
-TEST_F(MediaNotificationViewTest, PlayPauseButtonClick) {
-  EXPECT_EQ(0, media_controller()->toggle_suspend_resume_count());
+TEST_F(MediaNotificationViewTest, PlayButtonClick) {
+  EXPECT_EQ(0, media_controller()->resume_count());
+
+  gfx::Point cursor_location(1, 1);
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kPlay), &cursor_location);
+  GetEventGenerator()->MoveMouseTo(cursor_location);
+  GetEventGenerator()->ClickLeftButton();
+  Shell::Get()->media_notification_controller()->FlushForTesting();
+
+  EXPECT_EQ(1, media_controller()->resume_count());
+}
+
+TEST_F(MediaNotificationViewTest, PauseButtonClick) {
+  EXPECT_EQ(0, media_controller()->suspend_count());
+
+  media_session::mojom::MediaSessionInfoPtr session_info(
+      media_session::mojom::MediaSessionInfo::New());
+  session_info->playback_state =
+      media_session::mojom::MediaPlaybackState::kPlaying;
+  Shell::Get()->media_notification_controller()->MediaSessionInfoChanged(
+      session_info.Clone());
 
   gfx::Point cursor_location(1, 1);
-  views::View::ConvertPointToScreen(button_row()->child_at(2),
-                                    &cursor_location);
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kPause), &cursor_location);
   GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
 
-  EXPECT_EQ(1, media_controller()->toggle_suspend_resume_count());
+  EXPECT_EQ(1, media_controller()->suspend_count());
 }
 
 TEST_F(MediaNotificationViewTest, PreviousTrackButtonClick) {
   EXPECT_EQ(0, media_controller()->previous_track_count());
 
   gfx::Point cursor_location(1, 1);
-  views::View::ConvertPointToScreen(button_row()->child_at(0),
-                                    &cursor_location);
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kPreviousTrack), &cursor_location);
   GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
@@ -224,8 +266,8 @@ TEST_F(MediaNotificationViewTest, SeekBackwardButtonClick) {
   EXPECT_EQ(0, media_controller()->seek_backward_count());
 
   gfx::Point cursor_location(1, 1);
-  views::View::ConvertPointToScreen(button_row()->child_at(1),
-                                    &cursor_location);
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kSeekBackward), &cursor_location);
   GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
@@ -237,9 +279,9 @@ TEST_F(MediaNotificationViewTest, SeekForwardButtonClick) {
   EXPECT_EQ(0, media_controller()->seek_forward_count());
 
   gfx::Point cursor_location(1, 1);
-  views::View::ConvertPointToScreen(button_row()->child_at(3),
-                                    &cursor_location);
-  GetEventGenerator()->MoveMouseTo(cursor_location.x(), cursor_location.y());
+  views::View::ConvertPointToScreen(
+      GetButtonForAction(MediaSessionAction::kSeekForward), &cursor_location);
+  GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
 
@@ -251,7 +293,7 @@ TEST_F(MediaNotificationViewTest, ClickNotification) {
 
   gfx::Point cursor_location(1, 1);
   views::View::ConvertPointToScreen(view(), &cursor_location);
-  GetEventGenerator()->MoveMouseTo(cursor_location.x(), cursor_location.y());
+  GetEventGenerator()->MoveMouseTo(cursor_location);
   GetEventGenerator()->ClickLeftButton();
   Shell::Get()->media_notification_controller()->FlushForTesting();
 
@@ -260,8 +302,8 @@ TEST_F(MediaNotificationViewTest, ClickNotification) {
 
 TEST_F(MediaNotificationViewTest, PlayToggle_FromActiveSessionChanged) {
   {
-    views::ToggleImageButton* button =
-        static_cast<views::ToggleImageButton*>(button_row()->child_at(2));
+    views::ToggleImageButton* button = static_cast<views::ToggleImageButton*>(
+        GetButtonForAction(MediaSessionAction::kPlay));
     ASSERT_EQ(views::ToggleImageButton::kViewClassName, button->GetClassName());
     EXPECT_FALSE(button->toggled_for_testing());
   }
@@ -284,16 +326,16 @@ TEST_F(MediaNotificationViewTest, PlayToggle_FromActiveSessionChanged) {
   ShowNotificationAndCaptureView(std::move(session_info));
 
   {
-    views::ToggleImageButton* button =
-        static_cast<views::ToggleImageButton*>(button_row()->child_at(2));
+    views::ToggleImageButton* button = static_cast<views::ToggleImageButton*>(
+        GetButtonForAction(MediaSessionAction::kPause));
     ASSERT_EQ(views::ToggleImageButton::kViewClassName, button->GetClassName());
     EXPECT_TRUE(button->toggled_for_testing());
   }
 }
 
 TEST_F(MediaNotificationViewTest, PlayToggle_FromObserver_Empty) {
-  views::ToggleImageButton* button =
-      static_cast<views::ToggleImageButton*>(button_row()->child_at(2));
+  views::ToggleImageButton* button = static_cast<views::ToggleImageButton*>(
+      GetButtonForAction(MediaSessionAction::kPlay));
   ASSERT_EQ(views::ToggleImageButton::kViewClassName, button->GetClassName());
   EXPECT_FALSE(button->toggled_for_testing());
 
@@ -303,8 +345,8 @@ TEST_F(MediaNotificationViewTest, PlayToggle_FromObserver_Empty) {
 }
 
 TEST_F(MediaNotificationViewTest, PlayToggle_FromObserver_PlaybackState) {
-  views::ToggleImageButton* button =
-      static_cast<views::ToggleImageButton*>(button_row()->child_at(2));
+  views::ToggleImageButton* button = static_cast<views::ToggleImageButton*>(
+      GetButtonForAction(MediaSessionAction::kPlay));
   ASSERT_EQ(views::ToggleImageButton::kViewClassName, button->GetClassName());
   EXPECT_FALSE(button->toggled_for_testing());
 

services/media_session/public/cpp/test/test_media_controller.cc

@@ -17,6 +17,14 @@ mojom::MediaControllerPtr TestMediaController::CreateMediaControllerPtr() {
   return ptr;
 }
 
+void TestMediaController::Suspend() {
+  ++suspend_count_;
+}
+
+void TestMediaController::Resume() {
+  ++resume_count_;
+}
+
 void TestMediaController::ToggleSuspendResume() {
   ++toggle_suspend_resume_count_;
 }

services/media_session/public/cpp/test/test_media_controller.h

@@ -22,8 +22,8 @@ class COMPONENT_EXPORT(MEDIA_SESSION_TEST_SUPPORT_CPP) TestMediaController
   mojom::MediaControllerPtr CreateMediaControllerPtr();
 
   // mojom::MediaController:
-  void Suspend() override {}
-  void Resume() override {}
+  void Suspend() override;
+  void Resume() override;
   void ToggleSuspendResume() override;
   void AddObserver(mojom::MediaSessionObserverPtr) override;
   void PreviousTrack() override;
@@ -34,6 +34,8 @@ class COMPONENT_EXPORT(MEDIA_SESSION_TEST_SUPPORT_CPP) TestMediaController
     return toggle_suspend_resume_count_;
   }
 
+  int suspend_count() const { return suspend_count_; }
+  int resume_count() const { return resume_count_; }
   int add_observer_count() const { return add_observer_count_; }
   int previous_track_count() const { return previous_track_count_; }
   int next_track_count() const { return next_track_count_; }
@@ -42,6 +44,8 @@ class COMPONENT_EXPORT(MEDIA_SESSION_TEST_SUPPORT_CPP) TestMediaController
 
  private:
   int toggle_suspend_resume_count_ = 0;
+  int suspend_count_ = 0;
+  int resume_count_ = 0;
   int add_observer_count_ = 0;
   int previous_track_count_ = 0;
   int next_track_count_ = 0;