Bug 82973 – Add keySystem attribute to canPlayType() for Encrypted Media Extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[David Dorwin]

Add keySystem attribute to canPlayType() per http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media.html#extensions.

[David Dorwin]

Created attachment 135360 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 135360 [details]
Patch

Attachment 135360 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12319337

New failing tests:
media/encrypted-media/encrypted-media-can-play-type-webm.html
media/encrypted-media/encrypted-media-can-play-type.html

[WebKit Review Bot]

Created attachment 135393 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 135360 [details]
Patch

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.

[David Dorwin]

Comment on attachment 135360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135360&action=review

>> Source/Platform/chromium/public/WebMimeRegistry.h:52
>> +                                               ) {
> 
> These two lines should be combined.

Done

>> Source/Platform/chromium/public/WebMimeRegistry.h:53
>> +      return supportsMediaMIMEType(mimeType, codecs);
> 
> WebKit uses four-space indent.

Done

>> Source/WebCore/dom/DOMImplementation.cpp:394
>> +#endif
> 
> Rather than adding ENABLE(ENCRYPTED_MEDIA) everywhere, we should just add the extra parameter and pass around an empty (or null) string.

I removed most of them. Let me know if more or less should be removed.

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

Done

>> 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.)

I did this at eric.carlson's request. As discussed offline, I'm leaving as is.

>> 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.

Moved to comment.

[David Dorwin]

Created attachment 136329 [details]
Patch

[Philippe Normand]

Comment on attachment 136329 [details]
Patch

Attachment 136329 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12369399

[Early Warning System Bot]

Comment on attachment 136329 [details]
Patch

Attachment 136329 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12374250

[Early Warning System Bot]

Comment on attachment 136329 [details]
Patch

Attachment 136329 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12369401

[Build Bot]

Comment on attachment 136329 [details]
Patch

Attachment 136329 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12367994

[David Dorwin]

Created attachment 136339 [details]
fixed typo in ChangeLogs

[Philippe Normand]

Comment on attachment 136339 [details]
fixed typo in ChangeLogs

Attachment 136339 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12371482

[Early Warning System Bot]

Comment on attachment 136339 [details]
fixed typo in ChangeLogs

Attachment 136339 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12376028

[Early Warning System Bot]

Comment on attachment 136339 [details]
fixed typo in ChangeLogs

Attachment 136339 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12371503

[David Dorwin]

Created attachment 136351 [details]
Restored feature define guards around MediaEngineSupportsType so platforms not supporting the feature do not need to be modified.

[WebKit Review Bot]

Comment on attachment 136351 [details]
Restored feature define guards around MediaEngineSupportsType so platforms not supporting the feature do not need to be modified.

Attachment 136351 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12370605

New failing tests:
media/encrypted-media/encrypted-media-can-play-type-webm.html
media/encrypted-media/encrypted-media-can-play-type.html

[WebKit Review Bot]

Created attachment 136401 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 136351 [details]
Restored feature define guards around MediaEngineSupportsType so platforms not supporting the feature do not need to be modified.

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.

[David Dorwin]

Comment on attachment 136351 [details]
Restored feature define guards around MediaEngineSupportsType so platforms not supporting the feature do not need to be modified.

View in context: https://bugs.webkit.org/attachment.cgi?id=136351&action=review

>> 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.

Done

>> Source/WebCore/html/HTMLMediaElement.cpp:841
>> +        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() ).

Done (both plus below)

[David Dorwin]

Created attachment 136712 [details]
Use null String

[Adam Barth]

Comment on attachment 136712 [details]
Use null String

Look great.  Thanks!

[David Dorwin]

Comment on attachment 136712 [details]
Use null String

Passed all EWS now that 82971 has landed.

[WebKit Review Bot]

Comment on attachment 136712 [details]
Use null String

Clearing flags on attachment: 136712

Committed r113914: <http://trac.webkit.org/changeset/113914>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.