From ecdf6e4cdc1049924bc03afe66af0077bba3cc6c Mon Sep 17 00:00:00 2001
From: Oleh Desiatyrikov <[email protected]>
Date: Tue, 09 Jun 2026 10:01:29 -0700
Subject: [PATCH] Reland "[ImmersivePlayback] Simplify immersive Picture-in-Picture confirmation flow"

This is a reland of commit e472fe98af9472b48586022b7877e1c6579f0340

Original change's description:
> [ImmersivePlayback] Simplify immersive Picture-in-Picture confirmation flow
>
> Merges the separate RequestImmersivePlaybackConfirmation Mojo request
> directly into StartSession on the browser side.
>
> - Mojo changes: StartSession now receives a `bool request_immersive` flag.
> - State Machine: PictureInPictureServiceImpl implements a single-active-request confirmation queue. New/subsequent sessions immediately preempt and cancel any pending confirmation.
> - Fullscreen bugfix: Prevent exiting fullscreen when entering immersive Picture-in-Picture, as native fullscreen must remain active to display the dialog.
>
> Bug: 514069440
> Fixed: 514069440
> Change-Id: I694fb6b2d7a411b567ceb215f875e84f66b59dd5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7868111
> Reviewed-by: Giovanni Ortuno Urquidi <[email protected]>
> Reviewed-by: David Bokan <[email protected]>
> Reviewed-by: Frank Liberato <[email protected]>
> Commit-Queue: Oleh Desiatyrikov (xWF) <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1640570}

Bug: 514069440
Change-Id: I842e716a61c7a1371a63830f672f9edbb69b6e68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7899097
Reviewed-by: Frank Liberato <[email protected]>
Commit-Queue: Oleh Desiatyrikov (xWF) <[email protected]>
Reviewed-by: Giovanni Ortuno Urquidi <[email protected]>
Reviewed-by: David Bokan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1644066}
---

diff --git a/content/browser/picture_in_picture/picture_in_picture_service_impl.cc b/content/browser/picture_in_picture/picture_in_picture_service_impl.cc
index 232f4b6..f8ad3ec 100644
--- a/content/browser/picture_in_picture/picture_in_picture_service_impl.cc
+++ b/content/browser/picture_in_picture/picture_in_picture_service_impl.cc
@@ -6,10 +6,13 @@
 
 #include <utility>
 
+#include "base/functional/bind.h"
 #include "content/browser/picture_in_picture/picture_in_picture_session.h"
 #include "content/browser/picture_in_picture/video_picture_in_picture_window_controller_impl.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
 
 namespace content {
 
@@ -38,15 +41,42 @@
     bool show_play_pause_button,
     mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer,
     const gfx::Rect& source_bounds,
-    blink::mojom::ImmersiveOptionsPtr immersive_options,
+    bool request_immersive,
     StartSessionCallback callback) {
+  // Invalidate any pending immersive confirmation flows. Note that this does
+  // not immediately destroy the `PendingSession` stored in the pending
+  // confirmation callback, as the callback object is owned by the controller
+  // and will be destroyed when the controller releases or runs the callback.
+  immersive_confirmation_weak_factory_.InvalidateWeakPtrs();
+
+  auto pending_session = std::make_unique<PendingSession>(
+      player_id, std::move(player_remote), surface_id, natural_size,
+      show_play_pause_button, std::move(observer), source_bounds,
+      mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+          std::move(callback), mojo::NullRemote(), gfx::Size()));
+
+  if (!request_immersive) {
+    StartSessionInternal(std::move(pending_session),
+                         /*immersive_options=*/nullptr);
+  } else {
+    StartSessionImmersive(std::move(pending_session));
+  }
+}
+
+void PictureInPictureServiceImpl::StartSessionInternal(
+    std::unique_ptr<PictureInPictureServiceImpl::PendingSession>
+        pending_session,
+    blink::mojom::ImmersiveOptionsPtr immersive_options) {
   gfx::Size window_size;
   mojo::PendingRemote<blink::mojom::PictureInPictureSession> session_remote;
 
   auto result = GetController().StartSession(
-      this, MediaPlayerId(render_frame_host().GetGlobalId(), player_id),
-      std::move(player_remote), surface_id, natural_size,
-      show_play_pause_button, std::move(observer), source_bounds,
+      this,
+      MediaPlayerId(render_frame_host().GetGlobalId(),
+                    pending_session->player_id),
+      std::move(pending_session->player_remote), pending_session->surface_id,
+      pending_session->natural_size, pending_session->show_play_pause_button,
+      std::move(pending_session->observer), pending_session->source_bounds,
       std::move(immersive_options), &session_remote, &window_size);
 
   if (result == PictureInPictureResult::kSuccess) {
@@ -58,14 +88,58 @@
             blink::scheduler::WebSchedulerTrackedFeature::kPictureInPicture);
   }
 
