EME v0.1: Report defaultURL in KeyMessage.
Created attachment 178051 [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 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.
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.
(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.
Created attachment 178296 [details] Patch
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.
Comment on attachment 178296 [details] Patch Clearing flags on attachment: 178296 Committed r137724: <http://trac.webkit.org/changeset/137724>
All reviewed patches have been landed. Closing bug.
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.
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.
Fixed the unused parameters issue and not modifying MediaPlayerPrivateAVFoundationObjC.mm's use of keyMessage in http://trac.webkit.org/changeset/137750