[webkit-reviews] review granted: [Bug 215240] r262456 broke sites that expect webkitDisplayingFullscreen to be true almost immediately : [Attachment 406255] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 215240: r262456 broke sites that expect webkitDisplayingFullscreen to be
true almost immediately
https://bugs.webkit.org/show_bug.cgi?id=215240

Attachment 406255: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 406255
  --> https://bugs.webkit.org/attachment.cgi?id=406255
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.