Picture-in-Picture: move all logic to the PictureInPictureWindowControllerImpl.

This removes the duality between PictureInPictureServiceImpl and
PictureInPictureWindowControllerImpl that made handling the
PictureInPictureSession object more complicated than it should. Now, the service
is simply a shell passing the requests to the controller which then handles the
current session at the WebContents level.

This CL also introduces a content_browsertest test in order to allow for
integration tests that do not include the //chrome layer. Follow-ups may include
moving some browser_tests to it as they tend to be flaky and they are not
testing the actual UI.

Also adds a check for surface_id being null when the session creation is
requested.

Bug: 1067152, 1069858
FIXES: 1067152, 1069858
Change-Id: I1c389c513cc227e6d0f9d5e447926186600cdc2b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2197996
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#772029}

content/browser/picture_in_picture/picture_in_picture_content_browsertest.cc

+// Copyright 2020 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 "content/browser/picture_in_picture/picture_in_picture_service_impl.h"
+#include "content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/overlay_window.h"
+#include "content/public/common/content_client.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "net/dns/mock_host_resolver.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "third_party/blink/public/mojom/picture_in_picture/picture_in_picture.mojom.h"
+
+using testing::Mock;
+using testing::NiceMock;
+
+namespace content {
+
+namespace {
+
+class TestOverlayWindow : public OverlayWindow {
+ public:
+  TestOverlayWindow() = default;
+  ~TestOverlayWindow() override = default;
+
+  static std::unique_ptr<OverlayWindow> Create(
+      PictureInPictureWindowController* controller) {
+    return std::unique_ptr<OverlayWindow>(new TestOverlayWindow());
+  }
+
+  bool IsActive() override { return false; }
+  void Close() override {}
+  void ShowInactive() override {}
+  void Hide() override {}
+  bool IsVisible() override { return false; }
+  bool IsAlwaysOnTop() override { return false; }
+  gfx::Rect GetBounds() override { return gfx::Rect(size_); }
+  void UpdateVideoSize(const gfx::Size& natural_size) override {
+    size_ = natural_size;
+  }
+  void SetPlaybackState(PlaybackState playback_state) override {}
+  void SetAlwaysHidePlayPauseButton(bool is_visible) override {}
+  void SetSkipAdButtonVisibility(bool is_visible) override {}
+  void SetNextTrackButtonVisibility(bool is_visible) override {}
+  void SetPreviousTrackButtonVisibility(bool is_visible) override {}
+  void SetSurfaceId(const viz::SurfaceId& surface_id) override {}
+  cc::Layer* GetLayerForTesting() override { return nullptr; }
+
+ private:
+  gfx::Size size_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestOverlayWindow);
+};
+
+class TestContentBrowserClient : public ContentBrowserClient {
+ public:
+  TestContentBrowserClient() = default;
+  ~TestContentBrowserClient() override = default;
+
+  std::unique_ptr<OverlayWindow> CreateWindowForPictureInPicture(
+      PictureInPictureWindowController* controller) override {
+    return TestOverlayWindow::Create(controller);
+  }
+};
+
+class TestWebContentsDelegate : public WebContentsDelegate {
+ public:
+  TestWebContentsDelegate() = default;
+  ~TestWebContentsDelegate() override = default;
+
+  PictureInPictureResult EnterPictureInPicture(
+      WebContents* web_contents,
+      const viz::SurfaceId&,
+      const gfx::Size& natural_size) override {
+    return PictureInPictureResult::kSuccess;
+  }
+
+  MOCK_METHOD0(ExitPictureInPicture, void());
+};
+
+class PictureInPictureContentBrowserTest : public ContentBrowserTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ContentBrowserTest::SetUpCommandLine(command_line);
+
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kEnableBlinkFeatures, "PictureInPictureAPI");
+  }
+
+  void SetUpOnMainThread() override {
+    ContentBrowserTest::SetUpOnMainThread();
+
+    host_resolver()->AddRule("*", "127.0.0.1");
+
+    old_browser_client_ = SetBrowserClientForTesting(&content_browser_client_);
+    shell()->web_contents()->SetDelegate(&web_contents_delegate_);
+  }
+
+  void TearDownOnMainThread() override {
+    SetBrowserClientForTesting(old_browser_client_);
+
+    ContentBrowserTest::TearDownOnMainThread();
+  }
+
+ protected:
+  NiceMock<TestWebContentsDelegate> web_contents_delegate_;
+
+ private:
+  ContentBrowserClient* old_browser_client_ = nullptr;
+  TestContentBrowserClient content_browser_client_;
+};
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(PictureInPictureContentBrowserTest,
+                       RequestSecondVideoInSameRFHDoesNotCloseWindow) {
+  EXPECT_CALL(web_contents_delegate_, ExitPictureInPicture()).Times(0);
+
+  EXPECT_TRUE(NavigateToURL(
+      shell(), GetTestUrl("media/picture_in_picture", "two-videos.html")));
+
+  // Play first video.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(), "videos[0].play();"));
+
+  base::string16 expected_title = base::ASCIIToUTF16("videos[0] playing");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Play second video.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(), "videos[1].play();"));
+
+  expected_title = base::ASCIIToUTF16("videos[1] playing");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Send first video in Picture-in-Picture.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(),
+                            "videos[0].requestPictureInPicture();"));
+
+  expected_title = base::ASCIIToUTF16("videos[0] entered picture-in-picture");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Send second video in Picture-in-Picture.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(),
+                            "videos[1].requestPictureInPicture();"));
+
+  expected_title = base::ASCIIToUTF16("videos[1] entered picture-in-picture");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // The session should still be active and ExitPictureInPicture() never called.
+  EXPECT_NE(nullptr, PictureInPictureWindowControllerImpl::FromWebContents(
+                         shell()->web_contents())
+                         ->active_session_for_testing());
+
+  Mock::VerifyAndClearExpectations(&web_contents_delegate_);
+}
+
+IN_PROC_BROWSER_TEST_F(PictureInPictureContentBrowserTest,
+                       RequestSecondVideoInDifferentRFHDoesNotCloseWindow) {
+  EXPECT_CALL(web_contents_delegate_, ExitPictureInPicture()).Times(0);
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  EXPECT_TRUE(NavigateToURL(
+      shell(),
+      embedded_test_server()->GetURL(
+          "example.com", "/media/picture_in_picture/two-videos.html")));
+
+  base::string16 expected_title = base::ASCIIToUTF16("iframe loaded");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Play first video.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(), "videos[0].play();"));
+
+  expected_title = base::ASCIIToUTF16("videos[0] playing");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Play second video (in iframe).
+  ASSERT_TRUE(
+      ExecuteScript(shell()->web_contents(), "iframeVideos[0].play();"));
+
+  expected_title = base::ASCIIToUTF16("iframeVideos[0] playing");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Send first video in Picture-in-Picture.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(),
+                            "videos[0].requestPictureInPicture();"));
+
+  expected_title = base::ASCIIToUTF16("videos[0] entered picture-in-picture");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // Send second video in Picture-in-Picture.
+  ASSERT_TRUE(ExecuteScript(shell()->web_contents(),
+                            "iframeVideos[0].requestPictureInPicture();"));
+
+  expected_title =
+      base::ASCIIToUTF16("iframeVideos[0] entered picture-in-picture");
+  EXPECT_EQ(
+      expected_title,
+      TitleWatcher(shell()->web_contents(), expected_title).WaitAndGetTitle());
+
+  // The session should still be active and ExitPictureInPicture() never called.
+  EXPECT_NE(nullptr, PictureInPictureWindowControllerImpl::FromWebContents(
+                         shell()->web_contents())
+                         ->active_session_for_testing());
+
+  Mock::VerifyAndClearExpectations(&web_contents_delegate_);
+}
+
+}  // namespace content

