Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tasks for enterPictureInPicture and exitPictureInPicture. #233

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

markafoltz
Copy link
Contributor

@markafoltz markafoltz commented Sep 26, 2024

Closes #230: ensures that Promises returned by these methods use the media element task source.

Also fixes several ambiguous cross references.


Preview | Diff

@markafoltz
Copy link
Contributor Author

Should also close #225

@markafoltz markafoltz self-assigned this Sep 27, 2024
@beaufortfrancois
Copy link
Collaborator

@markafoltz From what I can see in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc;l=158?q=kMediaElementEvent%20f:picture&ss=chromium%2Fchromium%2Fsrc, Chromium's implementation will no longer comply with the spec. I may be wrong about this but is this what we want ?

Note that this PR is related to #229

@markafoltz
Copy link
Contributor Author

It should be impossible to fire events or resolve promises off of the main thread in Blink, so where do you see Chrome not complying? (Sorry I don't know the PiP code.)

@markafoltz
Copy link
Contributor Author

Thanks for the pointer as well, this should also close #229

@beaufortfrancois
Copy link
Collaborator

It should be impossible to fire events or resolve promises off of the main thread in Blink, so where do you see Chrome not complying? (Sorry I don't know the PiP code.)

Like I said, I may be wrong, but https://source.chromium.org/search?q=kMediaElementEvent%20f:picture&sq=&ss=chromium%2Fchromium%2Fsrc returns results where we specifically use the media element task source in Picture-in-Picture, and I'm not sure it covers all the cases you've updated in this PR.

An example would be the user agent MUST <a>queue a picture-in-picture task</a> to <a>fire an event</a> named {{resize}} at {{pictureInPictureElement}}. at https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_window.cc;l=30;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb
I believe we should be using something like EnqueueEvent(*Event::Create(event_type_names::kResize), TaskType::kMediaElementEvent); instead of synchronously dispatching the event with DispatchEvent(*Event::Create(event_type_names::kResize)); like we do today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants