Bug 211645

Created attachment 398943 [details] WIP patch

<rdar://problem/63057695>

Created attachment 398960 [details] WIP patch v2

Created attachment 398979 [details] WIP patch v3

Created attachment 398985 [details] WIP patch v4

Comment on attachment 398985 [details] WIP patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review > Source/WebCore/ChangeLog:3 > + Enable the mock video presentation mode in related layout tests and fix test failures This title is confusing. This is a patch that fixes some bugs in our media code. I think that ideally the bug title would make it clear what those bugs are, not just the technique used to test to find them. Or does this patch's cleanup of internal state only relate to enabling more tests, and not fix any bugs?

Comment on attachment 398985 [details] WIP patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review >> Source/WebCore/ChangeLog:3 >> + Enable the mock video presentation mode in related layout tests and fix test failures > > This title is confusing. This is a patch that fixes some bugs in our media code. I think that ideally the bug title would make it clear what those bugs are, not just the technique used to test to find them. > > Or does this patch's cleanup of internal state only relate to enabling more tests, and not fix any bugs? Thanks for the reviewing and sorry for the confusing title. Initially, this patch was to un-skip some tests competing resources (specifically Picture-in-Picture and video fullscreen) by enabling the mock video presentation mode. But after un-skipping these tests, I found other test flakiness (timeout and crash due to assertion failures). So the patch got bigger and bigger. To summary, this patch does the following things: (1) Enable the mock video presentation mode for some tests related to picture-in-picture and video fullscreen to make sure they can run in parallel. (2) Cleanup the internal state variables of HTMLMediaElement to fix an assertion failure in a special case only happens in the test (on Mac, and element fullscreen API is disabled). (3) Fix a mistake in r202274 (again, the mistake only affects tests like item (2). (4) Update HTMLVideoElement::webkitDisplayingFullscreen() to fix the timing issue of some tests. The updated function returns true after the entering fullscreen/Picture-in-Picture is really completed. Many tests use that function to check state and proceed the test when its return value is changed. (5) There are some minor clean-up in this patch as well, such as adding/fixing comments in the tests, and removing unnecessary code in HTMLMediaElement::exitFullscreen(). So this patch only fixes issues found in the tests and make the tests deterministic.

(In reply to Peng Liu from comment #7) > So this patch only fixes issues found in the tests and make the tests > deterministic. OK, so to confirm, no benefit outside the tests?

Correct.

The issues fixed by this patch only affect tests as far as I know.

Comment on attachment 398985 [details] WIP patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review > Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226 > + return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture()); Too many parentheses here. > LayoutTests/media/media-fullscreen-loop-inline.html:22 > + window.internals.settings.setAllowsInlineMediaPlayback(false); > + window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true); > + // Disable the Fullscreen API (element fullscreen) support > + window.internals.settings.setFullScreenEnabled(false); > + window.internals.setMockVideoPresentationModeEnabled(true); > + window.internals.setMediaElementRestrictions(video, "NoRestrictions"); These should all be just "internals", not "window.internals". > LayoutTests/media/media-fullscreen-pause-inline.html:23 > + window.internals.settings.setAllowsInlineMediaPlayback(false); > + window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true); > + // Disable the Fullscreen API (element fullscreen) support > + window.internals.settings.setFullScreenEnabled(false); > + window.internals.setMockVideoPresentationModeEnabled(true); > + window.internals.setMediaElementRestrictions(video, "NoRestrictions"); Ditto. > LayoutTests/media/media-fullscreen-return-to-inline.html:23 > + window.internals.settings.setAllowsInlineMediaPlayback(false); > + window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(false); > + // Disable the Fullscreen API (element fullscreen) support > + window.internals.settings.setFullScreenEnabled(false); > + window.internals.setMockVideoPresentationModeEnabled(true); Ditto.

Comment on attachment 398985 [details] WIP patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review >> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226 >> + return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture()); > > Too many parentheses here. The second set make sense, but the first set aren’t needed.

Created attachment 399044 [details] Patch for landing

Comment on attachment 398985 [details] WIP patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review >>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226 >>> + return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture()); >> >> Too many parentheses here. > > The second set make sense, but the first set aren’t needed. Right, fixed. >> LayoutTests/media/media-fullscreen-loop-inline.html:22 >> + window.internals.setMediaElementRestrictions(video, "NoRestrictions"); > > These should all be just "internals", not "window.internals". Fixed. >> LayoutTests/media/media-fullscreen-pause-inline.html:23 >> + window.internals.setMediaElementRestrictions(video, "NoRestrictions"); > > Ditto. Fixed. >> LayoutTests/media/media-fullscreen-return-to-inline.html:23 >> + window.internals.setMockVideoPresentationModeEnabled(true); > > Ditto. Fixed. > LayoutTests/media/media-fullscreen-return-to-inline.html:47 > + window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true); Replaced the "window.internals" with "internals". > LayoutTests/media/video-fullscreen-only-playback.html:23 > + window.internals.setMockVideoPresentationModeEnabled(true); Replaced "window.internals" with "internals".

Committed r261493: <https://trac.webkit.org/changeset/261493> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399044 [details].

*** Bug 175195 has been marked as a duplicate of this bug. ***

*** Bug 193399 has been marked as a duplicate of this bug. ***

*** Bug 187618 has been marked as a duplicate of this bug. ***

*** Bug 182571 has been marked as a duplicate of this bug. ***

*** Bug 137311 has been marked as a duplicate of this bug. ***

Reopen this bug to fix some build failures.

Created attachment 399060 [details] A follow-up patch to fix build failures

Committed r261501: <https://trac.webkit.org/changeset/261501> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399060 [details].

*** Bug 200957 has been marked as a duplicate of this bug. ***