[webkit-reviews] review denied: [Bug 24683] XHR in Workers doesn't set the referrer correctly. : [Attachment 82980] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 24683: XHR in Workers doesn't set the referrer correctly.
https://bugs.webkit.org/show_bug.cgi?id=24683

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82980&action=review

I'm not in love with this patch.  It's not really clear when it's safe to pass
an empty originating URL.

I've wanted to add a mechanism like this for a long time.  Another place we
need something like this so that data URLs can inherit the origin of their
requestor.  Given that we can't extend the ResourceRequest structure (due to
sadness in the Mac port), the approach I had looked at before was to create a
structure that contained both the resource request and the origin information. 
At some level, that's just a syntactic change from what you've done in this
patch (which just passes the two next to each other), but that seems like a
better road forward.

Concretely, if you don't want to introduce the larger structure (which will
likely require some refactoring), it might be worth looking at whether we can
avoid using an empty KURL as an originating URL.  For example, if you
disentangle createOutgoingReferrer from FrameLoader, then you can rename
originatingURL to outgoingReferrer and do the URL tweaking on the worker
thread.

I'm going to mark this R- for now, but if you think I'm way off base, please
feel free to re-nominate it.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:72
> -DocumentThreadableLoader::DocumentThreadableLoader(Document* document,
ThreadableLoaderClient* client, BlockingBehavior blockingBehavior, const
ResourceRequest& request, const ThreadableLoaderOptions& options)
> +DocumentThreadableLoader::DocumentThreadableLoader(Document* document,
ThreadableLoaderClient* client, BlockingBehavior blockingBehavior, const
ResourceRequest& request, const ThreadableLoaderOptions& options, const KURL&
originatingURL)

What is an originatingURL ?  That's like the referrer but without the fragment
(and friends) removed?

> Source/WebCore/loader/FrameLoader.cpp:644
> -void FrameLoader::setOutgoingReferrer(const KURL& url)
> +KURL FrameLoader::createOutgoingReferrer(const KURL& url)
>  {
>      KURL outgoingReferrer(url);
>      outgoingReferrer.setUser(String());
>      outgoingReferrer.setPass(String());
>      outgoingReferrer.removeFragmentIdentifier();
> -    m_outgoingReferrer = outgoingReferrer.string();
> +    return outgoingReferrer;
> +}

Should this be a method on KURL?  Maybe a free function somewhere?  It seems
unrelated to FrameLoader now.