content/browser/picture_in_picture/picture_in_picture_service_impl.cc

@@ -6,8 +6,9 @@
 
 #include <utility>
 
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/picture_in_picture/picture_in_picture_session.h"
-#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h"
 #include "content/public/browser/web_contents_delegate.h"
 
 namespace content {
@@ -36,30 +37,22 @@ void PictureInPictureServiceImpl::StartSession(
     mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer,
     StartSessionCallback callback) {
   gfx::Size window_size;
-
-  WebContentsImpl* web_contents_impl = static_cast<WebContentsImpl*>(
-      WebContents::FromRenderFrameHost(render_frame_host()));
-
-  auto result = web_contents_impl->EnterPictureInPicture(surface_id.value(),
-                                                         natural_size);
-
   mojo::PendingRemote<blink::mojom::PictureInPictureSession> session_remote;
 
-  // Picture-in-Picture may not be supported by all embedders, so we should only
-  // create the session if the EnterPictureInPicture request was successful.
-  if (result == PictureInPictureResult::kSuccess) {
-    active_session_ = std::make_unique<PictureInPictureSession>(
-        this, MediaPlayerId(render_frame_host_, player_id), surface_id,
-        natural_size, show_play_pause_button,
-        session_remote.InitWithNewPipeAndPassReceiver(), std::move(observer),
-        &window_size);
-
-    // Frames are to be blocklisted from the back-forward cache because the
-    // picture-in-picture continues to be displayed while the page is in the
-    // cache instead of closing.
-    static_cast<RenderFrameHostImpl*>(render_frame_host_)
-        ->OnSchedulerTrackedFeatureUsed(
-            blink::scheduler::WebSchedulerTrackedFeature::kPictureInPicture);
+  if (surface_id.has_value()) {
+    auto result = GetController().StartSession(
+        this, MediaPlayerId(render_frame_host(), player_id), surface_id.value(),
+        natural_size, show_play_pause_button, std::move(observer),
+        &session_remote, &window_size);
+
+    if (result == PictureInPictureResult::kSuccess) {
+      // Frames are to be blocklisted from the back-forward cache because the
+      // picture-in-picture continues to be displayed while the page is in the
+      // cache instead of closing.
+      static_cast<RenderFrameHostImpl*>(render_frame_host())
+          ->OnSchedulerTrackedFeatureUsed(
+              blink::scheduler::WebSchedulerTrackedFeature::kPictureInPicture);
+    }
   }
 
   std::move(callback).Run(std::move(session_remote), window_size);
@@ -68,14 +61,18 @@ void PictureInPictureServiceImpl::StartSession(
 PictureInPictureServiceImpl::PictureInPictureServiceImpl(
     RenderFrameHost* render_frame_host,
     mojo::PendingReceiver<blink::mojom::PictureInPictureService> receiver)
-    : FrameServiceBase(render_frame_host, std::move(receiver)),
-      render_frame_host_(render_frame_host) {}
+    : FrameServiceBase(render_frame_host, std::move(receiver)) {}
 
 PictureInPictureServiceImpl::~PictureInPictureServiceImpl() {
   // If the service is destroyed because the frame was destroyed, the session
   // may still be active and it has to be shutdown before its dtor runs.
-  if (active_session_)
-    active_session_->Shutdown();
+  GetController().OnServiceDeleted(this);
+}
+
+PictureInPictureWindowControllerImpl&
+PictureInPictureServiceImpl::GetController() {
+  return *PictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
+      WebContents::FromRenderFrameHost(render_frame_host()));
 }
 
 }  // namespace content

content/browser/picture_in_picture/picture_in_picture_service_impl.h

@@ -7,7 +7,6 @@
 
 #include <memory>
 
-#include "base/containers/unique_ptr_adapters.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/frame_service_base.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -16,13 +15,16 @@
 
 namespace content {
 
-class PictureInPictureSession;
+class PictureInPictureWindowControllerImpl;
 
 // Receives Picture-in-Picture messages from a given RenderFrame. There is one
-// PictureInPictureServiceImpl per RenderFrameHost. The service gets a hold of
-// a PictureInPictureSession to which it delegates most of the interactions with
-// the rest of the Picture-in-Picture classes such as
-// PictureInPictureWindowController.
+// PictureInPictureServiceImpl per RenderFrameHost. The service pipes the
+// `StartSession()` call to the PictureInPictureWindowControllerImpl which owns
+// the created session. The same object will get notified when the service is
+// killed given that the PictureInPictureWindowControllerImpl is
+// WebContents-bound instead of RenderFrameHost.
+// PictureInPictureServiceImpl owns itself. It self-destruct as needed, see the
+// FrameServiceBase's documentation for more information.
 class CONTENT_EXPORT PictureInPictureServiceImpl final
     : public content::FrameServiceBase<blink::mojom::PictureInPictureService> {
  public:
@@ -43,10 +45,6 @@ class CONTENT_EXPORT PictureInPictureServiceImpl final
       mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>,
       StartSessionCallback) final;
 
-  PictureInPictureSession* active_session_for_testing() const {
-    return active_session_.get();
-  }
-
  private:
   friend class PictureInPictureSession;
 
@@ -55,9 +53,7 @@ class CONTENT_EXPORT PictureInPictureServiceImpl final
       mojo::PendingReceiver<blink::mojom::PictureInPictureService>);
   ~PictureInPictureServiceImpl() override;
 
-  RenderFrameHost* render_frame_host_ = nullptr;
-
-  std::unique_ptr<PictureInPictureSession> active_session_;
+  PictureInPictureWindowControllerImpl& GetController();
 
   DISALLOW_COPY_AND_ASSIGN(PictureInPictureServiceImpl);
 };

content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/test/bind_test_util.h"
 #include "build/build_config.h"
+#include "content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h"
 #include "content/common/media/media_player_delegate_messages.h"
 #include "content/public/browser/overlay_window.h"
 #include "content/public/browser/web_contents_delegate.h"
@@ -22,6 +23,8 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+using testing::_;
+
 namespace content {
 
 class DummyPictureInPictureSessionObserver
@@ -137,6 +140,11 @@ class PictureInPictureServiceImplTest : public RenderViewHostImplTestHarness {
 
 TEST_F(PictureInPictureServiceImplTest, MAYBE_EnterPictureInPicture) {
   const int kPlayerVideoOnlyId = 30;
+  const PictureInPictureWindowControllerImpl* controller =
+      PictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
+          contents());
+
+  ASSERT_TRUE(controller);
 
   DummyPictureInPictureSessionObserver observer;
   mojo::Receiver<blink::mojom::PictureInPictureSessionObserver>
@@ -146,7 +154,7 @@ TEST_F(PictureInPictureServiceImplTest, MAYBE_EnterPictureInPicture) {
   observer_receiver.Bind(observer_remote.InitWithNewPipeAndPassReceiver());
 
   // If Picture-in-Picture there shouldn't be an active session.
-  EXPECT_FALSE(service().active_session_for_testing());
+  EXPECT_FALSE(controller->active_session_for_testing());
 
   viz::SurfaceId surface_id =
       viz::SurfaceId(viz::FrameSinkId(1, 1),
@@ -171,7 +179,7 @@ TEST_F(PictureInPictureServiceImplTest, MAYBE_EnterPictureInPicture) {
             window_size = b;
           }));
 
-  EXPECT_TRUE(service().active_session_for_testing());
+  EXPECT_TRUE(controller->active_session_for_testing());
   EXPECT_TRUE(session_remote);
   EXPECT_EQ(gfx::Size(42, 42), window_size);
 
@@ -181,15 +189,20 @@ TEST_F(PictureInPictureServiceImplTest, MAYBE_EnterPictureInPicture) {
   contents()->GetMainFrame()->OnMessageReceived(
       MediaPlayerDelegateHostMsg_OnMediaDestroyed(
           contents()->GetMainFrame()->GetRoutingID(), kPlayerVideoOnlyId));
-  EXPECT_TRUE(service().active_session_for_testing());
+  EXPECT_TRUE(controller->active_session_for_testing());
 }
 
 TEST_F(PictureInPictureServiceImplTest, EnterPictureInPicture_NotSupported) {
   const int kPlayerVideoOnlyId = 30;
+  const PictureInPictureWindowControllerImpl* controller =
+      PictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
+          contents());
+
+  ASSERT_TRUE(controller);
+  EXPECT_FALSE(controller->active_session_for_testing());
+
   mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>
       observer_remote;
-  EXPECT_FALSE(service().active_session_for_testing());
-
   viz::SurfaceId surface_id =
       viz::SurfaceId(viz::FrameSinkId(1, 1),
                      viz::LocalSurfaceId(
@@ -213,10 +226,52 @@ TEST_F(PictureInPictureServiceImplTest, EnterPictureInPicture_NotSupported) {
             window_size = b;
           }));
 
-  EXPECT_FALSE(service().active_session_for_testing());
-  // The |session_remote| won't be bound because the |pending_remote| received
-  // in the StartSessionCallback will be invalid due to PictureInPictureSession
-  // not ever being created (meaning the the receiver won't be bound either).
+  EXPECT_FALSE(controller->active_session_for_testing());
+
+  // The |session_remote| won't be bound because the |remote| received in the
+  // StartSessionCallback will be invalid due to PictureInPictureSession not
+  // ever being created (meaning the the receiver won't be bound either).
+  EXPECT_FALSE(session_remote);
+  EXPECT_EQ(gfx::Size(), window_size);
+}
+
+// The |surface_id| is an optional parameter in the StartSession() call but
+// needs to be non-null in order to create a session at the moment. The creation
+// will early return if that condition isn't satisfied, failing to create the
+// session.
+TEST_F(PictureInPictureServiceImplTest, EnterPictureInPicture_NoSurfaceId) {
+  const int kPlayerVideoOnlyId = 30;
+  const PictureInPictureWindowControllerImpl* controller =
+      PictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
+          contents());
+
+  ASSERT_TRUE(controller);
+  EXPECT_FALSE(controller->active_session_for_testing());
+
+  mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>
+      observer_remote;
+
+  EXPECT_CALL(delegate(), EnterPictureInPicture(_, _, _)).Times(0);
+
+  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote;
+  gfx::Size window_size;
+
+  service().StartSession(
+      kPlayerVideoOnlyId, base::nullopt, gfx::Size(42, 42),
+      true /* show_play_pause_button */, std::move(observer_remote),
+      base::BindLambdaForTesting(
+          [&](mojo::PendingRemote<blink::mojom::PictureInPictureSession> remote,
+              const gfx::Size& b) {
+            if (remote.is_valid())
+              session_remote.Bind(std::move(remote));
+            window_size = b;
+          }));
+
+  EXPECT_FALSE(controller->active_session_for_testing());
+
+  // The |session_remote| won't be bound because the |remote| received in the
+  // StartSessionCallback will be invalid due to PictureInPictureSession not
+  // ever being created (meaning the the receiver won't be bound either).
   EXPECT_FALSE(session_remote);
   EXPECT_EQ(gfx::Size(), window_size);
 }

content/browser/picture_in_picture/picture_in_picture_session.cc

@@ -16,25 +16,14 @@ namespace content {
 PictureInPictureSession::PictureInPictureSession(
     PictureInPictureServiceImpl* service,
     const MediaPlayerId& player_id,
-    const base::Optional<viz::SurfaceId>& surface_id,
-    const gfx::Size& natural_size,
-    bool show_play_pause_button,
     mojo::PendingReceiver<blink::mojom::PictureInPictureSession> receiver,
-    mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer,
-    gfx::Size* window_size)
+    mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer)
     : service_(service),
       receiver_(this, std::move(receiver)),
       player_id_(player_id),
       observer_(std::move(observer)) {
   receiver_.set_disconnect_handler(base::BindOnce(
       &PictureInPictureSession::OnConnectionError, base::Unretained(this)));
-
-  GetController().SetActiveSession(this);
-  GetController().EmbedSurface(surface_id.value(), natural_size);
-  GetController().SetAlwaysHidePlayPauseButton(show_play_pause_button);
-  GetController().Show();
-
-  *window_size = GetController().GetSize();
 }
 
 PictureInPictureSession::~PictureInPictureSession() {
@@ -50,17 +39,28 @@ void PictureInPictureSession::Update(
     const base::Optional<viz::SurfaceId>& surface_id,
     const gfx::Size& natural_size,
     bool show_play_pause_button) {
-  player_id_ = MediaPlayerId(service_->render_frame_host_, player_id);
+  player_id_ = MediaPlayerId(service_->render_frame_host(), player_id);
 
   GetController().EmbedSurface(surface_id.value(), natural_size);
   GetController().SetAlwaysHidePlayPauseButton(show_play_pause_button);
-  GetController().SetActiveSession(this);
 }
 
 void PictureInPictureSession::NotifyWindowResized(const gfx::Size& size) {
   observer_->OnWindowSizeChanged(size);
 }
 
+void PictureInPictureSession::Disconnect() {
+  // |is_stopping_| shouldn't be true for the implementation in //chrome but if
+  // the WebContentsDelegate's Picture-in-Picture calls are empty, it's possible
+  // for `Disconnect()` to be called even after `StopInternal()` as the
+  // expectation of self-destruction will no longer be true.
+  if (is_stopping_)
+    return;
+
+  is_stopping_ = true;
+  observer_->OnStopped();
+}
+
 void PictureInPictureSession::Shutdown() {
   if (is_stopping_)
     return;
@@ -73,8 +73,6 @@ void PictureInPictureSession::StopInternal(StopCallback callback) {
 
   is_stopping_ = true;
 
-  GetWebContentsImpl()->ExitPictureInPicture();
-
   // `OnStopped()` should only be called if there is no callback to run, as a
   // contract in the API.
   if (callback)
@@ -82,10 +80,8 @@ void PictureInPictureSession::StopInternal(StopCallback callback) {
   else
     observer_->OnStopped();
 
-  GetController().SetActiveSession(nullptr);
-
-  // Reset must happen after everything is done as it will destroy |this|.
-  service_->active_session_.reset();
+  // |this| will be deleted after this call.
+  GetWebContentsImpl()->ExitPictureInPicture();
 }
 
 void PictureInPictureSession::OnConnectionError() {

content/browser/picture_in_picture/picture_in_picture_session.h

@@ -20,9 +20,10 @@ class WebContentsImpl;
 
 // The PicutreInPictureSession communicates with the
 // PictureInPictureWindowController and the WebContents. It is created by the
-// PictureInPictureService but deletes itself. When created, the session will
-// enter Picture-in-Picture and when deleted, it will automatically exit
-// Picture-in-Picture unless another session became active.
+// PictureInPictureWindowControllerImpl which also deletes it. When created, the
+// session will be expected to be active (in Picture-in-Picture) and when
+// deleted, it will automatically exit Picture-in-Picture unless another session
+// became active.
 // The session MUST be stopped before its dtor runs to avoid unexpected
 // deletion.
 class PictureInPictureSession : public blink::mojom::PictureInPictureSession {
@@ -30,13 +31,9 @@ class PictureInPictureSession : public blink::mojom::PictureInPictureSession {
   PictureInPictureSession(
       PictureInPictureServiceImpl* service,
       const MediaPlayerId& player_id,
-      const base::Optional<viz::SurfaceId>& surface_id,
-      const gfx::Size& natural_size,
-      bool show_play_pause_button,
       mojo::PendingReceiver<blink::mojom::PictureInPictureSession> receiver,
       mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>
-          observer,
-      gfx::Size* window_size);
+          observer);
   ~PictureInPictureSession() override;
 
   // blink::mojom::PictureInPictureSession interface.
@@ -52,10 +49,19 @@ class PictureInPictureSession : public blink::mojom::PictureInPictureSession {
   // if there are none.
   const base::Optional<MediaPlayerId>& player_id() const { return player_id_; }
 
+  // Stops the session without closing the window. It will prevent the session
+  // to later trying to shutdown when the PictureInPictureWindowController is
+  // notified.
+  void Disconnect();
+
   // Shuts down the session. Called by the window controller when the window is
   // closed.
   void Shutdown();
 
+  // Returns the PictureInPictureServiceImpl instance associated with this
+  // session. It cannot be null.
+  PictureInPictureServiceImpl* service() { return service_; }
+
  private:
   PictureInPictureSession() = delete;
 
@@ -74,7 +80,9 @@ class PictureInPictureSession : public blink::mojom::PictureInPictureSession {
   // session.
   PictureInPictureWindowControllerImpl& GetController();
 
-  // Owns |this|.
+  // Will notified The PictureInPictureWindowControllerImpl who owns |this| when
+  // it gets destroyed in order for |this| to be destroyed too. Indirectly owns
+  // |this|.
   PictureInPictureServiceImpl* service_;
 
   mojo::Receiver<blink::mojom::PictureInPictureSession> receiver_;

content/browser/picture_in_picture/picture_in_picture_window_controller_impl.cc

@@ -5,6 +5,7 @@
 #include "content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h"
 
 #include <set>
+#include <utility>
 
 #include "components/viz/common/surfaces/surface_id.h"
 #include "content/browser/media/media_web_contents_observer.h"
@@ -15,6 +16,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/overlay_window.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"  // for PictureInPictureResult
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_client.h"
 
@@ -207,15 +209,44 @@ void PictureInPictureWindowControllerImpl::UpdateMediaPlayerId() {
   UpdatePlaybackState(IsPlayerActive(), !media_player_id_.has_value());
 }
 
-void PictureInPictureWindowControllerImpl::SetActiveSession(
-    PictureInPictureSession* session) {
-  if (active_session_ == session)
-    return;
+PictureInPictureResult PictureInPictureWindowControllerImpl::StartSession(
+    PictureInPictureServiceImpl* service,
+    const MediaPlayerId& player_id,
+    const viz::SurfaceId& surface_id,
+    const gfx::Size& natural_size,
+    bool show_play_pause_button,
+    mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer,
+    mojo::PendingRemote<blink::mojom::PictureInPictureSession>* session_remote,
+    gfx::Size* window_size) {
+  auto result = initiator_->EnterPictureInPicture(surface_id, natural_size);
+
+  // Picture-in-Picture may not be supported by all embedders, so we should only
+  // create the session if the EnterPictureInPicture request was successful.
+  if (result != PictureInPictureResult::kSuccess)
+    return result;
 
   if (active_session_)
-    active_session_->Shutdown();
+    active_session_->Disconnect();
+
+  active_session_ = std::make_unique<PictureInPictureSession>(
+      service, player_id, session_remote->InitWithNewPipeAndPassReceiver(),
+      std::move(observer));
 
-  active_session_ = session;
+  EmbedSurface(surface_id, natural_size);
+  SetAlwaysHidePlayPauseButton(show_play_pause_button);
+  Show();
+
+  *window_size = GetSize();
+  return result;
+}
+
+void PictureInPictureWindowControllerImpl::OnServiceDeleted(
+    PictureInPictureServiceImpl* service) {
+  if (!active_session_ || active_session_->service() != service)
+    return;
+
+  active_session_->Shutdown();
+  active_session_ = nullptr;
 }
 
 void PictureInPictureWindowControllerImpl::SetAlwaysHidePlayPauseButton(
@@ -314,10 +345,11 @@ void PictureInPictureWindowControllerImpl::OnLeavingPictureInPicture(
   }
 
   if (media_player_id_.has_value()) {
-    if (active_session_)
+    if (active_session_) {
       active_session_->Shutdown();
+      active_session_ = nullptr;
+    }
 
-    active_session_ = nullptr;
     media_player_id_.reset();
   }
 }

content/browser/picture_in_picture/picture_in_picture_window_controller_impl.h

@@ -10,26 +10,37 @@
 
 #include "base/memory/weak_ptr.h"
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
+#include "content/common/content_export.h"
 #include "content/public/browser/media_player_id.h"
 #include "content/public/browser/picture_in_picture_window_controller.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 
+#include "third_party/blink/public/mojom/picture_in_picture/picture_in_picture.mojom.h"
+
 namespace content {
 
 class MediaWebContentsObserver;
+class PictureInPictureServiceImpl;
 class PictureInPictureSession;
 class WebContents;
 class WebContentsImpl;
-
-// TODO(thakis,mlamouri): PictureInPictureWindowControllerImpl isn't
-// CONTENT_EXPORT'd because it creates complicated build issues with
-// WebContentsUserData being a non-exported template. As a result, the class
-// uses CONTENT_EXPORT for methods that are being used from tests.
-// CONTENT_EXPORT should be moved back to the class when the Windows build will
-// work with it. https://crbug.com/589840.
-class PictureInPictureWindowControllerImpl
+enum class PictureInPictureResult;
+
+// PictureInPictureWindowControllerImpl is the corner stone of the
+// Picture-in-Picture feature in the //content layer. It handles the session
+// creation requests (sent by the PictureInPictureServiceImpl), owns the session
+// object and therefore handles its lifetime, and communicate with the rest of
+// the browser. Requests to the WebContents are sent by the controller and it
+// gets notified when the browser needs it to update the Picture-in-Picture
+// session.
+// The PictureInPictureWindowControllerImpl is managing Picture-in-Picture at a
+// WebContents level. If multiple calls request a Picture-in-Picture session
+// either in the same frame or in different frames, the controller will handle
+// creating the new session, stopping the current one and making sure the window
+// is kept around when possible.
+class CONTENT_EXPORT PictureInPictureWindowControllerImpl
     : public PictureInPictureWindowController,
       public WebContentsUserData<PictureInPictureWindowControllerImpl>,
       public WebContentsObserver {
@@ -37,31 +48,31 @@ class PictureInPictureWindowControllerImpl
   // Gets a reference to the controller associated with |initiator| and creates
   // one if it does not exist. The returned pointer is guaranteed to be
   // non-null.
-  CONTENT_EXPORT static PictureInPictureWindowControllerImpl*
-  GetOrCreateForWebContents(WebContents* initiator);
+  static PictureInPictureWindowControllerImpl* GetOrCreateForWebContents(
+      WebContents* initiator);
 
   ~PictureInPictureWindowControllerImpl() override;
 
   using PlayerSet = std::set<int>;
 
   // PictureInPictureWindowController:
-  CONTENT_EXPORT void Show() override;
-  CONTENT_EXPORT void Close(bool should_pause_video) override;
-  CONTENT_EXPORT void CloseAndFocusInitiator() override;
-  CONTENT_EXPORT void OnWindowDestroyed() 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;
-  CONTENT_EXPORT void UpdatePlaybackState(bool is_playing,
-                                          bool reached_end_of_stream) override;
-  CONTENT_EXPORT void SetAlwaysHidePlayPauseButton(bool is_visible) override;
-  CONTENT_EXPORT void SkipAd() override;
-  CONTENT_EXPORT void NextTrack() override;
-  CONTENT_EXPORT void PreviousTrack() override;
-
-  CONTENT_EXPORT void MediaSessionActionsChanged(
+  void Show() override;
+  void Close(bool should_pause_video) override;
+  void CloseAndFocusInitiator() override;
+  void OnWindowDestroyed() override;
+  OverlayWindow* GetWindowForTesting() override;
+  void UpdateLayerBounds() override;
+  bool IsPlayerActive() override;
+  WebContents* GetInitiatorWebContents() override;
+  bool TogglePlayPause() override;
+  void UpdatePlaybackState(bool is_playing,
+                           bool reached_end_of_stream) override;
+  void SetAlwaysHidePlayPauseButton(bool is_visible) override;
+  void SkipAd() override;
+  void NextTrack() override;
+  void PreviousTrack() override;
+
+  void MediaSessionActionsChanged(
       const std::set<media_session::mojom::MediaSessionAction>& actions);
 
   gfx::Size GetSize();
@@ -82,19 +93,37 @@ class PictureInPictureWindowControllerImpl
   void EmbedSurface(const viz::SurfaceId& surface_id,
                     const gfx::Size& natural_size);
 
-  // Sets the active Picture-in-Picture session associated with the controller.
-  // This is different from the service's active session as there is one
-  // controller per WebContents and one service per RenderFrameHost.
-  // The current session may be shut down as a side effect of this.
-  void SetActiveSession(PictureInPictureSession* session);
+  // Called by PictureInPictureServiceImpl when a session request is received.
+  // The call should return the |session_remote| and |window_size| as out
+  // params. A failure to create the session should be expressed with an empty
+  // |window_size| and uninitialized |session_remote|.
+  // Returns whether the session creation was successful.
+  PictureInPictureResult StartSession(
+      PictureInPictureServiceImpl* service,
+      const MediaPlayerId&,
+      const viz::SurfaceId& surface_id,
+      const gfx::Size& natural_size,
+      bool show_play_pause_button,
+      mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>,
+      mojo::PendingRemote<blink::mojom::PictureInPictureSession>*
+          session_remote,
+      gfx::Size* window_size);
+
+  // Called by PictureInPictureServiceImpl when the service is about to be
+  // destroyed. It allows |this| to close the |active_session_| if it is
+  // associated with the service.
+  void OnServiceDeleted(PictureInPictureServiceImpl* service);
+
+  PictureInPictureSession* active_session_for_testing() const {
+    return active_session_.get();
+  }
 
  private:
   friend class WebContentsUserData<PictureInPictureWindowControllerImpl>;
 
   // Use PictureInPictureWindowControllerImpl::GetOrCreateForWebContents() to
   // create an instance.
-  CONTENT_EXPORT explicit PictureInPictureWindowControllerImpl(
-      WebContents* initiator);
+  explicit PictureInPictureWindowControllerImpl(WebContents* initiator);
 
   // Signal to the media player that |this| is leaving Picture-in-Picture mode.
   void OnLeavingPictureInPicture(bool should_pause_video);
@@ -141,7 +170,7 @@ class PictureInPictureWindowControllerImpl
   // session object makes the bridge with the renderer process by handling
   // requests and holding states such as the active player id.
   // The session will be nullptr when there is no active session.
-  PictureInPictureSession* active_session_ = nullptr;
+  std::unique_ptr<PictureInPictureSession> active_session_;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

content/test/BUILD.gn

@@ -1019,6 +1019,7 @@ test("content_browsertests") {
     "../browser/origin_trials/origin_trials_browsertest.cc",
     "../browser/payments/payment_app_browsertest.cc",
     "../browser/performance_memory_browsertest.cc",
+    "../browser/picture_in_picture/picture_in_picture_content_browsertest.cc",
     "../browser/pointer_lock_browsertest.cc",
     "../browser/pointer_lock_browsertest.h",
     "../browser/pointer_lock_browsertest_mac.mm",

content/test/data/media/picture_in_picture/two-videos.html

+<!DOCTYPE html>
+<html>
+<body>
+<video src="../tulip2.webm"></video>
+<video src="../tulip2.webm"></video>
+<iframe src="data:text/html,<video src='../tulip2.webm'></video>"></iframe>
+</body>
+<script>
+  const videos = document.querySelectorAll('video');
+  videos[0].addEventListener('playing', e => {
+    document.title = 'videos[0] playing';
+  });
+  videos[0].addEventListener('enterpictureinpicture', e => {
+    document.title = 'videos[0] entered picture-in-picture';
+  });
+  videos[1].addEventListener('playing', e => {
+    document.title = 'videos[1] playing';
+  });
+  videos[1].addEventListener('enterpictureinpicture', e => {
+    document.title = 'videos[1] entered picture-in-picture';
+  });
+
+  let iframeVideos = [];
+  if (window.top == window.self) {
+    const iframe = document.createElement('iframe');
+    iframe.src = 'two-videos.html';
+    document.body.appendChild(iframe);
+    iframe.addEventListener('load', () => {
+      document.title = 'iframe loaded';
+
+      iframeVideos = iframe.contentDocument.querySelectorAll('video');
+      iframeVideos[0].addEventListener('playing', e => {
+        document.title = 'iframeVideos[0] playing';
+      });
+      iframeVideos[0].addEventListener('enterpictureinpicture', e => {
+        document.title = 'iframeVideos[0] entered picture-in-picture';
+      });
+    });
+  }
+</script>
+</html>