[webkit-reviews] review granted: [Bug 86409] Site-specific hack: Disclaim WebM as a supported type on Mac for YouTube. : [Attachment 142036] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 86409: Site-specific hack: Disclaim WebM as a supported type on Mac for
YouTube.
https://bugs.webkit.org/show_bug.cgi?id=86409

Attachment 142036: Patch
https://bugs.webkit.org/attachment.cgi?id=142036&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=142036&action=review


r=me, but I am concerned about the URL vs. securityOrigin issue that Sam raised


> Source/WebCore/dom/DOMImplementation.cpp:72
> +    DOMImplementationSupportsTypeClient(bool needsHacks, String host)

Should be const String&.

> Source/WebCore/dom/DOMImplementation.cpp:74
> +    : m_needsHacks(needsHacks)
> +    , m_host(host)

We normally indent these by four spaces.

> Source/WebCore/dom/DOMImplementation.cpp:79
> +    bool mediaPlayerNeedsSiteSpecificHacks() const OVERRIDE { return
m_needsHacks; }
> +    String mediaPlayerDocumentHost() const OVERRIDE { return m_host; }

Should include the keyword virtual as well -- we are normally explicit about
that.

These two overrides can and should be private instead of public.

> Source/WebCore/html/HTMLMediaElement.cpp:4396
> +    return document()->securityOrigin()->host();

I thought Sam said this should be based on the document’s URL, not its
securityOrigin?

> Source/WebCore/platform/graphics/MediaPlayer.cpp:735
> +	   if ((host.endsWith(".youtube.com", false) ||
equalIgnoringCase("youtube.com", host))
> +	       && (contentType.type().startsWith("video/webm", false) ||
contentType.type().startsWith("video/x-flv", false)))
> +	       return IsNotSupported;

Right here is where the “why” comment belongs.

> Source/WebCore/platform/graphics/MediaPlayer.h:203
> +    static MediaPlayer::SupportsType supportsType(const ContentType&, const
String& keySystem, const MediaPlayerSupportsTypeClient*);

Could use a reference here instead of a pointer.