Summary: | Add Encrypted Media Extensions events to HTMLMediaElement | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Dorwin <ddorwin> | ||||||||||||||
Component: | Media | Assignee: | David Dorwin <ddorwin> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, haraken, jamesr, japhet, ojan, scherkus, simon.fraser, tkent+wkapi, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 82968 | ||||||||||||||||
Attachments: |
|
Description
David Dorwin
2012-04-02 16:44:21 PDT
Created attachment 135394 [details]
Patch
Comment on attachment 135394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135394&action=review All changes require a ChangeLog (See http://www.webkit.org/coding/contributing.html) > Source/WebCore/bindings/v8/Dictionary.cpp:302 > +bool Dictionary::get(const String& key, RefPtr<Uint8Array>& value) const This function should be outside the ENABLE(ENCRYPTED_MEDIA) ifdef. > Source/WebCore/html/HTMLMediaElement.cpp:1742 > + default: > + ASSERT_NOT_REACHED(); > + return; Generally WebKit leaves off the default case so that the compiler will complain if we forget an enum. > Source/WebCore/html/HTMLMediaElement.cpp:1765 > + initializer.message = messageRef; You should either say "messageRef.release() or just move the call to Uint8Array::create to this line. > Source/WebCore/html/HTMLMediaElement.cpp:1780 > + initializer.initData = initDataRef; ditto > Source/WebCore/html/MediaKeyEvent.cpp:42 > + You've got an extra blank line here. > Source/WebKit/chromium/public/WebMediaPlayerClient.h:71 > + virtual void keyAdded(const WebKit::WebString&, const WebKit::WebString&) = 0; > + virtual void keyError(const WebKit::WebString&, const WebKit::WebString&, MediaKeyErrorCode, unsigned short systemCode) = 0; > + virtual void keyMessage(const WebKit::WebString&, const WebKit::WebString&, const unsigned char*, unsigned) = 0; > + virtual void keyNeeded(const WebKit::WebString&, const WebKit::WebString&, const unsigned char* initData, unsigned initDataLength) = 0; We're already in the WebKit namespace, so you shouldn't need the "WebKit::" in WebKit::WebString Comment on attachment 135394 [details]
Patch
Very minor comments above. The main problem is just the procedural issue of needing a ChangeLog.
We should have fishd look at the WebKit API parts of this patch (and likely of the other patch as well).
Comment on attachment 135394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135394&action=review >> Source/WebCore/bindings/v8/Dictionary.cpp:302 >> +bool Dictionary::get(const String& key, RefPtr<Uint8Array>& value) const > > This function should be outside the ENABLE(ENCRYPTED_MEDIA) ifdef. Done >> Source/WebCore/html/HTMLMediaElement.cpp:1742 >> + return; > > Generally WebKit leaves off the default case so that the compiler will complain if we forget an enum. Done. Without it, we will continue through the rest of this function. That could be handled, but it adds more code. > Source/WebCore/html/HTMLMediaElement.cpp:1749 > + initializer.errorCode = errorCodeRef; Changed this one too. >> Source/WebCore/html/HTMLMediaElement.cpp:1765 >> + initializer.message = messageRef; > > You should either say "messageRef.release() or just move the call to Uint8Array::create to this line. Done >> Source/WebCore/html/HTMLMediaElement.cpp:1780 >> + initializer.initData = initDataRef; > > ditto Done. >> Source/WebCore/html/MediaKeyEvent.cpp:42 >> + > > You've got an extra blank line here. Done >> Source/WebKit/chromium/public/WebMediaPlayerClient.h:71 >> + virtual void keyNeeded(const WebKit::WebString&, const WebKit::WebString&, const unsigned char* initData, unsigned initDataLength) = 0; > > We're already in the WebKit namespace, so you shouldn't need the "WebKit::" in WebKit::WebString Done Created attachment 136344 [details]
Patch
Created attachment 136782 [details]
Rebased after 82973 landed
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 136782 [details]
Rebased after 82973 landed
This looks good on a first pass, but I'll need to go through it in more detail to actually review it. (Other reviewers should feel free to review it in the meantime.)
Comment on attachment 136782 [details] Rebased after 82973 landed Attachment 136782 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12393057 Created attachment 136797 [details]
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case)
Comment on attachment 136797 [details] Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case) View in context: https://bugs.webkit.org/attachment.cgi?id=136797&action=review This looks pretty straightforward. Thanks. > Source/WebCore/html/MediaKeyEvent.idl:37 > + readonly attribute [InitializedByEventConstructor] DOMString defaultUrl; Is this really supposed to be defaultUrl and not defaultURL ? I though capitalizing URL was idiomatic in the platform. (I could be misremembering.) Comment on attachment 136797 [details] Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case) View in context: https://bugs.webkit.org/attachment.cgi?id=136797&action=review >> Source/WebCore/html/MediaKeyEvent.idl:37 >> + readonly attribute [InitializedByEventConstructor] DOMString defaultUrl; > > Is this really supposed to be defaultUrl and not defaultURL ? I though capitalizing URL was idiomatic in the platform. (I could be misremembering.) Looking at other WebKit IDL files, there is a mixture of capitalizations but most are "URL". The proposed spec uses the former but not for any good reason. I'll change it here and file a bug against the proposed spec. Created attachment 136983 [details]
defaultUrl => defaultURL
Created attachment 136985 [details]
Fixed ChangeLog
Comment on attachment 136985 [details] Fixed ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=136985&action=review > LayoutTests/fast/js/resources/js-test-pre.js:257 > +function shouldBeZero(_a) { shouldBe(_a, "0"); } By the way, I really appreciate you improving this core scripts rather than making one-off solutions for just your tests. Comment on attachment 136985 [details] Fixed ChangeLog Clearing flags on attachment: 136985 Committed r114067: <http://trac.webkit.org/changeset/114067> All reviewed patches have been landed. Closing bug. This changed the results of the fast/dom/Window/window-property-descriptors.html test, which is now broken on the bots. Simon, which bots? I clicked a few and didn't see that test failing. Can you provide a link? Thanks. http://build.webkit.org/results/Lion%20Release%20(Tests)/r114135%20(7573)/fast/dom/Window/window-property-descriptors-pretty-diff.html Looks like this just needs a new baseline. It's picking up the new functions you added to the test harness. |