<rdar://problem/56267934>

Created attachment 381878 [details] Patch WIP

Created attachment 381910 [details] Patch

Comment on attachment 381910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381910&action=review > Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:104 > + videoElement.scheduleEvent(EnterPictureInPictureEvent::create(eventNames().enterpictureinpictureEvent, WTFMove(initializer))); Why fire an event? The spec says to abort the Picture-in-Picture algorithm at step 7 if requestPictureInPicture is called on the current PiP element, and the event is fired at step 11: When the request Picture-in-Picture algorithm with video, userActivationRequired and playingRequired is invoked, the user agent MUST run the following steps: ... 7. If video is pictureInPictureElement, abort these steps. ... 11. Queue a task to fire an event with the name enterpictureinpicture using EnterPictureInPictureEvent at the video with its bubbles attribute initialized to true and its pictureInPictureWindow attribute initialized to Picture-in-Picture window. https://w3c.github.io/picture-in-picture/#request-picture-in-picture-algorithm > Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:173 > m_videoElement.document().setPictureInPictureElement(nullptr); > + m_pictureInPictureWindow->setSize({0, 0}); The spec says to run the close window algorithm before clearing pictureInPictureElement. The close window algorithm says to fire a resize event at pictureInPictureElement if the size changes, so you should call pictureInPictureWindowResized instead of setting the size directly. > Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187 > + m_pictureInPictureWindow->setSize(windowSize); > + auto resizeEvent = Event::create(eventNames().resizeEvent, Event::CanBubble::Yes, Event::IsCancelable::No); > + resizeEvent->setTarget(m_pictureInPictureWindow); > + m_videoElement.scheduleEvent(WTFMove(resizeEvent)); The event should only fire when the window size changes. > Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:46 > + , m_width(0) > + , m_height(0) Nit: why not use an IntSize for the member variable as well? > Source/WebCore/html/HTMLVideoElement.cpp:532 > + // fullscreenMode() does not always provide the correct fullscreen mode > + // when mode changing is happening It seems that it would be better to fix that problem instead of working around it here so we don't trip over this again.

Comment on attachment 381910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381910&action=review > Source/WebCore/ChangeLog:9 > + With manual test, the implementation passes > + - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html What is holding up writing an automated test for this?

Comment on attachment 381910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381910&action=review >> Source/WebCore/ChangeLog:9 >> + - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html > > What is holding up writing an automated test for this? https://bugs.webkit.org/show_bug.cgi?id=202617 Current WebkitTestRunner does not support wpt/resources/testdriver.js:test_driver.bless() properly, so the operations requiring user gesture will not work properly in the test cases. >> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:104 >> + videoElement.scheduleEvent(EnterPictureInPictureEvent::create(eventNames().enterpictureinpictureEvent, WTFMove(initializer))); > > Why fire an event? The spec says to abort the Picture-in-Picture algorithm at step 7 if requestPictureInPicture is called on the current PiP element, and the event is fired at step 11: > > When the request Picture-in-Picture algorithm with video, userActivationRequired and playingRequired is invoked, the user agent MUST run the following steps: > ... > 7. If video is pictureInPictureElement, abort these steps. > ... > 11. Queue a task to fire an event with the name enterpictureinpicture using EnterPictureInPictureEvent at the video with its bubbles attribute initialized to true and its pictureInPictureWindow attribute initialized to Picture-in-Picture window. > > https://w3c.github.io/picture-in-picture/#request-picture-in-picture-algorithm Good catch. Yes, we should not fire an event here. But we do need to resolve the promise with the current picture-in-picture window here. >> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:173 >> + m_pictureInPictureWindow->setSize({0, 0}); > > The spec says to run the close window algorithm before clearing pictureInPictureElement. The close window algorithm says to fire a resize event at pictureInPictureElement if the size changes, so you should call pictureInPictureWindowResized instead of setting the size directly. I use the setSize() function to set the width and height to 0, which indicates the picture-in-picture window is in the closed state. Defining a close() function to do that seems to be a better idea. >> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187 >> + m_videoElement.scheduleEvent(WTFMove(resizeEvent)); > > The event should only fire when the window size changes. Right, this implementation does the same as you describe. >> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:46 >> + , m_height(0) > > Nit: why not use an IntSize for the member variable as well? Yes, we can do that. Will change the implementation. >> Source/WebCore/html/HTMLVideoElement.cpp:532 >> + // when mode changing is happening > > It seems that it would be better to fix that problem instead of working around it here so we don't trip over this again. Agree. But that will be a big patch. Changing the presentation mode is a multiple-step operation and more than one process is involved. The current implementation does not handle the state synchronization very well. I think reporting a new bug to track the fix will be a better idea.

Comment on attachment 381910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381910&action=review >>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187 >>> + m_videoElement.scheduleEvent(WTFMove(resizeEvent)); >> >> The event should only fire when the window size changes. > > Right, this implementation does the same as you describe. Oh, got your point. You mean we need to check the original window size and only fire event if the window size is changed. Right? Actually function pictureInPictureWindowResized() will be called only if the window size IS changed, but we definitely can check the size in this function.

