[Blink] Implement RequestImmersivePlayback in PictureInPictureController [chromium/src : main]

[Oleh Desiatyrikov (xWF) (Gerrit)]

Attention needed from Frank Liberato

Oleh Desiatyrikov (xWF) added 3 comments

File third_party/blink/renderer/core/html/media/html_video_element.cc

Line 919, Patchset 3: .RequestImmersivePlayback(*this);

Frank Liberato . resolved

not sure i follow this - it feels wrong to request immersive every time, unconditionally.

Oleh Desiatyrikov (xWF)

The idea was to check `WebContentsDelegate::IsImmersivePlaybackEnabled` and fall back if it's not supported. I could probably put this into a `web_settings` and check the current state of the feature this way.

Oleh Desiatyrikov (xWF)

I added a check for the `GetDocument().GetSettings()->GetImmersiveVideoPlaybackEnabled()` flag. So, we won't try to request the playback if it's not enabled.

File third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc

Line 640, Patchset 3: pending_video_element_ = &video_element;

Frank Liberato . resolved

this pattern feels very fragile - e.g. if a second request comes in while the first is pending. i suggest binding it as an argument into the callback @643, if there's a way to do that with oilpan. i forget the rules.

Oleh Desiatyrikov (xWF)

Yep, now I see it (after splitting the entire implementation into a smaller chunks). I will look into making it more robust.

Oleh Desiatyrikov (xWF)

Bound the video element into a callback.

Line 649, Patchset 3: if (pending_video_element_ &&

Frank Liberato . resolved

this feels like a race - lots of stuff can happen while the user is considering their answer carefully.

it might work out because EnterPictureInPicture() might check all the state we care about.

Oleh Desiatyrikov (xWF)

I was thinking about mechanism to dismiss the dialog if element leaves fullscreen or is destroyed. But yes, I need to add more checks here at least to ensure the element is still valid.

Oleh Desiatyrikov (xWF)

Added an extra check for `IsElementAllowed` before calling the `EnterImmersivePlayback`.

Open in Gerrit

Related details

Attention is currently required from:

• Frank Liberato

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I75c9438278b83ef1d1f4217db0f0cf0f799af45b

Gerrit-Change-Number: 7716732

Gerrit-PatchSet: 7

Gerrit-Owner: Oleh Desiatyrikov (xWF) <desiat...@google.com>

Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>

Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>

Gerrit-CC: Gurmeet Kalra <gurm...@google.com>

Gerrit-CC: Yizhi Zhao <yi...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-Attention: Frank Liberato <libe...@chromium.org>

Gerrit-Comment-Date: Thu, 02 Apr 2026 18:17:24 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Oleh Desiatyrikov (xWF) <desiat...@google.com>

Comment-In-Reply-To: Frank Liberato <libe...@chromium.org>

[Oleh Desiatyrikov (xWF) (Gerrit)]

Attention needed from Frank Liberato

Oleh Desiatyrikov (xWF) added 1 comment

File third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.h

Line 166, Patchset 3: // Handles the result of an immersive session request.

Frank Liberato . resolved

i think you need to describe 'immersive' better. most folks who touch this will not understand what that means.

Oleh Desiatyrikov (xWF)

Something like "Xr Immersive Video Playback Session"?

Oleh Desiatyrikov (xWF)

Renamed to `OnImmersivePlaybackConfirmationResult` for clarity.

Gerrit-Comment-Date: Thu, 02 Apr 2026 18:18:29 +0000