Bug 104284 – EME v0.1: Report defaultURL in KeyMessage.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[David Dorwin]

EME v0.1: Report defaultURL in KeyMessage.

[David Dorwin]

Created attachment 178051 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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 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 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)]

(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]

Created attachment 178296 [details]
Patch

[David Dorwin]

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 on attachment 178296 [details]
Patch

Clearing flags on attachment: 178296

Committed r137724: <http://trac.webkit.org/changeset/137724>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

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]

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]

Fixed the unused parameters issue and not modifying MediaPlayerPrivateAVFoundationObjC.mm's use of keyMessage in http://trac.webkit.org/changeset/137750