-  std::move(callback).Run(std::move(session_remote), window_size);
+  std::move(pending_session->callback)
+      .Run(std::move(session_remote), window_size);
 }
 
-void PictureInPictureServiceImpl::RequestImmersivePlaybackConfirmation(
-    RequestImmersivePlaybackConfirmationCallback callback) {
-  GetController().RequestImmersivePlaybackConfirmation(std::move(callback));
+void PictureInPictureServiceImpl::StartSessionImmersive(
+    std::unique_ptr<PictureInPictureServiceImpl::PendingSession>
+        pending_session) {
+  // Immersive playback confirmation flow can only be requested in a
+  // browser-native fullscreen state.
+  auto* web_contents = WebContents::FromRenderFrameHost(&render_frame_host());
+  if (!web_contents || !web_contents->IsFullscreen()) {
+    return;
+  }
+
+  GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(
+      &PictureInPictureServiceImpl::OnImmersivePlaybackConfirmation,
+      immersive_confirmation_weak_factory_.GetWeakPtr(),
+      std::move(pending_session)));
 }
 
+void PictureInPictureServiceImpl::OnImmersivePlaybackConfirmation(
+    std::unique_ptr<PendingSession> pending_session,
+    blink::mojom::ImmersivePlaybackConfirmationResultPtr result) {
+  if (result->status !=
+          blink::mojom::ImmersivePlaybackConfirmationStatus::kConfirmed ||
+      !result->options) {
+    return;
+  }
+
+  StartSessionInternal(std::move(pending_session), std::move(result->options));
+}
+
+PictureInPictureServiceImpl::PendingSession::PendingSession(
+    uint32_t player_id,
+    mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> player_remote,
+    const viz::SurfaceId& surface_id,
+    const gfx::Size& natural_size,
+    bool show_play_pause_button,
+    mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver> observer,
+    const gfx::Rect& source_bounds,
+    PictureInPictureServiceImpl::StartSessionCallback callback)
+    : player_id(player_id),
+      player_remote(std::move(player_remote)),
+      surface_id(surface_id),
+      natural_size(natural_size),
+      show_play_pause_button(show_play_pause_button),
+      observer(std::move(observer)),
+      source_bounds(source_bounds),
+      callback(std::move(callback)) {}
+
+PictureInPictureServiceImpl::PendingSession::~PendingSession() = default;
+
 PictureInPictureServiceImpl::PictureInPictureServiceImpl(
     RenderFrameHost& render_frame_host,
     mojo::PendingReceiver<blink::mojom::PictureInPictureService> receiver)
@@ -74,7 +148,11 @@
 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.
-  GetController().OnServiceDeleted(this);
+  if (auto* controller =
+          VideoPictureInPictureWindowControllerImpl::FromWebContents(
+              WebContents::FromRenderFrameHost(&render_frame_host()))) {
+    controller->OnServiceDeleted(this);
+  }
 }
 
 VideoPictureInPictureWindowControllerImpl&
diff --git a/content/browser/picture_in_picture/picture_in_picture_service_impl.h b/content/browser/picture_in_picture/picture_in_picture_service_impl.h
index e84aacc..29b15e4 100644
--- a/content/browser/picture_in_picture/picture_in_picture_service_impl.h
+++ b/content/browser/picture_in_picture/picture_in_picture_service_impl.h
@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_SERVICE_IMPL_H_
 #define CONTENT_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_SERVICE_IMPL_H_
 
