Bug 215240 – r262456 broke sites that expect webkitDisplayingFullscreen to be true almost immediately

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Some JavaScript libraries expect webkitDisplayingFullscreen to return true immediately after calling webkitEnterFullscreen.

[Eric Carlson]

<rdar://problem/66284042>

[Eric Carlson]

Created attachment 406125 [details]
Patch

[Eric Carlson]

Created attachment 406126 [details]
Patch

[youenn fablet]

Comment on attachment 406126 [details]
Patch

Looks like a nice simplification but iOS sim is not happy.
Let's rerun the test to see whether these were flaky failures.

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

> Source/WebCore/html/HTMLVideoElement.cpp:355
> +    return isFullscreen();

We could inline it in the header file.

> LayoutTests/media/presentationmodechanged-fired-once.html:22
> +			}, 15000);

Indentation is not correct.

> LayoutTests/media/presentationmodechanged-fired-once.html:53
> +

Unneeded.

> LayoutTests/media/video-presentation-mode.html:20
> +                }, 15000);

This is hit now by the iOSsim bot.

[Peng Liu]

Comment on attachment 406126 [details]
Patch

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

>> Source/WebCore/html/HTMLVideoElement.cpp:355
>> +    return isFullscreen();
> 
> We could inline it in the header file.

Just realized a problem here. With this change, some layout tests may send requests to change presentation mode before the transition is done. Unfortunately, those requests will be ignore by the video element and then end with a test failure/timeout.

> LayoutTests/media/presentationmodechanged-fired-once.html:20
> +    		setTimeout( () => {

Nit. The space is not needed here.

[Eric Carlson]

Created attachment 406255 [details]
Patch

[Darin Adler]

Are we also doing the evangelism side of this to try to get the Akamai script fixed?

[Darin Adler]

Comment on attachment 406255 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:989
> +#if PLATFORM(IOS_FAMILY)

I’d like to see a comment documenting a tiny bit about why we need the quirk and predicting/describing when we can remove it. Can likely be a short comment without tons of details, but should not require looking back at the ChangeLog.

> Source/WebCore/page/Quirks.cpp:1000
> +    return classNames.contains("akamai-html5") && classNames.contains("akamai-media-element");

Could use _s here to make the AtomString construction slightly more efficient. Or use static const AtomString so we make the atom strings only once the first time this function is called.

> Source/WebCore/page/Quirks.h:114
> +    bool needsAkamaiMediaPlayerQuirk(const HTMLElement&) const;

Why not have this function take an HTMLVideoElement instead of an HTMLElement? The is<HTMLVideoElement> check seems wasteful.

[Eric Carlson]

Created attachment 406278 [details]
Patch for landing

[Eric Carlson]

Created attachment 406301 [details]
Patch for landing

[Eric Carlson]

Comment on attachment 406255 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/page/Quirks.cpp:989
>> +#if PLATFORM(IOS_FAMILY)
> 
> I’d like to see a comment documenting a tiny bit about why we need the quirk and predicting/describing when we can remove it. Can likely be a short comment without tons of details, but should not require looking back at the ChangeLog.

Fixed.

>> Source/WebCore/page/Quirks.cpp:1000
>> +    return classNames.contains("akamai-html5") && classNames.contains("akamai-media-element");
> 
> Could use _s here to make the AtomString construction slightly more efficient. Or use static const AtomString so we make the atom strings only once the first time this function is called.

Great point, changed to use const AtomStrings.

>> Source/WebCore/page/Quirks.h:114
>> +    bool needsAkamaiMediaPlayerQuirk(const HTMLElement&) const;
> 
> Why not have this function take an HTMLVideoElement instead of an HTMLElement? The is<HTMLVideoElement> check seems wasteful.

Fixed.

[EWS]

Committed r265437: <https://trac.webkit.org/changeset/265437>

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

[Peng Liu]

Reopening to attach new patch.

[Peng Liu]

Created attachment 412131 [details]
A follow-up patch

[Peng Liu]

Will file a new bug to land the follow-up patch. Closing this bug.