Bug 215240

<rdar://problem/66284042>

Created attachment 406125 [details]
Patch

Created attachment 406126 [details]
Patch

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.

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.

Created attachment 406255 [details]
Patch

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

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.

Created attachment 406278 [details]
Patch for landing

Created attachment 406301 [details]
Patch for landing

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.

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

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

Reopening to attach new patch.

Created attachment 412131 [details]
A follow-up patch

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