Commit 831a1ec9 authored by François Beaufort's avatar François Beaufort Committed by Commit Bot

[Picture-in-Picture] Reset MediaPlayerId when user closes PiP window.

This makes sure the MediaPlayerId of the Picture-in-Picture video is
reset when user closes Picture-in-Picture window manually or when user
requests Picture-in-Picture several times.

Bug: 879592, 889011
Change-Id: I3ceea8de7fc5ee0e2c81268154daaa065b29b92b
Reviewed-on: https://chromium-review.googlesource.com/1236276Reviewed-by: default avatarJochen Eisinger <jochen@chromium.org>
Reviewed-by: default avatarMounir Lamouri <mlamouri@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#594441}
parent dc3de3ce
...@@ -354,6 +354,9 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest, ...@@ -354,6 +354,9 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest,
// Stop video being played Picture-in-Picture and check if that's tracked. // Stop video being played Picture-in-Picture and check if that's tracked.
window_controller()->Close(true /* should_pause_video */); window_controller()->Close(true /* should_pause_video */);
EXPECT_FALSE(active_web_contents->HasPictureInPictureVideo()); EXPECT_FALSE(active_web_contents->HasPictureInPictureVideo());
// Reload page should not crash.
ui_test_utils::NavigateToURL(browser(), test_page_url);
} }
#if !defined(OS_ANDROID) #if !defined(OS_ANDROID)
...@@ -1335,12 +1338,6 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest, ...@@ -1335,12 +1338,6 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest,
EXPECT_TRUE(ExecuteScriptAndExtractBool( EXPECT_TRUE(ExecuteScriptAndExtractBool(
active_web_contents, "isInPictureInPicture();", &in_picture_in_picture)); active_web_contents, "isInPictureInPicture();", &in_picture_in_picture));
EXPECT_FALSE(in_picture_in_picture); EXPECT_FALSE(in_picture_in_picture);
// TODO(edcourtney): When the renderer process is destroyed, it calls into
// MediaWebContentsObserver::ExitPictureInPictureInternal which Closes the
// current PIP. However, this may not be a WebContents sourced PIP, so this
// close can be spurious.
EXPECT_CALL(mock_controller(), Close(_));
} }
IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest, IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest,
......
...@@ -123,6 +123,10 @@ MediaWebContentsObserver::GetPictureInPictureVideoMediaPlayerId() const { ...@@ -123,6 +123,10 @@ MediaWebContentsObserver::GetPictureInPictureVideoMediaPlayerId() const {
return pip_player_; return pip_player_;
} }
void MediaWebContentsObserver::ResetPictureInPictureVideoMediaPlayerId() {
pip_player_.reset();
}
bool MediaWebContentsObserver::OnMessageReceived( bool MediaWebContentsObserver::OnMessageReceived(
const IPC::Message& msg, const IPC::Message& msg,
RenderFrameHost* render_frame_host) { RenderFrameHost* render_frame_host) {
...@@ -380,7 +384,6 @@ void MediaWebContentsObserver::OnPictureInPictureSurfaceChanged( ...@@ -380,7 +384,6 @@ void MediaWebContentsObserver::OnPictureInPictureSurfaceChanged(
const gfx::Size& natural_size, const gfx::Size& natural_size,
bool show_play_pause_button) { bool show_play_pause_button) {
DCHECK(surface_id.is_valid()); DCHECK(surface_id.is_valid());
DCHECK(pip_player_);
pip_player_ = MediaPlayerId(render_frame_host, delegate_id); pip_player_ = MediaPlayerId(render_frame_host, delegate_id);
...@@ -540,7 +543,7 @@ void MediaWebContentsObserver::ExitPictureInPictureInternal() { ...@@ -540,7 +543,7 @@ void MediaWebContentsObserver::ExitPictureInPictureInternal() {
// Reset must happen after notifying the WebContents because it may interact // Reset must happen after notifying the WebContents because it may interact
// with it. // with it.
pip_player_.reset(); ResetPictureInPictureVideoMediaPlayerId();
UpdateVideoLock(); UpdateVideoLock();
} }
......
...@@ -74,6 +74,10 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver { ...@@ -74,6 +74,10 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
const base::Optional<MediaPlayerId>& GetPictureInPictureVideoMediaPlayerId() const base::Optional<MediaPlayerId>& GetPictureInPictureVideoMediaPlayerId()
const; const;
// Reset the MediaPlayerId of the picture in picture video when user closes
// Picture-in-Picture window manually.
void ResetPictureInPictureVideoMediaPlayerId();
// WebContentsObserver implementation. // WebContentsObserver implementation.
void WebContentsDestroyed() override; void WebContentsDestroyed() override;
void RenderFrameDeleted(RenderFrameHost* render_frame_host) override; void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
......
...@@ -129,7 +129,7 @@ OverlayWindow* PictureInPictureWindowControllerImpl::GetWindowForTesting() { ...@@ -129,7 +129,7 @@ OverlayWindow* PictureInPictureWindowControllerImpl::GetWindowForTesting() {
} }
void PictureInPictureWindowControllerImpl::UpdateLayerBounds() { void PictureInPictureWindowControllerImpl::UpdateLayerBounds() {
if (window_ && window_->IsVisible()) { if (media_player_id_.has_value() && window_ && window_->IsVisible()) {
media_web_contents_observer_->OnPictureInPictureWindowResize( media_web_contents_observer_->OnPictureInPictureWindowResize(
window_->GetBounds().size()); window_->GetBounds().size());
} }
...@@ -208,6 +208,7 @@ void PictureInPictureWindowControllerImpl::OnLeavingPictureInPicture( ...@@ -208,6 +208,7 @@ void PictureInPictureWindowControllerImpl::OnLeavingPictureInPicture(
new MediaPlayerDelegateMsg_EndPictureInPictureMode( new MediaPlayerDelegateMsg_EndPictureInPictureMode(
media_player_id_->render_frame_host->GetRoutingID(), media_player_id_->render_frame_host->GetRoutingID(),
media_player_id_->delegate_id)); media_player_id_->delegate_id));
media_web_contents_observer_->ResetPictureInPictureVideoMediaPlayerId();
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment