[Picture in Picture] Show correct media controls when window is opened.

Currently, the initial media control shown is always the play
(untoggled) icon. This change checks for whether or not the play is
currently active, or playing, when entering Picture-in-Picture mode.

This change also updates how media player state (active / inactive) and
other relevant information (e.g. media player id) is retrieved. The
MediaWebContentsObserver and MediaPlayerId are not updated after
entering Picture-in-Picture mode.

BUG: 836389
Change-Id: I64baafebc20bf91fd457866237d1ec8dd526fd68
Reviewed-on: https://chromium-review.googlesource.com/1034158
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555952}

chrome/browser/ui/views/overlay/overlay_window_views.cc

@@ -192,7 +192,7 @@ void OverlayWindowViews::SetUpViews() {
                             kPlayPauseIconSize.width(), SK_ColorWHITE);
   play_pause_controls_view_->SetToggledImage(views::Button::STATE_NORMAL,
                                              &pause_icon);
-  play_pause_controls_view_->SetToggled(false);
+  play_pause_controls_view_->SetToggled(controller_->IsPlayerActive());
 
   // Paint to ui::Layers to use in the OverlaySurfaceEmbedder.
   video_view_->SetPaintToLayer(ui::LAYER_TEXTURED);
@@ -309,6 +309,9 @@ void OverlayWindowViews::OnMouseEvent(ui::MouseEvent* event) {
         controller_->Close();
         event->SetHandled();
       } else if (GetPlayPauseControlsBounds().Contains(event->location())) {
+        // Retrieve expected active state based on what command was sent in
+        // TogglePlayPause() since the IPC message may not have been propogated
+        // the media player yet.
         bool is_active = controller_->TogglePlayPause();
         play_pause_controls_view_->SetToggled(is_active);
         event->SetHandled();

content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

@@ -47,23 +47,16 @@ PictureInPictureWindowControllerImpl::~PictureInPictureWindowControllerImpl() {
   if (initiator_->IsBeingDestroyed())
     return;
 
-  content::MediaWebContentsObserver* observer =
-      static_cast<content::WebContentsImpl* const>(initiator_)
-          ->media_web_contents_observer();
-  DCHECK(observer);
-
-  base::Optional<content::WebContentsObserver::MediaPlayerId> player_id =
-      observer->GetPictureInPictureVideoMediaPlayerId();
-
   // |this| is torn down when there is a new Picture-in-Picture initiator, such
   // as when a video in another tab requests to enter Picture-in-Picture. In
   // cases like this, pause the current video so there is only one video
   // playing at a time.
-  if (player_id.has_value() && observer->IsPlayerActive(*player_id)) {
-    player_id->first->Send(new MediaPlayerDelegateMsg_Pause(
-        player_id->first->GetRoutingID(), player_id->second));
-    player_id->first->Send(new MediaPlayerDelegateMsg_EndPictureInPictureMode(
-        player_id->first->GetRoutingID(), player_id->second));
+  if (IsPlayerActive()) {
+    media_player_id_->first->Send(new MediaPlayerDelegateMsg_Pause(
+        media_player_id_->first->GetRoutingID(), media_player_id_->second));
+    media_player_id_->first->Send(
+        new MediaPlayerDelegateMsg_EndPictureInPictureMode(
+            media_player_id_->first->GetRoutingID(), media_player_id_->second));
   }
 }
 
@@ -72,9 +65,13 @@ PictureInPictureWindowControllerImpl::PictureInPictureWindowControllerImpl(
     : initiator_(initiator) {
   DCHECK(initiator_);
 
+  media_web_contents_observer_ = static_cast<WebContentsImpl* const>(initiator_)
+                                     ->media_web_contents_observer();
+  media_player_id_ =
+      media_web_contents_observer_->GetPictureInPictureVideoMediaPlayerId();
+
   window_ =
-      content::GetContentClient()->browser()->CreateWindowForPictureInPicture(
-          this);
+      GetContentClient()->browser()->CreateWindowForPictureInPicture(this);
   DCHECK(window_) << "Picture in Picture requires a valid window.";
 }
 
@@ -91,16 +88,11 @@ void PictureInPictureWindowControllerImpl::Close() {
 
   surface_id_ = viz::SurfaceId();
 
-  content::MediaWebContentsObserver* observer =
-      static_cast<content::WebContentsImpl* const>(initiator_)
-          ->media_web_contents_observer();
-  base::Optional<content::WebContentsObserver::MediaPlayerId> player_id =
-      observer->GetPictureInPictureVideoMediaPlayerId();
-  DCHECK(player_id.has_value());
-
-  if (observer->IsPlayerActive(*player_id))
-    player_id->first->Send(new MediaPlayerDelegateMsg_EndPictureInPictureMode(
-        player_id->first->GetRoutingID(), player_id->second));
+  if (IsPlayerActive()) {
+    media_player_id_->first->Send(
+        new MediaPlayerDelegateMsg_EndPictureInPictureMode(
+            media_player_id_->first->GetRoutingID(), media_player_id_->second));
+  }
 }
 
 void PictureInPictureWindowControllerImpl::EmbedSurface(
@@ -126,6 +118,14 @@ void PictureInPictureWindowControllerImpl::UpdateLayerBounds() {
     embedder_->UpdateLayerBounds();
 }
 
+bool PictureInPictureWindowControllerImpl::IsPlayerActive() {
+  if (!media_player_id_.has_value())
+    media_web_contents_observer_->GetPictureInPictureVideoMediaPlayerId();
+
+  return media_player_id_.has_value() &&
+         media_web_contents_observer_->IsPlayerActive(*media_player_id_);
+}
+
 WebContents* PictureInPictureWindowControllerImpl::GetInitiatorWebContents() {
   return initiator_;
 }
@@ -133,21 +133,14 @@ WebContents* PictureInPictureWindowControllerImpl::GetInitiatorWebContents() {
 bool PictureInPictureWindowControllerImpl::TogglePlayPause() {
   DCHECK(window_ && window_->IsActive());
 
-  content::MediaWebContentsObserver* observer =
-      static_cast<content::WebContentsImpl* const>(initiator_)
-          ->media_web_contents_observer();
-  base::Optional<content::WebContentsObserver::MediaPlayerId> player_id =
-      observer->GetPictureInPictureVideoMediaPlayerId();
-  DCHECK(player_id.has_value());
-
-  if (observer->IsPlayerActive(*player_id)) {
-    player_id->first->Send(new MediaPlayerDelegateMsg_Pause(
-        player_id->first->GetRoutingID(), player_id->second));
+  if (IsPlayerActive()) {
+    media_player_id_->first->Send(new MediaPlayerDelegateMsg_Pause(
+        media_player_id_->first->GetRoutingID(), media_player_id_->second));
     return false;
   }
 
-  player_id->first->Send(new MediaPlayerDelegateMsg_Play(
-      player_id->first->GetRoutingID(), player_id->second));
+  media_player_id_->first->Send(new MediaPlayerDelegateMsg_Play(
+      media_player_id_->first->GetRoutingID(), media_player_id_->second));
   return true;
 }
 

content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h

@@ -13,6 +13,7 @@
 namespace content {
 class OverlaySurfaceEmbedder;
 class WebContents;
+class MediaWebContentsObserver;
 
 // TODO(thakis,mlamouri): PictureInPictureWindowControllerImpl isn't
 // CONTENT_EXPORT'd because it creates complicated build issues with
@@ -39,6 +40,7 @@ class PictureInPictureWindowControllerImpl
                                    const gfx::Size& natural_size) override;
   CONTENT_EXPORT OverlayWindow* GetWindowForTesting() override;
   CONTENT_EXPORT void UpdateLayerBounds() override;
+  CONTENT_EXPORT bool IsPlayerActive() override;
   CONTENT_EXPORT WebContents* GetInitiatorWebContents() override;
   CONTENT_EXPORT bool TogglePlayPause() override;
 
@@ -52,7 +54,12 @@ class PictureInPictureWindowControllerImpl
 
   std::unique_ptr<OverlayWindow> window_;
   std::unique_ptr<OverlaySurfaceEmbedder> embedder_;
-  content::WebContents* const initiator_;
+  WebContents* const initiator_;
+
+  // Used to determine the state of the media player and route messages to
+  // the corresponding media player with id |media_player_id_|.
+  MediaWebContentsObserver* media_web_contents_observer_;
+  base::Optional<WebContentsObserver::MediaPlayerId> media_player_id_;
 
   viz::SurfaceId surface_id_;
 

content/public/browser/picture_in_picture_window_controller.h

@@ -39,6 +39,7 @@ class PictureInPictureWindowController {
                             const gfx::Size& natural_size) = 0;
   virtual OverlayWindow* GetWindowForTesting() = 0;
   virtual void UpdateLayerBounds() = 0;
+  virtual bool IsPlayerActive() = 0;
   virtual WebContents* GetInitiatorWebContents() = 0;
 
   // Commands.

media/blink/webmediaplayer_impl.cc

@@ -781,12 +781,14 @@ void WebMediaPlayerImpl::EnterPictureInPicture() {
   if (!pip_surface_id_.is_valid())
     return;
 
-  pip_surface_info_cb_.Run(pip_surface_id_, pipeline_metadata_.natural_size);
-
   // Updates the MediaWebContentsObserver with |delegate_id_| to track which
   // media player is in Picture-in-Picture mode.
+  // This must be called before |pip_surface_info_cb_| to ensure the
+  // Picture-in-Picture media player id is set before the controller is set up.
   delegate_->DidPictureInPictureSourceChange(delegate_id_);
 
+  pip_surface_info_cb_.Run(pip_surface_id_, pipeline_metadata_.natural_size);
+
   if (client_)
     client_->PictureInPictureStarted();
 }