[webkit-reviews] review denied: [Bug 64580] Add support for download='filename' in anchors : [Attachment 101111] Add test, remove special case for data URLs, ChangeLog

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied sadrul at chromium.org's request for
review:
Bug 64580: Add support for download='filename' in anchors
https://bugs.webkit.org/show_bug.cgi?id=64580

Attachment 101111: Add test, remove special case for data URLs, ChangeLog
https://bugs.webkit.org/attachment.cgi?id=101111&action=review

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


R-, mostly for the Origin header / FrameLoader question.  Please feel free to
renominate if I'm not understanding things properly.  Thanks for adding tests
in this iteration!

> Source/WebCore/html/HTMLAnchorElement.idl:29
> +	   attribute [Reflect] DOMString download;

Doe this API need to be conditional on something?  Do all the ports understand
how to use the suggested file name?  From reading the code, it seems like this
feature might be half-implemented on most ports because it will trigger the
download but not suggest the file name.  Maybe I'm misunderstanding?

> Source/WebCore/loader/FrameLoader.cpp:1243
> +    if (!referrer.isEmpty()) {
> +	   request.setHTTPReferrer(referrer);
> +	   RefPtr<SecurityOrigin> referrerOrigin =
SecurityOrigin::createFromString(referrer);
> +	   addHTTPOriginIfNeeded(request, referrerOrigin->toString());
> +    } else

Why are we adding an Origin header to GET requests?  Normally we don't do that
unless we're using CORS.  This request doesn't seem to be a CORS request,
however, because it's not using the CORS functions for preparing the request
(which do things like remove authentication information).

> Source/WebCore/loader/FrameLoader.h:275
> +    void prepareResourceRequest(ResourceRequest&, ReferrerPolicy);

This function name is very confusing.  Should we call this function all the
time when sending a request, or only in some circumstances?  With a name like
"prepareResourceRequest" it sounds like we should call this all the time
because we always want to prepare resource requests before issuing them.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1329
> -void FrameLoaderClient::startDownload(const ResourceRequest& request)
> +void FrameLoaderClient::startDownload(const ResourceRequest& request, const
String& suggestedName)

For example, on GTK, this looks like this function triggers the download but
ignores the suggestedName.