[webkit-reviews] review granted: [Bug 82973] Add keySystem attribute to canPlayType() for Encrypted Media Extensions : [Attachment 136351] Restored feature define guards around MediaEngineSupportsType so platforms not supporting the feature do not need to be modified.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 136351: Restored feature define guards around
MediaEngineSupportsType so platforms not supporting the feature do not need to
be modified.
https://bugs.webkit.org/attachment.cgi?id=136351&action=review

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


Looks like you've got some tests to clean up before we can land this patch.

> Source/WebCore/dom/DOMImplementation.cpp:390
> +    if (MediaPlayer::supportsType(ContentType(type), emptyString()))

By the way, you might want to use String() rather than emptyString().  The
former is the null string whereas the latter is the empty string.

> Source/WebCore/html/HTMLMediaElement.cpp:841
>	   ContentType contentType("");
> -	   loadResource(mediaURL, contentType);
> +	   // No key system information is available either.
> +	   loadResource(mediaURL, contentType, emptyString());

It's a super minor point, but your might want to match this pattern here (as
well as improve the contentType line to use emptyString() ).

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:677
> +    String keySystem;

Notice that here you're using the null string rather than the empty string. 
There probably isn't much of a difference, but if you're OCD like me, you might
want to make it consistent.