Set Ready For Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Notify the client that the video configuration has changed.Oleh Desiatyrikov (xWF)Instead of adding this, lets just add a getter for the spatial format on WebMediaPlayer since that's a more common pattern.
Dale CurtisYes, it would be a much cleaner approach! Thank you for the suggestion.
Does the `RequestVideoSpatialFormat()` implementation look alright to you?
Oleh Desiatyrikov (xWF)Yes, that looked fine. I didn't see any other method we could attach the information to, but if there is an existing one that runs when immersive mode starts, I'd consider attaching there.
Oleh Desiatyrikov (xWF)I could pass it over directly to the `PictureInPictureController::EnterPictureInPictureImmersive` [[1]](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/media/html_video_element.cc;l=1047;drc=256d8d19b35cf936c3e86339da5409fb88788707) -> `PictureInPictureService::StartSession` [[2]](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/picture_in_picture/picture_in_picture.mojom;l=87;drc=5496fbe366e6796c5b549c65a2cb267b5c933e35)
But we just refactored that part with @libe...@google.com here [[3]](https://g-issues.chromium.org/issues/514069440) + [[4]](crrev.com/c/7899097), so I'm sure if I should put it back there.
Dale Curtisso I'm **NOT** sure if I should put it back there
Oleh Desiatyrikov (xWF)StartSession() does seem like a reasonable place. What's your concern with adding it there?
Dale CurtisNot much of a concern, it's just that we cleaned up those options recently and I will put them back again.
But yes, it is a significantly better place to have these.
If so, I will probably merge this CL together with a followup - crrev.com/c/7885666.
Oleh Desiatyrikov (xWF)Ack
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: d...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): d...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
mojom lgtm; please don't use my LGTM for other files in Blink as I haven't reviewed them
pick an owner from content/browser/picture_in_picture/OWNERS for code there
Hello everyone!
I would greatly appreciate if you can help me by reviewing the following:
Please let me know if I need to peek other reviewers!
Thank you!
pick an owner from content/browser/picture_in_picture/OWNERS for code there
Would you mind reviewing the `components/embedder_support/android/java/src/org/chromium/components/embedder_support/delegate/WebContentsDelegateAndroid.java`?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF)pick an owner from content/browser/picture_in_picture/OWNERS for code there
Would you mind reviewing the `components/embedder_support/android/java/src/org/chromium/components/embedder_support/delegate/WebContentsDelegateAndroid.java`?
| Code-Review | +1 |
LGTM modulo nits
#include "media/base/video_spatial_format.h"Nit: Can you move this to `content/browser/picture_in_picture/picture_in_picture_service_impl.cc` and forward declaration of media::VideoSpatialFormat there?
#include "base/functional/bind.h"Can you explain those header changes?
SetVideoSpatialFormat(media::VideoSpatialFormat{Shall we have more tests for other options as well?
Nit: Can you move this to `content/browser/picture_in_picture/picture_in_picture_service_impl.cc` and forward declaration of media::VideoSpatialFormat there?
Done. Had to also move the `PendingSession` declaration to the .cc file.
Can you explain those header changes?
Refactored imports to remove unused references from previous iterations. Thanks for the catch!
Shall we have more tests for other options as well?
Added coverage for all currently supported combinations.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
media.mojom.VideoSpatialFormat spatial_format)nit: Consider adding the new `spatial_format` parameter explanation in method description comment above, explaining its role in the context of starting a PiP session.
nit: Consider adding the new `spatial_format` parameter explanation in method description comment above, explaining its role in the context of starting a PiP session.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
lgtm for
chrome/android/BUILD.gn
chrome/android/java/src/org/chromium/chrome/browser/app/tab_activity_glue/ActivityTabWebContentsDelegateAndroid.java
chrome/android/java/src/org/chromium/chrome/browser/media/immersive_playback/ImmersivePlaybackSnackbarController.java
chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroidImpl.java
chrome/android/junit/src/org/chromium/chrome/browser/media/immersive_playback/ImmersivePlaybackSnackbarControllerTest.java
chrome/browser/android/tab_web_contents_delegate_android.cc
chrome/browser/android/tab_web_contents_delegate_android.h
Hello, @dch...@chromium.org, @cl...@chromium.org! Would you mind helping me with the owners' approvals for the following files?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Is my review needed?
Thank you for checking in! I've moved you to CC. The vote from @cl...@chromium.org should cover your previous +1 on mojom.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
public void requestImmersivePlaybackConfirmation(existing code but either this should just be in TabWebContentsDelegateAndrodImpl, or the native code calling this should be in embedder_support
jni should not cross layer boundaries
can you fix in that in a separate CL.
public void requestImmersivePlaybackConfirmation(existing code but either this should just be in TabWebContentsDelegateAndrodImpl, or the native code calling this should be in embedder_support
jni should not cross layer boundaries
can you fix in that in a separate CL.
Acknowledged.
Created a bug to track this change: crbug.com/527879786.
[ImmersivePlayback] Plumb video spatial format to PiP service and confirmation flow
Propagates video spatial format (VideoStereoMode and VideoProjectionType
enums) from Blink's WebMediaPlayer through
PictureInPictureService::StartSession.
Plumbs ImmersiveOptions through VideoPictureInPictureWindowController,
WebContentsDelegate, and ActivityTabWebContentsDelegateAndroid to
trigger the immersive playback confirmation snackbar on Android.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |