Bug 104284

Summary: EME v0.1: Report defaultURL in KeyMessage.
Product: WebKit Reporter: David Dorwin <ddorwin>
Component: MediaAssignee: David Dorwin <ddorwin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, jberlin, jer.noble, ojan.autocc, ojan, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

David Dorwin
Reported 2012-12-06 11:54:19 PST
EME v0.1: Report defaultURL in KeyMessage.
Attachments
Patch (11.10 KB, patch)
2012-12-06 12:00 PST, David Dorwin
no flags
Patch (12.80 KB, patch)
2012-12-07 15:51 PST, David Dorwin
no flags
David Dorwin
Comment 1 2012-12-06 12:00:05 PST
WebKit Review Bot
Comment 2 2012-12-06 12:03:34 PST
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.
Darin Fisher (:fishd, Google)
Comment 3 2012-12-07 00:02:31 PST
Comment on attachment 178051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178051&action=review > Source/WebCore/html/HTMLMediaElement.cpp:1883 > + initializer.defaultURL = defaultURL; do you need to possibly resolve defaultURL against the document's URL? can defaultURL be a relative URL? if so, consider using document's completeURL method, which conveniently returns KURL. > Source/WebKit/chromium/public/WebMediaPlayerClient.h:78 > + virtual void keyMessage(const WebString&, const WebString&, const unsigned char*, unsigned, const WebString&) = 0; nit: these parameters really should have names. parameters should have names if their names wouldn't be redundant with their typename. otherwise, it is very unclear what these parameters refer to. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:254 > +void WebMediaPlayerClientImpl::keyMessage(const WebString& keySystem, const WebString& sessionId, const unsigned char* message, unsigned messageLength, const WebString& defaultURL) URLs are usually passed as WebURL in the API and KURL internally within WebCore.
David Dorwin
Comment 4 2012-12-07 10:56:17 PST
Comment on attachment 178051 [details] Patch Thanks. I think we need to decide whether the spec should say that the URL should be validated, which I believe will determine the type. View in context: https://bugs.webkit.org/attachment.cgi?id=178051&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:1883 >> + initializer.defaultURL = defaultURL; > > do you need to possibly resolve defaultURL against the document's URL? can defaultURL be a relative URL? if so, consider using document's completeURL method, which conveniently returns KURL. This is a URL from the media data (container) [1] or the license. So, it must be absolute and is unrelated to the document. This is just a pass-through from the media to the app. In the spec, it is just a DOMString [2]. I assume that using WebURL/KURL would mean we would only pass valid URLs to the app. The spec doesn't say anything about validating it (or even that it must be a valid URL). Maybe it should. What are your thoughts from a spec point of view? I'll post this question, but I'm interested in your thoughts. [1] http://dvcs.w3.org/hg/html-media/raw-file/eme-v0.1b/encrypted-media/encrypted-media.html#dom-defaulturl [2] http://dvcs.w3.org/hg/html-media/raw-file/eme-v0.1b/encrypted-media/encrypted-media.html#dom-mediakeymessageevent >> Source/WebKit/chromium/public/WebMediaPlayerClient.h:78 >> + virtual void keyMessage(const WebString&, const WebString&, const unsigned char*, unsigned, const WebString&) = 0; > > nit: these parameters really should have names. parameters should have names if their names wouldn't be redundant with their typename. otherwise, it is very unclear what these parameters refer to. Will fix. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:254 >> +void WebMediaPlayerClientImpl::keyMessage(const WebString& keySystem, const WebString& sessionId, const unsigned char* message, unsigned messageLength, const WebString& defaultURL) > > URLs are usually passed as WebURL in the API and KURL internally within WebCore. See discussion above. Will change here if we decide to use KURL.
Darin Fisher (:fishd, Google)
Comment 5 2012-12-07 14:11:38 PST
(In reply to comment #4) > (From update of attachment 178051 [details]) > Thanks. I think we need to decide whether the spec should say that the URL should be validated, which I believe will determine the type. > > View in context: https://bugs.webkit.org/attachment.cgi?id=178051&action=review > > >> Source/WebCore/html/HTMLMediaElement.cpp:1883 > >> + initializer.defaultURL = defaultURL; > > > > do you need to possibly resolve defaultURL against the document's URL? can defaultURL be a relative URL? if so, consider using document's completeURL method, which conveniently returns KURL. > > This is a URL from the media data (container) [1] or the license. So, it must be absolute and is unrelated to the document. This is just a pass-through from the media to the app. In the spec, it is just a DOMString [2]. I assume that using WebURL/KURL would mean we would only pass valid URLs to the app. > The spec doesn't say anything about validating it (or even that it must be a valid URL). Maybe it should. What are your thoughts from a spec point of view? I'll post this question, but I'm interested in your thoughts. My thoughts are simple: It should only be called an URL if it is an URL. If it is an URL, then it is subject to URL canonicalization, etc. It should be represented using KURL / WebURL / GURL, etc. If it is just an opaque string, then it needs a different name.
David Dorwin
Comment 6 2012-12-07 15:51:20 PST
David Dorwin
Comment 7 2012-12-07 15:53:20 PST
Comment on attachment 178051 [details] Patch I confirmed that this works and only valid URLs get passed from Chromium, which uses GURL to convert the string. View in context: https://bugs.webkit.org/attachment.cgi?id=178051&action=review >>>> Source/WebCore/html/HTMLMediaElement.cpp:1883 >>>> + initializer.defaultURL = defaultURL; >>> >>> do you need to possibly resolve defaultURL against the document's URL? can defaultURL be a relative URL? if so, consider using document's completeURL method, which conveniently returns KURL. >> >> This is a URL from the media data (container) [1] or the license. So, it must be absolute and is unrelated to the document. This is just a pass-through from the media to the app. In the spec, it is just a DOMString [2]. I assume that using WebURL/KURL would mean we would only pass valid URLs to the app. >> The spec doesn't say anything about validating it (or even that it must be a valid URL). Maybe it should. What are your thoughts from a spec point of view? I'll post this question, but I'm interested in your thoughts. >> >> [1] http://dvcs.w3.org/hg/html-media/raw-file/eme-v0.1b/encrypted-media/encrypted-media.html#dom-defaulturl >> [2] http://dvcs.w3.org/hg/html-media/raw-file/eme-v0.1b/encrypted-media/encrypted-media.html#dom-mediakeymessageevent > > My thoughts are simple: It should only be called an URL if it is an URL. If it is an URL, then it is subject to URL canonicalization, etc. It should be represented using KURL / WebURL / GURL, etc. > > If it is just an opaque string, then it needs a different name. Done. >>> Source/WebKit/chromium/public/WebMediaPlayerClient.h:78 >>> + virtual void keyMessage(const WebString&, const WebString&, const unsigned char*, unsigned, const WebString&) = 0; >> >> nit: these parameters really should have names. parameters should have names if their names wouldn't be redundant with their typename. otherwise, it is very unclear what these parameters refer to. > > Will fix. Done. >>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:254 >>> +void WebMediaPlayerClientImpl::keyMessage(const WebString& keySystem, const WebString& sessionId, const unsigned char* message, unsigned messageLength, const WebString& defaultURL) >> >> URLs are usually passed as WebURL in the API and KURL internally within WebCore. > > See discussion above. Will change here if we decide to use KURL. Done.
WebKit Review Bot
Comment 8 2012-12-14 00:26:03 PST
Comment on attachment 178296 [details] Patch Clearing flags on attachment: 178296 Committed r137724: <http://trac.webkit.org/changeset/137724>
WebKit Review Bot
Comment 9 2012-12-14 00:26:07 PST
All reviewed patches have been landed. Closing bug.
Jer Noble
Comment 10 2012-12-14 09:18:33 PST
This patch caused numerous build errors in one of our internal mac build bots. In the future, please avoid adding unused parameter names to empty functions without an accompanying UNUSED_PARAM() macro.
Jer Noble
Comment 11 2012-12-14 09:21:01 PST
For reference, here are the build errors which were seen after r137724: OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:189:66: error: unused parameter 'keySystem' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:189:91: error: unused parameter 'sessionId' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:190:66: error: unused parameter 'keySystem' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:190:91: error: unused parameter 'sessionId' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:190:136: error: unused parameter 'systemCode' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:191:68: error: unused parameter 'keySystem' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:191:93: error: unused parameter 'sessionId' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:191:125: error: unused parameter 'message' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:191:143: error: unused parameter 'messageLength' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:191:170: error: unused parameter 'defaultURL' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:192:67: error: unused parameter 'keySystem' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:192:92: error: unused parameter 'sessionId' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:192:124: error: unused parameter 'initData' [-Werror,-Wunused-parameter] OpenSource/Source/WebCore/platform/graphics/MediaPlayer.h:192:143: error: unused parameter 'initDataLength' [-Werror,-Wunused-parameter] We'll fix these in an upcoming changeset.
Jessie Berlin
Comment 12 2012-12-14 09:22:34 PST
Fixed the unused parameters issue and not modifying MediaPlayerPrivateAVFoundationObjC.mm's use of keyMessage in http://trac.webkit.org/changeset/137750
Note You need to log in before you can comment on or make changes to this bug.