[webkit-reviews] review granted: [Bug 78614] Set Referrer header for media downloads : [Attachment 127441] Proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 78614: Set Referrer header for media downloads
https://bugs.webkit.org/show_bug.cgi?id=78614

Attachment 127441: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=127441&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127441&action=review


Shouldn't the test have custom results on some platforms now? Or is it already
skipped there?

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

Please mention that a test was modified to cover this.

> Source/WebCore/html/HTMLMediaElement.cpp:3825
> +	   return emptyString();

Probably doesn't matter here, but I'd have returned a null string here,
signaling that we don't have a value.

>>> Source/WebCore/html/HTMLMediaElement.cpp:3827
>>> +	 return
SecurityPolicy::generateReferrerHeader(document()->referrerPolicy(),
KURL(ParsedURLString, url), frame->loader()->outgoingReferrer());
>> 
>> I tried to follow the code, but couldn't find what guarantees that the url
argument is a result of calling string() on a valid KURL. This is the only case
when ParsedURLString-style constructor can be used.
> 
> HTMLMediaElement::loadResource passes a valid KURL to MediaPlayer::load, 
MediaPlayer::loadWithNextMediaEngine passes url.string() to the media engine.

OK. I wish compiler could enforce this (there was some talk about adding
ParsedURLString class for tis purpose).

> Source/WebCore/platform/graphics/MediaPlayer.cpp:939
> +    if (m_mediaPlayerClient)
> +	   return m_mediaPlayerClient->mediaPlayerReferrer(url);
> +    return emptyString();

Null string here, too, and we usually prefer early return to nesting normal
code path.

> Source/WebCore/platform/graphics/MediaPlayer.h:171
> +    virtual String mediaPlayerReferrer(const String&) const { return
emptyString(); }

Null string again (what are the subclasses that don't need to implement?)

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:265
> +    if (!referrer.isEmpty()) {

This check should remain isEmpty even if you change other places to make null
strings. I'm not really sure why, but it seems like common pattern.

> LayoutTests/ChangeLog:9
> +	       anything bug video-referer.html.

s/bug/but/