Bug 82971

Created attachment 135355 [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.

David, would you be willing to send an email to webkit-dev announcing this new feature? http://www.webkit.org/coding/adding-features.html Thanks.

@abarth: Workiing on it. Thanks.

Comment on attachment 135355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135355&action=review This patch looks pretty good. A few minor comments below. > Source/WebCore/ChangeLog:10 > + Add Encrypted Media Extensions methods to HTMLMediaElement > + https://bugs.webkit.org/show_bug.cgi?id=82971 > + > + Reviewed by NOBODY (OOPS!). > + > + Tests: media/encrypted-media/encrypted-media-not-loaded.html > + media/encrypted-media/encrypted-media-syntax.html > + Please add some more information to the ChangeLog. For example, you might mention the ENABLE(ENCRYPTED_MEDIA) macro and reference the spec that you're implementing. > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:210 > + static bool webkitEncryptedMediaEnabled() { return isEncryptedMediaEnabled; } > + static void setWebkitEncryptedMediaEnabled(bool isEnabled) { isEncryptedMediaEnabled = isEnabled; } We can drop the "webkit" prefix here. (I know other features use the "webkit" prefix, but they shouldn't.) > Source/WebCore/html/HTMLMediaElement.cpp:2241 > +ExceptionCode exceptionCodeForMediaKeyException(MediaPlayer::MediaKeyException exception) Please mark this function as "static" so that it has internal linkage. Also, we tend to put functions like this near the top of the file. > Source/WebCore/html/HTMLMediaElement.cpp:2252 > + default: > + ASSERT_NOT_REACHED(); > + return INVALID_STATE_ERR; WebKit tends to leave off the "default" case so that the compiler complains if we forget an enum value. > Source/WebCore/html/HTMLMediaElement.cpp:2270 > + if (initData.get()) { There's no need to call ".get()" here. There's an implicit conversion to bool. > Source/WebCore/html/HTMLMediaElement.cpp:2303 > + if (initData.get()) { ditto > Source/WebCore/html/HTMLMediaElement.cpp:2387 > - > + Generally, we don't like to trim trailing whitespace on unrelated lines because it messes up "svn blame". Would you be willing to remove these parts of the patch? > Source/WebCore/html/HTMLMediaElement.idl:130 > + This change seems unrelated. > LayoutTests/platform/mac/Skipped:480 > +fast/events/constructors/media-key-event-constructor.html I don't see this test in this patch... Maybe it's in a later patch?

Created attachment 136320 [details] Patch

Comment on attachment 135355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135355&action=review >> Source/WebCore/ChangeLog:10 >> + > > Please add some more information to the ChangeLog. For example, you might mention the ENABLE(ENCRYPTED_MEDIA) macro and reference the spec that you're implementing. Added that information. >> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:210 >> + static void setWebkitEncryptedMediaEnabled(bool isEnabled) { isEncryptedMediaEnabled = isEnabled; } > > We can drop the "webkit" prefix here. (I know other features use the "webkit" prefix, but they shouldn't.) Done >> Source/WebCore/html/HTMLMediaElement.cpp:2241 >> +ExceptionCode exceptionCodeForMediaKeyException(MediaPlayer::MediaKeyException exception) > > Please mark this function as "static" so that it has internal linkage. Also, we tend to put functions like this near the top of the file. Done >> Source/WebCore/html/HTMLMediaElement.cpp:2252 >> + return INVALID_STATE_ERR; > > WebKit tends to leave off the "default" case so that the compiler complains if we forget an enum value. Done >> Source/WebCore/html/HTMLMediaElement.cpp:2270 >> + if (initData.get()) { > > There's no need to call ".get()" here. There's an implicit conversion to bool. Done >> Source/WebCore/html/HTMLMediaElement.cpp:2303 >> + if (initData.get()) { > > ditto Done >> Source/WebCore/html/HTMLMediaElement.cpp:2387 >> + > > Generally, we don't like to trim trailing whitespace on unrelated lines because it messes up "svn blame". Would you be willing to remove these parts of the patch? Done. >> Source/WebCore/html/HTMLMediaElement.idl:130 >> + > > This change seems unrelated. Done >> LayoutTests/platform/mac/Skipped:480 >> +fast/events/constructors/media-key-event-constructor.html > > I don't see this test in this patch... Maybe it's in a later patch? Correct. It's added in https://bugs.webkit.org/show_bug.cgi?id=82974. I was trying to avoid editing these same 6 files again, but I can remove it if you prefer.

Created attachment 136338 [details] Patch

Comment on attachment 136338 [details] Patch Thanks.

Comment on attachment 136338 [details] Patch Clearing flags on attachment: 136338 Committed r113736: <http://trac.webkit.org/changeset/113736>

All reviewed patches have been landed. Closing bug.