[webkit-reviews] review granted: [Bug 52916] Expose "suggested filename" for a resource based on its resource response. : [Attachment 79784] Patch v1

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 52916: Expose "suggested filename" for a resource based on its resource
response.
https://bugs.webkit.org/show_bug.cgi?id=52916

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79784&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:206
> +WKStringRef
WKBundleFrameCopySuggestedFilenameForResourceURL(WKBundleFrameRef frameRef,
WKURLRef urlRef)

Maybe "...ForSubresourceWithURL" would be more accurate?

Are the "Ref" suffixes really needed on the parameters?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:208
> +    return
toCopiedAPI(toImpl(frameRef)->suggestedFilenameForResourceURL(WebCore::KURL(Web
Core::KURL(), toImpl(urlRef)->string())));

So we don't get to assume that all strings from WKURLRefs can be used with
KURL's ParsedURLStringTag constructor?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.h:62
> +WK_EXPORT WKStringRef
WKBundleFrameCopySuggestedFilenameForResourceURL(WKBundleFrameRef page,
WKURLRef url);

"page" seems wrong.

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:538
> +    if (DocumentLoader* loader = m_coreFrame->loader()->documentLoader()) {
> +	   if (RefPtr<ArchiveResource> subresource = loader->subresource(url))
> +	       return subresource->response().suggestedFilename();
> +    }

I think early returns would be nicer.

> Source/WebKit2/WebProcess/WebPage/WebFrame.h:112
> +    String suggestedFilenameForResourceURL(const WebCore::KURL& url) const;

No need for "url" here.