Created attachment 381986 [details] Patch Update the patch based on comments from Eric

Comment on attachment 381986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381986&action=review > Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:101 > + auto pictureInPictureWindow = videoElementPictureInPicture->m_pictureInPictureWindow; > + promise->resolve<IDLInterface<PictureInPictureWindow>>(*pictureInPictureWindow); Nit: I'm not sure the local variable adds anything. > Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:169 > INFO_LOG(LOGIDENTIFIER); > + m_pictureInPictureWindow->close(); > m_videoElement.document().setPictureInPictureElement(nullptr); Step 2 of the "exit Picture-in-Picture algorithm" is "run the close window algorithm", and the "close window algorithm" says you must: ... set the window state is set to closed. ... set the width attribute to 0. ... set the height attribute to 0. ... if the size of the Picture-in-Picture window changes, queue a task to fire a 'resize' event So if this is called when the window has not already been set to { 0, 0 }, you need to fire a 'resize' event. > Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:58 > + m_size.setWidth(0); > + m_size.setHeight(0); Nit: I think you can simplify this to something like "m_size = { 0, 0 }" > Source/WebCore/html/HTMLVideoElement.cpp:532 > + // fullscreenMode() does not always provide the correct fullscreen mode > + // when mode changing is happening Nit: it would be good to have a bug filed for the real fix for this problem so we don't lose track of it. Please include the bug number here.

Comment on attachment 381986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381986&action=review >> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:101 >> + promise->resolve<IDLInterface<PictureInPictureWindow>>(*pictureInPictureWindow); > > Nit: I'm not sure the local variable adds anything. Right, will fix it. >> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:169 >> m_videoElement.document().setPictureInPictureElement(nullptr); > > Step 2 of the "exit Picture-in-Picture algorithm" is "run the close window algorithm", and the "close window algorithm" says you must: > > ... set the window state is set to closed. > ... set the width attribute to 0. > ... set the height attribute to 0. > ... if the size of the Picture-in-Picture window changes, queue a task to fire a 'resize' event > > So if this is called when the window has not already been set to { 0, 0 }, you need to fire a 'resize' event. Hmm, you mean we need to fire a resize event with the window size to be {0, 0}? That sounds a little weird because a "leavepictureinpicture" event will be fired as well. I am not convinced that the spec means that behavior. If it does, we may need to report a bug to it. What do you think? >> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:58 >> + m_size.setHeight(0); > > Nit: I think you can simplify this to something like "m_size = { 0, 0 }" Right, will change the patch. >> Source/WebCore/html/HTMLVideoElement.cpp:532 >> + // when mode changing is happening > > Nit: it would be good to have a bug filed for the real fix for this problem so we don't lose track of it. Please include the bug number here. https://bugs.webkit.org/show_bug.cgi?id=203443

