[webkit-reviews] review denied: [Bug 104284] EME v0.1: Report defaultURL in KeyMessage. : [Attachment 178051] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied David Dorwin
<ddorwin at chromium.org>'s request for review:
Bug 104284: EME v0.1: Report defaultURL in KeyMessage.
https://bugs.webkit.org/show_bug.cgi?id=104284

Attachment 178051: Patch
https://bugs.webkit.org/attachment.cgi?id=178051&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.