Picture-in-Picture: use a singleton to manage open windows.

This should enforce one Picture-in-Picture window per Chrome instance.
It should also solve a few edge cases where a WebContents was closed and
the associated PictureInPictureWindowController was still around and
later used.

Note that we may be able to better architecture this. It is a naive
design that simply move the code out of browser.cc.

Bug: 851775
Change-Id: Ib26ffadd1035f58ffa9bcf475070a469361c9e6e
Reviewed-on: https://chromium-review.googlesource.com/1107304
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: apacible <apacible@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569613}

chrome/browser/BUILD.gn

@@ -1029,6 +1029,8 @@ jumbo_split_static_library("browser") {
     "permissions/permission_uma_util.h",
     "permissions/permission_util.cc",
     "permissions/permission_util.h",
+    "picture_in_picture/picture_in_picture_window_manager.cc",
+    "picture_in_picture/picture_in_picture_window_manager.h",
     "platform_util.h",
     "platform_util_chromeos.cc",
     "platform_util_internal.h",

chrome/browser/picture_in_picture/picture_in_picture_window_controller_browsertest.cc

@@ -54,6 +54,29 @@ class PictureInPictureWindowControllerBrowserTest
     return pip_window_controller_;
   }
 
+  void LoadTabAndEnterPictureInPicture(Browser* browser) {
+    GURL test_page_url = ui_test_utils::GetTestUrl(
+        base::FilePath(base::FilePath::kCurrentDirectory),
+        base::FilePath(
+            FILE_PATH_LITERAL("media/picture-in-picture/window-size.html")));
+    ui_test_utils::NavigateToURL(browser, test_page_url);
+
+    content::WebContents* active_web_contents =
+        browser->tab_strip_model()->GetActiveWebContents();
+    ASSERT_NE(nullptr, active_web_contents);
+
+    SetUpWindowController(active_web_contents);
+
+    ASSERT_TRUE(content::ExecuteScript(active_web_contents,
+                                       "enterPictureInPicture();"));
+
+    // Wait for confirmation that the window was created.
+    base::string16 expected_title = base::ASCIIToUTF16("1");
+    EXPECT_EQ(expected_title,
+              content::TitleWatcher(active_web_contents, expected_title)
+                  .WaitAndGetTitle());
+  }
+
  private:
   content::PictureInPictureWindowController* pip_window_controller_ = nullptr;
   base::test::ScopedFeatureList feature_list_;
@@ -660,3 +683,20 @@ IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest,
   EXPECT_EQ(1u, active_web_contents->GetAllFrames().size());
   EXPECT_FALSE(window_controller()->GetWindowForTesting()->IsVisible());
 }
+
+IN_PROC_BROWSER_TEST_F(PictureInPictureWindowControllerBrowserTest,
+                       MultipleBrowserWindowOnePIPWindow) {
+  LoadTabAndEnterPictureInPicture(browser());
+
+  content::PictureInPictureWindowController* first_controller =
+      window_controller();
+  EXPECT_TRUE(first_controller->GetWindowForTesting()->IsVisible());
+
+  Browser* second_browser = CreateBrowser(browser()->profile());
+  LoadTabAndEnterPictureInPicture(second_browser);
+
+  content::PictureInPictureWindowController* second_controller =
+      window_controller();
+  EXPECT_FALSE(first_controller->GetWindowForTesting()->IsVisible());
+  EXPECT_TRUE(second_controller->GetWindowForTesting()->IsVisible());
+}

chrome/browser/picture_in_picture/picture_in_picture_window_manager.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
+
+#include "content/public/browser/picture_in_picture_window_controller.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "ui/gfx/geometry/size.h"
+
+class PictureInPictureWindowManager::WebContentsDestroyedObserver
+    : public content::WebContentsObserver {
+ public:
+  WebContentsDestroyedObserver(PictureInPictureWindowManager* owner,
+                               content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents), owner_(owner) {}
+
+  ~WebContentsDestroyedObserver() final = default;
+
+  void WebContentsDestroyed() final { owner_->CloseWindowInternal(); }
+
+ private:
+  // Owns |this|.
+  PictureInPictureWindowManager* owner_ = nullptr;
+};
+
+PictureInPictureWindowManager* PictureInPictureWindowManager::GetInstance() {
+  return base::Singleton<PictureInPictureWindowManager>::get();
+}
+
+gfx::Size PictureInPictureWindowManager::EnterPictureInPicture(
+    content::WebContents* web_contents,
+    const viz::SurfaceId& surface_id,
+    const gfx::Size& natural_size) {
+  // If there was already a controller, close the existing window before
+  // creating the next one.
+  if (pip_window_controller_)
+    CloseWindowInternal();
+
+  // Create or update |pip_window_controller_| for the current WebContents.
+  if (!pip_window_controller_ ||
+      pip_window_controller_->GetInitiatorWebContents() != web_contents) {
+    CreateWindowInternal(web_contents);
+  }
+
+  pip_window_controller_->EmbedSurface(surface_id, natural_size);
+  return pip_window_controller_->Show();
+}
+
+void PictureInPictureWindowManager::ExitPictureInPicture() {
+  if (pip_window_controller_)
+    CloseWindowInternal();
+}
+
+void PictureInPictureWindowManager::CreateWindowInternal(
+    content::WebContents* web_contents) {
+  destroyed_observer_ =
+      std::make_unique<WebContentsDestroyedObserver>(this, web_contents);
+  pip_window_controller_ =
+      content::PictureInPictureWindowController::GetOrCreateForWebContents(
+          web_contents);
+}
+
+void PictureInPictureWindowManager::CloseWindowInternal() {
+  DCHECK(destroyed_observer_);
+  DCHECK(pip_window_controller_);
+
+  destroyed_observer_.reset();
+  pip_window_controller_->Close(false /* should_pause_video */);
+  pip_window_controller_ = nullptr;
+}
+
+PictureInPictureWindowManager::PictureInPictureWindowManager() = default;
+
+PictureInPictureWindowManager::~PictureInPictureWindowManager() = default;

chrome/browser/picture_in_picture/picture_in_picture_window_manager.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_MANAGER_H_
+#define CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_MANAGER_H_
+
+#include "base/memory/singleton.h"
+
+namespace content {
+class PictureInPictureWindowController;
+class WebContents;
+}  // namespace content
+
+namespace gfx {
+class Size;
+}  // namespace gfx
+
+namespace viz {
+class SurfaceId;
+}  // namespace viz
+
+// PictureInPictureWindowManager is a singleton that handles the lifetime of the
+// current Picture-in-Picture window and its PictureInPictureWindowController.
+// The class also guarantees that only one window will be present per Chrome
+// instances regardless of the number of windows, tabs, profiles, etc.
+class PictureInPictureWindowManager {
+ public:
+  // Returns the singleton instance.
+  static PictureInPictureWindowManager* GetInstance();
+
+  gfx::Size EnterPictureInPicture(content::WebContents*,
+                                  const viz::SurfaceId&,
+                                  const gfx::Size&);
+  void ExitPictureInPicture();
+
+ private:
+  friend struct base::DefaultSingletonTraits<PictureInPictureWindowManager>;
+  class WebContentsDestroyedObserver;
+
+  // Create a Picture-in-Picture window and register it in order to be closed
+  // when needed.
+  // This is suffixed with "Internal" because `CreateWindow` is part of the
+  // Windows API.
+  void CreateWindowInternal(content::WebContents*);
+
+  // Closes the active Picture-in-Picture window.
+  // There MUST be a window open.
+  // This is suffixed with "Internal" to keep consistency with the method above.
+  void CloseWindowInternal();
+
+  PictureInPictureWindowManager();
+  ~PictureInPictureWindowManager();
+
+  std::unique_ptr<WebContentsDestroyedObserver> destroyed_observer_;
+  content::PictureInPictureWindowController* pip_window_controller_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(PictureInPictureWindowManager);
+};
+
+#endif  // CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_MANAGER_H_

chrome/browser/ui/browser.cc

@@ -67,6 +67,7 @@
 #include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 #include "chrome/browser/pepper_broker_infobar_delegate.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
 #include "chrome/browser/plugins/plugin_finder.h"
 #include "chrome/browser/plugins/plugin_metadata.h"
 #include "chrome/browser/policy/developer_tools_policy_handler.h"
@@ -187,7 +188,6 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/overscroll_configuration.h"
-#include "content/public/browser/picture_in_picture_window_controller.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -1009,13 +1009,6 @@ void Browser::TabClosingAt(TabStripModel* tab_strip_model,
   // associated with their tab is still valid.
   WebContentsModalDialogManager::FromWebContents(contents)->CloseAllDialogs();
 
-  if (pip_window_controller_ &&
-      pip_window_controller_->GetInitiatorWebContents() == contents) {
-    // When |contents| is closed, the previously referred to
-    // |pip_window_controller_| will also be torn down.
-    pip_window_controller_ = nullptr;
-  }
-
   // Page load metrics need to be informed that the WebContents will soon be
   // destroyed, so that upcoming visiblity changes can be ignored.
   page_load_metrics::MetricsWebContentsObserver* metrics_observer =
@@ -1417,27 +1410,12 @@ void Browser::OnDidBlockFramebust(content::WebContents* web_contents,
 
 gfx::Size Browser::EnterPictureInPicture(const viz::SurfaceId& surface_id,
                                          const gfx::Size& natural_size) {
-  // If there was already a controller, close the existing window before
-  // creating the next one.
-  if (pip_window_controller_)
-    pip_window_controller_->Close(false /* should_pause_video */);
-
-  // Create or update |pip_window_controller_| for the current WebContents.
-  if (!pip_window_controller_ ||
-      pip_window_controller_->GetInitiatorWebContents() !=
-          tab_strip_model_->GetActiveWebContents()) {
-    pip_window_controller_ =
-        content::PictureInPictureWindowController::GetOrCreateForWebContents(
-            tab_strip_model_->GetActiveWebContents());
-  }
-
-  pip_window_controller_->EmbedSurface(surface_id, natural_size);
-  return pip_window_controller_->Show();
+  return PictureInPictureWindowManager::GetInstance()->EnterPictureInPicture(
+      tab_strip_model_->GetActiveWebContents(), surface_id, natural_size);
 }
 
 void Browser::ExitPictureInPicture() {
-  if (pip_window_controller_)
-    pip_window_controller_->Close(false /* should_pause_video */);
+  PictureInPictureWindowManager::GetInstance()->ExitPictureInPicture();
 }
 
 bool Browser::IsMouseLocked() const {

chrome/browser/ui/browser.h

@@ -79,7 +79,6 @@ class BrowserCommandController;
 }
 
 namespace content {
-class PictureInPictureWindowController;
 class SessionStorageNamespace;
 }
 
@@ -998,13 +997,6 @@ class Browser : public TabStripModelObserver,
 
   std::unique_ptr<chrome::BrowserCommandController> command_controller_;
 
-  // |pip_window_controller_| is held as a SupportsUserData attachment on the
-  // content::WebContents, and thus scoped to the lifetime of the initiator
-  // content::WebContents.
-  // The current active Picture-in-Picture controller is held in case of
-  // updates to the relevant viz::SurfaceId.
-  content::PictureInPictureWindowController* pip_window_controller_ = nullptr;
-
   // True if the browser window has been shown at least once.
   bool window_has_shown_;