(In reply to Peng Liu from comment #6) > Comment on attachment 381910 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381910&action=review > > >> Source/WebCore/ChangeLog:9 > >> + - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html > > > > What is holding up writing an automated test for this? > > https://bugs.webkit.org/show_bug.cgi?id=202617 > Current WebkitTestRunner does not support > wpt/resources/testdriver.js:test_driver.bless() properly, so the operations > requiring user gesture will not work properly in the test cases. That seems to be about support WPT test. Can you write a regression test, even an API test, for this in the mean time?

Comment on attachment 381910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381910&action=review >>>> Source/WebCore/ChangeLog:9 >>>> + - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html >>> >>> What is holding up writing an automated test for this? >> >> https://bugs.webkit.org/show_bug.cgi?id=202617 >> Current WebkitTestRunner does not support wpt/resources/testdriver.js:test_driver.bless() properly, so the operations requiring user gesture will not work properly in the test cases. > > That seems to be about support WPT test. Can you write a regression test, even an API test, for this in the mean time? Sure, I will add some layout test cases for it.

Created attachment 382101 [details] Patch

Created attachment 382125 [details] Patch Update the patch to fix layout test failure

Created attachment 382134 [details] Patch The added layout test cases are only relevant on mac and iPad, they are enabled only for mac in this patch.

Created attachment 382146 [details] Patch Rebasing the patch

Comment on attachment 382146 [details] Patch Attachment 382146 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13187024 New failing tests: http/tests/misc/repeat-open-cancel.html

Created attachment 382152 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 382156 [details] Patch The layout test cases are enabled for mac-wk2 only

Comment on attachment 382156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382156&action=review This is much better and really close, but I think the test should be filled out a bit. r=me with a more complete test. > Source/WebCore/html/HTMLVideoElement.cpp:532 > + // fullscreenMode() does not always provide the correct fullscreen mode > + // when mode changing is happening Nit: this should reference the bug you filed about fixing this. > LayoutTests/media/picture-in-picture-api-pip-events.html:19 > + run('video.src = findMediaFile("video", "content/test")'); > + await waitFor(video, 'canplaythrough'); > + > + run('video.play()'); > + await waitFor(video, 'playing'); > + > + runWithKeyDown(function() { video.requestPictureInPicture(); }); > + await waitFor(video, 'enterpictureinpicture'); This is fine as far as it goes, but you should test the other steps in the request Picture-in-Picture algorithm: throw if the state is HAVE_NOTHING, throw if there is no video track, throw if not triggered by a user gesture, check that document.pictureInPictureElement is set correctly, check that the 'enterpictureinpicture' event's pictureInPictureWindow attribute is set. > LayoutTests/media/picture-in-picture-api-pip-events.html:22 > + runWithKeyDown(function() { document.exitPictureInPicture(); }); > + await waitFor(video, 'leavepictureinpicture'); This should check everything mandated by the exit Picture-in-Picture algorithm.

Comment on attachment 382156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382156&action=review >> Source/WebCore/html/HTMLVideoElement.cpp:532 >> + // when mode changing is happening > > Nit: this should reference the bug you filed about fixing this. OK, will do that. >> LayoutTests/media/picture-in-picture-api-pip-events.html:19 >> + await waitFor(video, 'enterpictureinpicture'); > > This is fine as far as it goes, but you should test the other steps in the request Picture-in-Picture algorithm: throw if the state is HAVE_NOTHING, throw if there is no video track, throw if not triggered by a user gesture, check that document.pictureInPictureElement is set correctly, check that the 'enterpictureinpicture' event's pictureInPictureWindow attribute is set. Will create a new test page for all those cases. >> LayoutTests/media/picture-in-picture-api-pip-events.html:22 >> + await waitFor(video, 'leavepictureinpicture'); > > This should check everything mandated by the exit Picture-in-Picture algorithm. OK, ditto.

Created attachment 382201 [details] Patch Upate the patch based on Eric's comments and add a bunch of layout test cases.

Created attachment 382221 [details] Updated patch for landing Update LayoutTests/media/picture-in-picture-api-enter-pip-2.html to fix a potential race condition (Caught by Eric).

Comment on attachment 382221 [details] Updated patch for landing Rejecting attachment 382221 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 382221, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13189372

Comment on attachment 382221 [details] Updated patch for landing Clearing flags on attachment: 382221 Committed r251745: <https://trac.webkit.org/changeset/251745>

All reviewed patches have been landed. Closing bug.

4 of the new tests added in https://trac.webkit.org/changeset/251745/webkit are flakey timeouts: media/picture-in-picture-api-enter-pip-4.html media/picture-in-picture-api-exit-pip-1.html media/picture-in-picture-api-pip-events.html media/picture-in-picture-api-pip-window.html The other tests may also be flakey timeouts but have not done that yet. History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=media%2Fpicture-in-picture-api-enter-pip-2.html&test=media%2Fpicture-in-picture-api-enter-pip-3.html&test=media%2Fpicture-in-picture-api-enter-pip-4.html&test=media%2Fpicture-in-picture-api-exit-pip-1.html&test=media%2Fpicture-in-picture-api-exit-pip-2.html&test=media%2Fpicture-in-picture-api-pip-events.html&test=media%2Fpicture-in-picture-api-pip-window.html Can this be looked at soon?

Sure, looking into them now. Thanks for the reminder.

(In reply to Truitt Savell from comment #27) > 4 of the new tests added in https://trac.webkit.org/changeset/251745/webkit > > are flakey timeouts: > media/picture-in-picture-api-enter-pip-4.html > media/picture-in-picture-api-exit-pip-1.html > media/picture-in-picture-api-pip-events.html > media/picture-in-picture-api-pip-window.html > > The other tests may also be flakey timeouts but have not done that yet. > > History: > https://results.webkit.org/?suite=layout-tests&suite=layout- > tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout- > tests&suite=layout-tests&test=media%2Fpicture-in-picture-api-enter-pip-2. > html&test=media%2Fpicture-in-picture-api-enter-pip-3. > html&test=media%2Fpicture-in-picture-api-enter-pip-4. > html&test=media%2Fpicture-in-picture-api-exit-pip-1. > html&test=media%2Fpicture-in-picture-api-exit-pip-2. > html&test=media%2Fpicture-in-picture-api-pip-events. > html&test=media%2Fpicture-in-picture-api-pip-window.html > > Can this be looked at soon? I can reproduce the timeout locally by running multiple cases simultaneously. A bug is reported for it. https://bugs.webkit.org/show_bug.cgi?id=203614

(In reply to Truitt Savell from comment #27) > > The other tests may also be flakey timeouts but have not done that yet. > The other test cases will be OK because they cover the corner cases in which PiP mode will not be activated.

Created attachment 382329 [details] Mark four layout test cases as flakey timeout

Reopening to fix layout tests flakey timeout.

Comment on attachment 382329 [details] Mark four layout test cases as flakey timeout Clearing flags on attachment: 382329 Committed r251797: <https://trac.webkit.org/changeset/251797>

All reviewed patches have been landed. Closing bug.