From 05cac719625ffb9eff9df13a0d85018b56b8a2e3 Mon Sep 17 00:00:00 2001 From: Mounir Lamouri Date: Wed, 25 Jul 2018 19:00:51 +0000 Subject: [PATCH] Picture-in-Picture: notifies the controller when window closed via system. When the internal object is destroyed, notify the controller so it can reset its internal state. It allows the user to close the window via any system/window manager UI. Bug: 863842 Change-Id: Ica84f4ffee2578658a60e0ae708a4e327d293077 Reviewed-on: https://chromium-review.googlesource.com/1147252 Reviewed-by: Camille Lamy Reviewed-by: apacible Commit-Queue: Mounir Lamouri Cr-Original-Commit-Position: refs/heads/master@{#577594}(cherry picked from commit c04a9a27665700d3697b59d74964bfae1aafe7f3) Reviewed-on: https://chromium-review.googlesource.com/1150580 Reviewed-by: Mounir Lamouri Cr-Commit-Position: refs/branch-heads/3497@{#83} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} --- ...n_picture_window_controller_browsertest.cc | 23 ++++++++++++ .../ui/views/overlay/overlay_window_views.cc | 4 +++ .../ui/views/overlay/overlay_window_views.h | 1 + ...cture_in_picture_window_controller_impl.cc | 36 ++++++++++++++----- ...icture_in_picture_window_controller_impl.h | 9 +++++ .../picture_in_picture_window_controller.h | 7 ++++ 6 files changed, 71 insertions(+), 9 deletions(-) diff --git a/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc b/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc index 8b465231ba6a..5ad7d13e7f6e 100644 --- a/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc +++ b/chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc @@ -916,4 +916,27 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest, #endif // defined(OS_LINUX) && !defined(OS_CHROMEOS) +// Tests that the Picture-in-Picture state is properly updated when the window +// is closed at a system level. +IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest, + CloseWindowNotifiesController) { + LoadTabAndEnterPictureInPicture(browser()); + + content::WebContents* active_web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + OverlayWindowViews* overlay_window = static_cast( + window_controller()->GetWindowForTesting()); + ASSERT_TRUE(overlay_window); + ASSERT_TRUE(overlay_window->IsVisible()); + + // Simulate closing from the system. + overlay_window->OnNativeWidgetDestroyed(); + + bool in_picture_in_picture = false; + ASSERT_TRUE(ExecuteScriptAndExtractBool( + active_web_contents, "isInPictureInPicture();", &in_picture_in_picture)); + EXPECT_FALSE(in_picture_in_picture); +} + #endif // !defined(OS_ANDROID) diff --git a/chrome/browser/ui/views/overlay/overlay_window_views.cc b/chrome/browser/ui/views/overlay/overlay_window_views.cc index 234a8f75c79d..b45ad4d2c84d 100644 --- a/chrome/browser/ui/views/overlay/overlay_window_views.cc +++ b/chrome/browser/ui/views/overlay/overlay_window_views.cc @@ -588,6 +588,10 @@ void OverlayWindowViews::OnNativeWidgetSizeChanged(const gfx::Size& new_size) { views::Widget::OnNativeWidgetSizeChanged(new_size); } +void OverlayWindowViews::OnNativeWidgetDestroyed() { + controller_->OnWindowDestroyed(); +} + void OverlayWindowViews::TogglePlayPause() { // Retrieve expected active state based on what command was sent in // TogglePlayPause() since the IPC message may not have been propogated diff --git a/chrome/browser/ui/views/overlay/overlay_window_views.h b/chrome/browser/ui/views/overlay/overlay_window_views.h index 54b1e1ece0c5..56e1d46bbba1 100644 --- a/chrome/browser/ui/views/overlay/overlay_window_views.h +++ b/chrome/browser/ui/views/overlay/overlay_window_views.h @@ -50,6 +50,7 @@ class OverlayWindowViews : public content::OverlayWindow, public views::Widget { void OnNativeBlur() override; void OnNativeWidgetMove() override; void OnNativeWidgetSizeChanged(const gfx::Size& new_size) override; + void OnNativeWidgetDestroyed() override; // Gets the bounds of the controls. gfx::Rect GetCloseControlsBounds(); diff --git a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc b/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc index eb9c84630b43..bb7180564ebe 100644 --- a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc +++ b/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc @@ -58,8 +58,7 @@ PictureInPictureWindowControllerImpl::PictureInPictureWindowControllerImpl( media_web_contents_observer_ = initiator_->media_web_contents_observer(); - window_ = - GetContentClient()->browser()->CreateWindowForPictureInPicture(this); + EnsureWindow(); DCHECK(window_) << "Picture in Picture requires a valid window."; } @@ -84,23 +83,25 @@ void PictureInPictureWindowControllerImpl::ClickCustomControl( } void PictureInPictureWindowControllerImpl::Close(bool should_pause_video) { - DCHECK(window_); - - if (!window_->IsVisible()) + if (!window_ || !window_->IsVisible()) return; window_->Hide(); - initiator_->SetHasPictureInPictureVideo(false); - - surface_id_ = viz::SurfaceId(); + CloseInternal(should_pause_video); +} - OnLeavingPictureInPicture(should_pause_video); +void PictureInPictureWindowControllerImpl::OnWindowDestroyed() { + window_ = nullptr; + embedder_ = nullptr; + CloseInternal(true /* should_pause_video */); } void PictureInPictureWindowControllerImpl::EmbedSurface( const viz::SurfaceId& surface_id, const gfx::Size& natural_size) { + EnsureWindow(); DCHECK(window_); + DCHECK(surface_id.is_valid()); surface_id_ = surface_id; @@ -195,4 +196,21 @@ void PictureInPictureWindowControllerImpl::OnLeavingPictureInPicture( } } +void PictureInPictureWindowControllerImpl::CloseInternal( + bool should_pause_video) { + initiator_->SetHasPictureInPictureVideo(false); + + surface_id_ = viz::SurfaceId(); + + OnLeavingPictureInPicture(should_pause_video); +} + +void PictureInPictureWindowControllerImpl::EnsureWindow() { + if (window_) + return; + + window_ = + GetContentClient()->browser()->CreateWindowForPictureInPicture(this); +} + } // namespace content diff --git a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h b/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h index 8cd191fef301..7ce6d853fc42 100644 --- a/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h +++ b/content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h @@ -36,6 +36,7 @@ class PictureInPictureWindowControllerImpl // PictureInPictureWindowController: CONTENT_EXPORT gfx::Size Show() override; CONTENT_EXPORT void Close(bool should_pause_video) override; + CONTENT_EXPORT void OnWindowDestroyed() override; CONTENT_EXPORT void ClickCustomControl( const std::string& control_id) override; CONTENT_EXPORT void EmbedSurface(const viz::SurfaceId& surface_id, @@ -59,6 +60,14 @@ class PictureInPictureWindowControllerImpl // Signal to the media player that |this| is leaving Picture-in-Picture mode. void OnLeavingPictureInPicture(bool should_pause_video); + // Internal method to set the states after the window was closed, whether via + // the system or Chromium. + void CloseInternal(bool should_pause_video); + + // Creates a new window if the previous one was destroyed. It can happen + // because of the system control of the window. + void EnsureWindow(); + std::unique_ptr window_; std::unique_ptr embedder_; WebContentsImpl* const initiator_; diff --git a/content/public/browser/picture_in_picture_window_controller.h b/content/public/browser/picture_in_picture_window_controller.h index 64f0104388ba..6df6f227d863 100644 --- a/content/public/browser/picture_in_picture_window_controller.h +++ b/content/public/browser/picture_in_picture_window_controller.h @@ -38,7 +38,14 @@ class PictureInPictureWindowController { // Returns the size of the window in pixels. virtual gfx::Size Show() = 0; + // Called to notify the controller that the window was requested to be closed + // by the user or the content. virtual void Close(bool should_pause_video) = 0; + + // Called by the window implementation to notify the controller that the + // window was requested to be closed and destroyed by the system. + virtual void OnWindowDestroyed() = 0; + virtual void ClickCustomControl(const std::string& control_id) = 0; virtual void EmbedSurface(const viz::SurfaceId& surface_id, const gfx::Size& natural_size) = 0;