Bug 221083

Summary: EnterPictureInPictureEvent event was renamed to PictureInPictureEvent in spec
Product: WebKit Reporter: Philip Jägenstedt <philip>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kangil.han, kondapallykalyan, peng.liu6, philipj, ryuan.choi, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
youennf: review+
Rebase the patch
none
[fast-cq] Patch for landing none

Description Philip Jägenstedt 2021-01-28 01:20:49 PST 
https://github.com/w3c/picture-in-picture/pull/189 renamed the EnterPictureInPictureEvent interface to just PictureInPictureEvent.

The event type is still "enterpictureinpicture" so doing this renaming should be very low risk.

There are some failures in WPT due to this:
https://wpt.fyi/results/picture-in-picture/idlharness.window.html?run_id=5142493001678848&run_id=5150525664264192&run_id=5705922615705600
Comment 1 Peng Liu 2021-01-28 10:21:23 PST 
We also need to update "leavepictureinpicture" event based on the discussion in https://github.com/w3c/picture-in-picture/issues/188.
Comment 3 Radar WebKit Bug Importer 2021-02-04 01:21:12 PST 
<rdar://problem/73971131>
Comment 4 Peng Liu 2022-01-28 10:43:26 PST 
Created attachment 450256 [details]
Patch
Comment 5 Peng Liu 2022-01-28 12:48:38 PST 
Created attachment 450269 [details]
Patch
Comment 6 youenn fablet 2022-01-29 01:50:54 PST 
Comment on attachment 450269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450269&action=review

r=me, worth checking API test results though.


> Source/WebCore/ChangeLog:11
> +        (https://github.com/w3c/picture-in-picture/issues/188)

Not sure how much of this is tested in WPT tests but maybe it is already covered and we should just resync the corresponding WPT tests.

> Source/WebCore/ChangeLog:13
> +        Covered by media/picture-in-picture/picture-in-picture-api-events.html.

PictureInPictureEvent new name is not covered though.
Can we update LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/ to take benefit of the IDL harness coverage?
I would hope the IDL harness coverage test to be runnable without any issue.
Comment 7 youenn fablet 2022-01-29 01:52:11 PST 
Hum, I see webkit.org/b/202617 now.
Might be worth the effort.
I still wonder whether some of the tests can already be run today, say imported/w3c/web-platform-tests/picture-in-picture/idlharness.window.html for instance. Can you try removing some of the Skip test expectations and update the expected.txt files in this patch?
Comment 8 Peng Liu 2022-02-07 10:10:04 PST 
(In reply to youenn fablet from comment #7)
> Hum, I see webkit.org/b/202617 now.
> Might be worth the effort.
> I still wonder whether some of the tests can already be run today, say
> imported/w3c/web-platform-tests/picture-in-picture/idlharness.window.html
> for instance. Can you try removing some of the Skip test expectations and
> update the expected.txt files in this patch?

Probably we can enable some tests now, but not all of them. The reason is that those tests "shares" the system picture-in-picture implementation and cannot execute in parallel. One possible solution would be enabling the MockVideoPresentationMode of the WKTR by default.
Comment 9 Peng Liu 2022-03-08 12:39:41 PST 
Created attachment 454145 [details]
Rebase the patch
Comment 10 Peng Liu 2022-03-09 14:35:02 PST 
Created attachment 454289 [details]
[fast-cq] Patch for landing
Comment 11 EWS 2022-03-10 21:04:38 PST 
Committed r291144 (248304@main): <https://commits.webkit.org/248304@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454289 [details].