+#include "base/memory/weak_ptr.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/document_service.h"
 #include "media/mojo/mojom/media_player.mojom.h"
@@ -49,20 +50,54 @@
       bool show_play_pause_button,
       mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>,
       const gfx::Rect& source_bounds,
-      blink::mojom::ImmersiveOptionsPtr immersive_options,
+      bool request_immersive,
       StartSessionCallback) final;
-  void RequestImmersivePlaybackConfirmation(
-      RequestImmersivePlaybackConfirmationCallback) final;
 

diff --git a/content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc b/content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc
index 05a8a314..8304362 100644
--- a/content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc
+++ b/content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc
@@ -5,6 +5,7 @@
 #include "content/browser/picture_in_picture/picture_in_picture_service_impl.h"
 
 #include <memory>
+#include <tuple>
 #include <utility>
 
 #include "base/memory/raw_ptr.h"
@@ -25,7 +26,10 @@
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/test_support/fake_message_dispatch_context.h"
+#include "mojo/public/cpp/test_support/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "third_party/blink/public/mojom/frame/fullscreen.mojom.h"
 #include "third_party/blink/public/mojom/picture_in_picture/picture_in_picture.mojom.h"
 
 using testing::_;
@@ -67,6 +71,23 @@
               (base::OnceCallback<
                   void(blink::mojom::ImmersivePlaybackConfirmationResultPtr)>),
               (override));
+
+  void EnterFullscreenModeForTab(
+      RenderFrameHost* requesting_frame,
+      const blink::mojom::FullscreenOptions& options) override {
+    is_fullscreen_ = true;
+  }
+
+  void ExitFullscreenModeForTab(WebContents* web_contents) override {
+    is_fullscreen_ = false;
+  }
+
+  bool IsFullscreenForTabOrPending(const WebContents* web_contents) override {
+    return is_fullscreen_;
+  }
+
+ private:
+  bool is_fullscreen_ = false;
 };
 
 class TestOverlayWindow : public VideoOverlayWindow {
@@ -187,6 +208,22 @@
     mojo::Remote<blink::mojom::PictureInPictureService> service_remote;
     service_impl_ = PictureInPictureServiceImpl::CreateForTesting(
         render_frame_host, service_remote.BindNewPipeAndPassReceiver());
+
+    surface_id_ = viz::SurfaceId(
+        viz::FrameSinkId(1, 1),
+        viz::LocalSurfaceId(
+            11, base::UnguessableToken::CreateForTesting(0x111111, 0)));
+
+    source_bounds_ = gfx::Rect(1, 2, 3, 4);
+    window_size_ = gfx::Size(42, 42);
+    show_play_pause_button_ = true;
+    player_id_ = 30;
+
+    default_immersive_options_ = blink::mojom::ImmersiveOptions::New();
+    default_immersive_options_->stereo_mode =
+        blink::mojom::ImmersiveStereoMode::kMono;
+    default_immersive_options_->projection_type =
+        blink::mojom::ImmersiveProjectionType::kQuad;
   }
 
   void TearDown() override {
@@ -198,6 +235,20 @@
 
   PictureInPictureDelegate& delegate() { return delegate_; }
 
+  const viz::SurfaceId& surface_id() const { return surface_id_; }
+
+  const gfx::Rect& source_bounds() const { return source_bounds_; }
+
+  const gfx::Size& window_size() const { return window_size_; }
+
+  bool show_play_pause_button() const { return show_play_pause_button_; }
+
+  int player_id() const { return player_id_; }
+
+  const blink::mojom::ImmersiveOptionsPtr& default_immersive_options() const {
+    return default_immersive_options_;
+  }
+
   mojo::PendingAssociatedRemote<media::mojom::MediaPlayer>
   BindMediaPlayerReceiverAndPassRemote() {
     return media_player_receiver_.BindMediaPlayerReceiverAndPassRemote();
@@ -205,6 +256,38 @@
 
   void ResetMediaPlayerReceiver() { media_player_receiver_.receiver().reset(); }
 
+  PictureInPictureServiceImpl::StartSessionCallback BindSession(
+      mojo::Remote<blink::mojom::PictureInPictureSession>& session_remote_out,
+      gfx::Size& window_size_out) {
+    return base::BindLambdaForTesting(
+        [&session_remote_out, &window_size_out](
+            mojo::PendingRemote<blink::mojom::PictureInPictureSession> remote,
+            const gfx::Size& b) {
+          if (remote.is_valid()) {
+            session_remote_out.Bind(std::move(remote));
+          }
+          window_size_out = b;
+        });
+  }
+
+  void EnterFullscreen() {
+    // Simulate fullscreen being entered.
+    std::ignore = main_test_rfh()->frame_tree_node()->UpdateUserActivationState(
+        blink::mojom::UserActivationUpdateType::kNotifyActivation,
+        blink::mojom::UserActivationNotificationType::kTest);
+    main_test_rfh()->EnterFullscreen(blink::mojom::FullscreenOptions::New(),
+                                     base::DoNothing());
+    ASSERT_TRUE(contents()->IsFullscreen());
+  }
+
+  VideoPictureInPictureWindowControllerImpl* GetController() {
+    auto* controller =
+        VideoPictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
+            contents());
+    CHECK(controller);
+    return controller;
+  }
+
  private:
   PictureInPictureTestBrowserClient browser_client_;
   PictureInPictureDelegate delegate_;
@@ -212,15 +295,16 @@
   raw_ptr<PictureInPictureServiceImpl> service_impl_;
   // Required to pass a valid PendingRemote to StartSession() in the tests.
   PictureInPictureMediaPlayerReceiver media_player_receiver_;
+  viz::SurfaceId surface_id_;
+  blink::mojom::ImmersiveOptionsPtr default_immersive_options_;
+  gfx::Rect source_bounds_;
+  gfx::Size window_size_;
+  bool show_play_pause_button_;
+  int player_id_;
 };
 
 TEST_F(PictureInPictureServiceImplTest, EnterPictureInPicture) {
-  const int kPlayerVideoOnlyId = 30;
-  const VideoPictureInPictureWindowControllerImpl* controller =
-      VideoPictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
-          contents());
-
-  ASSERT_TRUE(controller);
+  auto* controller = GetController();
 
   DummyPictureInPictureSessionObserver observer;
   mojo::Receiver<blink::mojom::PictureInPictureSessionObserver>
