[webkit-reviews] review denied: [Bug 82973] Add keySystem attribute to canPlayType() for Encrypted Media Extensions : [Attachment 135360] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied David Dorwin <ddorwin at chromium.org>'s
request for review:
Bug 82973: Add keySystem attribute to canPlayType() for Encrypted Media
Extensions
https://bugs.webkit.org/show_bug.cgi?id=82973

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=135360&action=review


I'm having trouble reading this patch because of all the ifdefs.  Rather than
adding all these ifdefs, we should just add the "system" parameter everywhere. 
We'll probably need an ifdef at the start of the call chain (e.g., when reading
the attribute out of the DOM), but otherwise we shouldn't need so many ifdefs.

> Source/Platform/chromium/public/WebMimeRegistry.h:52
> +						  const WebKit::WebString&
keySystem
> +						  ) {

These two lines should be combined.

> Source/Platform/chromium/public/WebMimeRegistry.h:53
> +	 return supportsMediaMIMEType(mimeType, codecs);

WebKit uses four-space indent.

> Source/WebCore/dom/DOMImplementation.cpp:394
> +#if ENABLE(ENCRYPTED_MEDIA)
> +    // Key system is not applicable here.
> +    if (MediaPlayer::supportsType(ContentType(type), emptyString()))
> +#else
> +    if (MediaPlayer::supportsType(ContentType(type)))
> +#endif

Rather than adding ENABLE(ENCRYPTED_MEDIA) everywhere, we should just add the
extra parameter and pass around an empty (or null) string.

> Source/WebCore/html/HTMLMediaElement.cpp:2890
> +KURL HTMLMediaElement::selectNextSourceChild(ContentType *contentType,
String *keySystem, InvalidURLAction actionIfInvalid)

ContentType *contentType => ContentType* contentType
String *keySystem => String* keySystem

> Source/WebCore/html/HTMLMediaElement.cpp:2959
> +	   // FIXME(82965): Add support for keySystem in <source>.

FIXME(82965): -> FIXME:

(We can add a link to the bug to the comment if you like.)

> Source/WebCore/html/HTMLMediaElement.cpp:2960
> +	   // system = source->system();

Generally, we don't have commented out code.  It's fine to just have a FIXME
comment that links to a bug.