Bug 82973

Created attachment 135360 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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

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

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.

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.

Created attachment 136329 [details] Patch

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

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

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

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

Created attachment 136339 [details] fixed typo in ChangeLogs

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

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

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

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

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

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

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.

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)

Created attachment 136712 [details] Use null String

Comment on attachment 136712 [details] Use null String Look great. Thanks!

Comment on attachment 136712 [details] Use null String Passed all EWS now that 82971 has landed.

Comment on attachment 136712 [details] Use null String Clearing flags on attachment: 136712 Committed r113914: <http://trac.webkit.org/changeset/113914>

All reviewed patches have been landed. Closing bug.