@@ -232,36 +316,23 @@
   // If Picture-in-Picture there shouldn't be an active session.
   EXPECT_FALSE(controller->active_session_for_testing());
 
-  viz::SurfaceId surface_id = viz::SurfaceId(
-      viz::FrameSinkId(1, 1),
-      viz::LocalSurfaceId(
-          11, base::UnguessableToken::CreateForTesting(0x111111, 0)));
-
   EXPECT_CALL(delegate(), IsPictureInPictureEnabled())
       .WillRepeatedly(testing::Return(true));
   EXPECT_CALL(delegate(), EnterPictureInPicture(contents()))
       .WillRepeatedly(testing::Return(PictureInPictureResult::kSuccess));
 
-  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote;
-  gfx::Size window_size;
+  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote_out;
+  gfx::Size window_size_out;
 
-  const gfx::Rect source_bounds(1, 2, 3, 4);
-  service().StartSession(
-      kPlayerVideoOnlyId, BindMediaPlayerReceiverAndPassRemote(), surface_id,
-      gfx::Size(42, 42), true /* show_play_pause_button */,
-      std::move(observer_remote), source_bounds,
-      nullptr /* immersive_options */,
-      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;
-          }));
+  service().StartSession(player_id(), BindMediaPlayerReceiverAndPassRemote(),
+                         surface_id(), window_size(), show_play_pause_button(),
+                         std::move(observer_remote), source_bounds(),
+                         /*request_immersive=*/false,
+                         BindSession(session_remote_out, window_size_out));
 
-  EXPECT_TRUE(session_remote);
-  EXPECT_EQ(gfx::Size(42, 42), window_size);
-  EXPECT_EQ(source_bounds, controller->GetSourceBounds());
+  EXPECT_TRUE(session_remote_out);
+  EXPECT_EQ(window_size(), window_size_out);
+  EXPECT_EQ(source_bounds(), controller->GetSourceBounds());
 
   // Picture-in-Picture media player id should not be reset when the media is
   // destroyed (e.g. video stops playing). This allows the Picture-in-Picture
