52916 2011-01-21 13:47:30 -0800 Expose "suggested filename" for a resource based on its resource response. 2011-06-18 11:53:18 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 beidson webkit-unassigned darin mitz webkit.review.bot oldest_to_newest 338307 0 beidson 2011-01-21 13:47:30 -0800 Expose "suggested filename" for a resource based on its resource response. In radar as <rdar://problem/8894125> 338309 1 79784 beidson 2011-01-21 13:49:25 -0800 Created attachment 79784 Patch v1 338312 2 webkit.review.bot 2011-01-21 13:52:40 -0800 Attachment 79784 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. 338313 3 79784 aroben 2011-01-21 13:54:01 -0800 Comment on attachment 79784 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. 338317 4 mitz 2011-01-21 13:56:45 -0800 (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>. 338318 5 beidson 2011-01-21 13:57:36 -0800 (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! 79784 2011-01-21 13:49:25 -0800 2011-01-21 13:54:01 -0800 Patch v1 patch.txt text/plain 4219 beidson SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDc2MzkwKQorKysgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjEgQEAKKzIwMTEtMDEtMjEgIEJyYWR5IEVp ZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS84ODk0MTI1PiBhbmQgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUyOTE2CisgICAgICAgIEV4cG9zZSAic3Vn Z2VzdGVkIGZpbGVuYW1lIiBmb3IgYSByZXNvdXJjZSBiYXNlZCBvbiBpdHMgcmVzb3VyY2UgcmVz cG9uc2UuCisKKyAgICAgICAgQVBJIHBpZWNlczoKKyAgICAgICAgKiBXZWJQcm9jZXNzL0luamVj dGVkQnVuZGxlL0FQSS9jL1dLQnVuZGxlRnJhbWUuY3BwOgorICAgICAgICAoV0tCdW5kbGVGcmFt ZUNvcHlTdWdnZXN0ZWRGaWxlbmFtZUZvclJlc291cmNlVVJMKToKKyAgICAgICAgKiBXZWJQcm9j ZXNzL0luamVjdGVkQnVuZGxlL0FQSS9jL1dLQnVuZGxlRnJhbWUuaDoKKyAgICAgICAgCisgICAg ICAgIEltcGxlbWVudGF0aW9uOgorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFt ZS5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYkZyYW1lOjpzdWdnZXN0ZWRGaWxlbmFtZUZvclJl c291cmNlVVJMKTogU2VlIGlmIHRoZSBEb2N1bWVudExvYWRlciBoYXMKKyAgICAgICAgICBhIHJl c291cmNlIGZvciB0aGlzIFVSTCBhbmQsIGlmIHNvLCByZXR1cm4gdGhlIHJlc3BvbnNlJ3Mgc3Vn Z2VzdGVkIGZpbGVuYW1lLgorICAgICAgICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5o OgorCiAyMDExLTAxLTIxICBTYW0gV2VpbmlnICA8c2FtQHdlYmtpdC5vcmc+CiAKICAgICAgICAg UmV2aWV3ZWQgYnkgQW5kZXJzIENhcmxzc29uLgpJbmRleDogU291cmNlL1dlYktpdDIvV2ViUHJv Y2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZUZyYW1lLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0luamVjdGVkQnVuZGxlL0FQSS9jL1dLQnVuZGxl RnJhbWUuY3BwCShyZXZpc2lvbiA3NjI2MCkKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3Mv SW5qZWN0ZWRCdW5kbGUvQVBJL2MvV0tCdW5kbGVGcmFtZS5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTIwMiwzICsyMDIsOCBAQCBXS19FWFBPUlQgV0tTaXplIFdLQnVuZGxlRnJhbWVHZXRTY3JvbGxP CiAgICAgCiAgICAgcmV0dXJuIHRvQVBJKHZpZXctPnNjcm9sbE9mZnNldCgpKTsKIH0KKworV0tT dHJpbmdSZWYgV0tCdW5kbGVGcmFtZUNvcHlTdWdnZXN0ZWRGaWxlbmFtZUZvclJlc291cmNlVVJM KFdLQnVuZGxlRnJhbWVSZWYgZnJhbWVSZWYsIFdLVVJMUmVmIHVybFJlZikKK3sKKyAgICByZXR1 cm4gdG9Db3BpZWRBUEkodG9JbXBsKGZyYW1lUmVmKS0+c3VnZ2VzdGVkRmlsZW5hbWVGb3JSZXNv dXJjZVVSTChXZWJDb3JlOjpLVVJMKFdlYkNvcmU6OktVUkwoKSwgdG9JbXBsKHVybFJlZiktPnN0 cmluZygpKSkpOworfQpJbmRleDogU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3RlZEJ1 bmRsZS9BUEkvYy9XS0J1bmRsZUZyYW1lLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkvYy9XS0J1bmRsZUZyYW1lLmgJKHJldmlzaW9u IDc2MjYwKQorKysgU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9JbmplY3RlZEJ1bmRsZS9BUEkv Yy9XS0J1bmRsZUZyYW1lLmgJKHdvcmtpbmcgY29weSkKQEAgLTU5LDYgKzU5LDggQEAgV0tfRVhQ T1JUIFdLUmVjdCBXS0J1bmRsZUZyYW1lR2V0Q29udGVudAogV0tfRVhQT1JUIFdLUmVjdCBXS0J1 bmRsZUZyYW1lR2V0VmlzaWJsZUNvbnRlbnRCb3VuZHMoV0tCdW5kbGVGcmFtZVJlZiBmcmFtZSk7 CiBXS19FWFBPUlQgV0tTaXplIFdLQnVuZGxlRnJhbWVHZXRTY3JvbGxPZmZzZXQoV0tCdW5kbGVG cmFtZVJlZiBmcmFtZSk7CiAKK1dLX0VYUE9SVCBXS1N0cmluZ1JlZiBXS0J1bmRsZUZyYW1lQ29w eVN1Z2dlc3RlZEZpbGVuYW1lRm9yUmVzb3VyY2VVUkwoV0tCdW5kbGVGcmFtZVJlZiBwYWdlLCBX S1VSTFJlZiB1cmwpOworCiAjaWZkZWYgX19jcGx1c3BsdXMKIH0KICNlbmRpZgpJbmRleDogU291 cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL1dlYkZyYW1lLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvV2ViRnJhbWUuY3BwCShyZXZpc2lv biA3NjI2MCkKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTM4LDYgKzM4LDcgQEAKICNpbmNsdWRlIDxKYXZhU2NyaXB0 Q29yZS9KU0xvY2suaD4KICNpbmNsdWRlIDxKYXZhU2NyaXB0Q29yZS9KU1ZhbHVlUmVmLmg+CiAj aW5jbHVkZSA8V2ViQ29yZS9BbmltYXRpb25Db250cm9sbGVyLmg+CisjaW5jbHVkZSA8V2ViQ29y ZS9BcmNoaXZlUmVzb3VyY2UuaD4KICNpbmNsdWRlIDxXZWJDb3JlL0NTU0NvbXB1dGVkU3R5bGVE ZWNsYXJhdGlvbi5oPgogI2luY2x1ZGUgPFdlYkNvcmUvQ2hyb21lLmg+CiAjaW5jbHVkZSA8V2Vi Q29yZS9Eb2N1bWVudExvYWRlci5oPgpAQCAtNTI2LDQgKzUyNywxOSBAQCBTdHJpbmcgV2ViRnJh bWU6OnByb3Zpc2lvbmFsVVJMKCkgY29uc3QKICAgICByZXR1cm4gbV9jb3JlRnJhbWUtPmxvYWRl cigpLT5wcm92aXNpb25hbERvY3VtZW50TG9hZGVyKCktPnVybCgpLnN0cmluZygpOwogfQogCitT dHJpbmcgV2ViRnJhbWU6OnN1Z2dlc3RlZEZpbGVuYW1lRm9yUmVzb3VyY2VVUkwoY29uc3QgS1VS TCYgdXJsKSBjb25zdAoreworICAgIGlmICghbV9jb3JlRnJhbWUpCisgICAgICAgIHJldHVybiBT dHJpbmcoKTsKKworICAgIGlmIChEb2N1bWVudExvYWRlciogbG9hZGVyID0gbV9jb3JlRnJhbWUt PmxvYWRlcigpLT5kb2N1bWVudExvYWRlcigpKSB7CisgICAgICAgIGlmIChSZWZQdHI8QXJjaGl2 ZVJlc291cmNlPiBzdWJyZXNvdXJjZSA9IGxvYWRlci0+c3VicmVzb3VyY2UodXJsKSkKKyAgICAg ICAgICAgIHJldHVybiBzdWJyZXNvdXJjZS0+cmVzcG9uc2UoKS5zdWdnZXN0ZWRGaWxlbmFtZSgp OworICAgIH0KKyAgICAKKyAgICAvLyBJZiB3ZSBjYW4ndCBxdWVyeSB0aGUgUmVzb3VyY2VSZXNw b25zZSBmb3IgdGhlIHN1Z2dlc3RlZCBmaWxlbmFtZSwgcmV0dXJuIGFuIGVtcHR5IHN0cmluZyBh bmQgbGV0IHRoZSBhcHAKKyAgICAvLyBjaG9vc2UgaXRzIG93biBiYWNrdXAgYmFzZWQgb2ZmIHRo ZSBVUkwuCisgICAgcmV0dXJuIFN0cmluZygpOworfQorCiB9IC8vIG5hbWVzcGFjZSBXZWJLaXQK SW5kZXg6IFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5oCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5oCShy ZXZpc2lvbiA3NjI2MCkKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJG cmFtZS5oCSh3b3JraW5nIGNvcHkpCkBAIC0xMDksNiArMTA5LDcgQEAgcHVibGljOgogICAgIGJv b2wgYWxsb3dzRm9sbG93aW5nTGluayhjb25zdCBXZWJDb3JlOjpLVVJMJikgY29uc3Q7CiAKICAg ICBTdHJpbmcgcHJvdmlzaW9uYWxVUkwoKSBjb25zdDsKKyAgICBTdHJpbmcgc3VnZ2VzdGVkRmls ZW5hbWVGb3JSZXNvdXJjZVVSTChjb25zdCBXZWJDb3JlOjpLVVJMJiB1cmwpIGNvbnN0OwogCiAg ICAgLy8gU2ltcGxlIGxpc3RlbmVyIGNsYXNzIHVzZWQgYnkgcGx1Zy1pbnMgdG8ga25vdyB3aGVu IGZyYW1lcyBmaW5pc2ggb3IgZmFpbCBsb2FkaW5nLgogICAgIGNsYXNzIExvYWRMaXN0ZW5lciB7 Cg==