Expose "suggested filename" for a resource based on its resource response. In radar as <rdar://problem/8894125>
Created attachment 79784 [details] Patch v1
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.
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>.
(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!