@@ -271,61 +342,31 @@
 }
 
 TEST_F(PictureInPictureServiceImplTest, EnterPictureInPicture_NotSupported) {
-  const int kPlayerVideoOnlyId = 30;
-  const VideoPictureInPictureWindowControllerImpl* controller =
-      VideoPictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
-          contents());
-
-  ASSERT_TRUE(controller);
-  EXPECT_FALSE(controller->active_session_for_testing());
-
-  mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>
-      observer_remote;
-  viz::SurfaceId surface_id = viz::SurfaceId(
-      viz::FrameSinkId(1, 1),
-      viz::LocalSurfaceId(
-          11, base::UnguessableToken::CreateForTesting(0x111111, 0)));
-
   EXPECT_CALL(delegate(), IsPictureInPictureEnabled())
       .WillRepeatedly(testing::Return(true));
   EXPECT_CALL(delegate(), EnterPictureInPicture(contents()))
       .WillRepeatedly(testing::Return(PictureInPictureResult::kNotSupported));
 
-  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote;
-  gfx::Size window_size;
-  const gfx::Rect source_bounds(1, 2, 3, 4);
+  mojo::PendingRemote<blink::mojom::PictureInPictureSessionObserver>
+      observer_remote;
+  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote_out;
+  gfx::Size window_size_out;
+  service().StartSession(player_id(), BindMediaPlayerReceiverAndPassRemote(),
+                         surface_id(), window_size(), show_play_pause_button(),
+                         std::move(observer_remote), source_bounds(),
+                         /*request_immersive=*/false,
+                         BindSession(session_remote_out, window_size_out));
 
-  service().StartSession(
-      kPlayerVideoOnlyId, BindMediaPlayerReceiverAndPassRemote(), surface_id,
-      gfx::Size(42, 42), true /* show_play_pause_button */,
-      std::move(observer_remote), source_bounds,
-      nullptr /* immersive_options */,
-      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(GetController()->active_session_for_testing());
 
-  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
+  // The |session_remote_out| 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);
+  EXPECT_FALSE(session_remote_out);
+  EXPECT_EQ(gfx::Size(), window_size_out);
 }
 
 TEST_F(PictureInPictureServiceImplTest, EnterImmersivePlayback) {
-  const int kPlayerVideoOnlyId = 30;
-  const VideoPictureInPictureWindowControllerImpl* controller =
-      VideoPictureInPictureWindowControllerImpl::GetOrCreateForWebContents(
-          contents());
-
-  ASSERT_TRUE(controller);
-  EXPECT_FALSE(controller->active_session_for_testing());
-
   DummyPictureInPictureSessionObserver observer;
   mojo::Receiver<blink::mojom::PictureInPictureSessionObserver>
       observer_receiver(&observer);
@@ -333,50 +374,157 @@
       observer_remote;
   observer_receiver.Bind(observer_remote.InitWithNewPipeAndPassReceiver());
 
-  viz::SurfaceId surface_id = viz::SurfaceId(
-      viz::FrameSinkId(1, 1),
-      viz::LocalSurfaceId(
-          11, base::UnguessableToken::CreateForTesting(0x111111, 0)));
+  EnterFullscreen();
 
   EXPECT_CALL(delegate(), IsImmersivePlaybackEnabled())
       .WillRepeatedly(testing::Return(true));
   EXPECT_CALL(delegate(), EnterPictureInPicture(contents()))
       .WillRepeatedly(testing::Return(PictureInPictureResult::kSuccess));
 
-  mojo::Remote<blink::mojom::PictureInPictureSession> session_remote;
-  gfx::Size window_size;
+  // Expect the delegate to confirm immersive playback with default options.
+  EXPECT_CALL(delegate(), RequestImmersivePlaybackConfirmation(_))
+      .WillOnce([options = default_immersive_options().Clone()](
+                    base::OnceCallback<void(
+                        blink::mojom::ImmersivePlaybackConfirmationResultPtr)>
+                        callback) mutable {
+        auto result = blink::mojom::ImmersivePlaybackConfirmationResult::New();
... (truncated)