Bug 52916 – Expose "suggested filename" for a resource based on its resource response.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Expose "suggested filename" for a resource based on its resource response.

In radar as <rdar://problem/8894125>

[Brady Eidson]

Created attachment 79784 [details]
Patch v1

[WebKit Review Bot]

Attachment 79784 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebFrame.h:112:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 79784 [details]
Patch v1

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(WebCore::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.

(In reply to comment #3)
> So we don't get to assume that all strings from WKURLRefs can be used with KURL's ParsedURLStringTag constructor?

No. See for example <http://trac.webkit.org/changeset/75355>.

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 79784 [details])
> 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?

I'll go with "ForResourceWithURL", because this would work on main resources, as well.
> 
> Are the "Ref" suffixes really needed on the parameters?

Talk to Sam and Anders.  :)

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:208
> > +    return toCopiedAPI(toImpl(frameRef)->suggestedFilenameForResourceURL(WebCore::KURL(WebCore::KURL(), toImpl(urlRef)->string())));
> 
> So we don't get to assume that all strings from WKURLRefs can be used with KURL's ParsedURLStringTag constructor?

Correct, we don't.  Sad, but true.

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

lol.  yup.

> > 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.

Sure.

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

Style bot got me already.

Thanks!