Bug 211645 – Enable the mock video presentation mode in related layout tests and fix test failures

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Enable mock video presentation mode in layout test media/media-fullscreen-loop-inline.html

[Peng Liu]

Created attachment 398943 [details]
WIP patch

[Radar WebKit Bug Importer]

<rdar://problem/63057695>

[Peng Liu]

Created attachment 398960 [details]
WIP patch v2

[Peng Liu]

Created attachment 398979 [details]
WIP patch v3

[Peng Liu]

Created attachment 398985 [details]
WIP patch v4

[Darin Adler]

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?

[Peng Liu]

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.

[Darin Adler]

(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?

[Peng Liu]

Correct.

[Peng Liu]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Peng Liu]

Created attachment 399044 [details]
Patch for landing

[Peng Liu]

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".

[EWS]

Committed r261493: <https://trac.webkit.org/changeset/261493>

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

[Peng Liu]

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

[Peng Liu]

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

[Peng Liu]

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

[Peng Liu]

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

[Peng Liu]

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

[Peng Liu]

Reopen this bug to fix some build failures.

[Peng Liu]

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

[EWS]

Committed r261501: <https://trac.webkit.org/changeset/261501>

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

[Peng Liu]

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