60583 2011-05-10 14:34:54 -0700 DOMFileSystemBase should not impose file naming restrictions on 'external' filesystems 2011-05-11 19:41:55 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Other Other RESOLVED FIXED P2 Normal --- 0 zelidrag webkit-unassigned abarth commit-queue ericu eric kinuko levin webkit.review.bot oldest_to_newest 401505 0 zelidrag 2011-05-10 14:34:54 -0700 DOMFileSystemBase should not perform file name validation (see http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#naming-restrictions for details) in case of an external file system. External file system provider implementations will take care of such validations if necessary. 401509 1 ericu 2011-05-10 14:37:05 -0700 Agreed; that seems like the kind of thing that should really be delegated. 401525 2 93012 zelidrag 2011-05-10 14:54:42 -0700 Created attachment 93012 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems 401755 3 93012 kinuko 2011-05-10 22:04:45 -0700 Comment on attachment 93012 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems lgtm, thanks. 401769 4 93012 eric 2011-05-10 22:24:45 -0700 Comment on attachment 93012 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems View in context: https://bugs.webkit.org/attachment.cgi?id=93012&action=review > Source/WebCore/ChangeLog:10 > + No new tests. (OOPS!) This will make the commit-queue fail. You should explain why there are no new tests (is testing impossible here?) or you should add tests. Also, the FileSystem feature should be announced on webkit-dev per the new feature guidelines: http://www.webkit.org/coding/adding-features.html 401776 5 93012 kinuko 2011-05-10 22:34:09 -0700 Comment on attachment 93012 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems View in context: https://bugs.webkit.org/attachment.cgi?id=93012&action=review > Source/WebCore/fileapi/DOMFileSystemBase.cpp:148 > + if (type != AsyncFileSystem::External && !DOMFilePath::isValidPath(absolutePath)) if ((type == Temporary || type == Persistent) && !DOMFilePath::isValidPath()) might be better? This is for basic checks that are explicitly noted in the spec (draft), therefore performing the checks for FS types that are spec'ed out would make more sense. 401786 6 kinuko 2011-05-10 23:02:43 -0700 (In reply to comment #4) > (From update of attachment 93012 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93012&action=review > > > Source/WebCore/ChangeLog:10 > > + No new tests. (OOPS!) > > This will make the commit-queue fail. You should explain why there are no new tests (is testing impossible here?) or you should add tests. > > Also, the FileSystem feature should be announced on webkit-dev per the new feature guidelines: > http://www.webkit.org/coding/adding-features.html Oh, is the guideline applied to the features that are already being experimented/tested? Well it'd be better to make this experimental feature more visible and to request more comments... Ok I'll be sending out an announce to webkit-dev. 402164 7 93156 zelidrag 2011-05-11 12:32:02 -0700 Created attachment 93156 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems addressed comments from the previous review cycle 402216 8 93012 zelidrag 2011-05-11 13:29:31 -0700 Comment on attachment 93012 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems View in context: https://bugs.webkit.org/attachment.cgi?id=93012&action=review >>> Source/WebCore/ChangeLog:10 >>> + No new tests. (OOPS!) >> >> This will make the commit-queue fail. You should explain why there are no new tests (is testing impossible here?) or you should add tests. >> >> Also, the FileSystem feature should be announced on webkit-dev per the new feature guidelines: >> http://www.webkit.org/coding/adding-features.html > > Oh, is the guideline applied to the features that are already being experimented/tested? Well it'd be better to make this experimental feature more visible and to request more comments... > Ok I'll be sending out an announce to webkit-dev. done >> Source/WebCore/fileapi/DOMFileSystemBase.cpp:148 >> + if (type != AsyncFileSystem::External && !DOMFilePath::isValidPath(absolutePath)) > > if ((type == Temporary || type == Persistent) && !DOMFilePath::isValidPath()) > > might be better? This is for basic checks that are explicitly noted in the spec (draft), therefore performing the checks for FS types that are spec'ed out would make more sense. done 402466 9 commit-queue 2011-05-11 17:28:13 -0700 The commit-queue encountered the following flaky tests while processing attachment 93156: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch. 402470 10 93156 commit-queue 2011-05-11 17:29:46 -0700 Comment on attachment 93156 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems Clearing flags on attachment: 93156 Committed r86291: <http://trac.webkit.org/changeset/86291> 402471 11 commit-queue 2011-05-11 17:29:51 -0700 All reviewed patches have been landed. Closing bug. 402563 12 webkit.review.bot 2011-05-11 19:41:55 -0700 http://trac.webkit.org/changeset/86291 might have broken GTK Linux 32-bit Release 93012 2011-05-10 14:54:42 -0700 2011-05-11 13:29:31 -0700 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems wk_60583.diff text/plain 2772 zelidrag SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg2MTgxKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDUtMTAgIFplbGlkcmFn IEhvcm51bmcgIDx6ZWxpZHJhZ0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUHJldmVudGVkIERPTUZpbGVTeXN0ZW1CYXNlIGZy b20gaW1wb3NpbmcgZmlsZSBuYW1pbmcgcmVzdHJpY3Rpb25zIG9uCisgICAgICAgIGV4dGVybmFs IGZpbGUgc3lzdGVtcy4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9NjA1ODMKKworICAgICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAg ICAqIGZpbGVhcGkvRE9NRmlsZVN5c3RlbUJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6cGF0 aFRvQWJzb2x1dGVQYXRoKToKKyAgICAgICAgKFdlYkNvcmU6OkRPTUZpbGVTeXN0ZW1CYXNlOjpn ZXRGaWxlKToKKyAgICAgICAgKFdlYkNvcmU6OkRPTUZpbGVTeXN0ZW1CYXNlOjpnZXREaXJlY3Rv cnkpOgorCiAyMDExLTA1LTEwICBCcmFkeSBFaWRzb24gIDxiZWlkc29uQGFwcGxlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBBbGljZSBMaXUuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9maWxl YXBpL0RPTUZpbGVTeXN0ZW1CYXNlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9m aWxlYXBpL0RPTUZpbGVTeXN0ZW1CYXNlLmNwcAkocmV2aXNpb24gODYxMzMpCisrKyBTb3VyY2Uv V2ViQ29yZS9maWxlYXBpL0RPTUZpbGVTeXN0ZW1CYXNlLmNwcAkod29ya2luZyBjb3B5KQpAQCAt MTM3LDcgKzEzNyw3IEBAIHN0YXRpYyBib29sIHZlcmlmeUFuZEdldERlc3RpbmF0aW9uUGF0aEYK ICAgICByZXR1cm4gdHJ1ZTsKIH0KIAotc3RhdGljIGJvb2wgcGF0aFRvQWJzb2x1dGVQYXRoKGNv bnN0IEVudHJ5QmFzZSogYmFzZSwgU3RyaW5nIHBhdGgsIFN0cmluZyYgYWJzb2x1dGVQYXRoKQor c3RhdGljIGJvb2wgcGF0aFRvQWJzb2x1dGVQYXRoKEFzeW5jRmlsZVN5c3RlbTo6VHlwZSB0eXBl LCBjb25zdCBFbnRyeUJhc2UqIGJhc2UsIFN0cmluZyBwYXRoLCBTdHJpbmcmIGFic29sdXRlUGF0 aCkKIHsKICAgICBBU1NFUlQoYmFzZSk7CiAKQEAgLTE0NSw3ICsxNDUsNyBAQCBzdGF0aWMgYm9v bCBwYXRoVG9BYnNvbHV0ZVBhdGgoY29uc3QgRW50CiAgICAgICAgIHBhdGggPSBET01GaWxlUGF0 aDo6YXBwZW5kKGJhc2UtPmZ1bGxQYXRoKCksIHBhdGgpOwogICAgIGFic29sdXRlUGF0aCA9IERP TUZpbGVQYXRoOjpyZW1vdmVFeHRyYVBhcmVudFJlZmVyZW5jZXMocGF0aCk7CiAKLSAgICBpZiAo IURPTUZpbGVQYXRoOjppc1ZhbGlkUGF0aChhYnNvbHV0ZVBhdGgpKQorICAgIGlmICh0eXBlICE9 IEFzeW5jRmlsZVN5c3RlbTo6RXh0ZXJuYWwgJiYgIURPTUZpbGVQYXRoOjppc1ZhbGlkUGF0aChh YnNvbHV0ZVBhdGgpKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgcmV0dXJuIHRydWU7CiB9 CkBAIC0yMDgsNyArMjA4LDcgQEAgYm9vbCBET01GaWxlU3lzdGVtQmFzZTo6Z2V0UGFyZW50KGNv bnN0IAogYm9vbCBET01GaWxlU3lzdGVtQmFzZTo6Z2V0RmlsZShjb25zdCBFbnRyeUJhc2UqIGJh c2UsIGNvbnN0IFN0cmluZyYgcGF0aCwgUGFzc1JlZlB0cjxXZWJLaXRGbGFncz4gZmxhZ3MsIFBh c3NSZWZQdHI8RW50cnlDYWxsYmFjaz4gc3VjY2Vzc0NhbGxiYWNrLCBQYXNzUmVmUHRyPEVycm9y Q2FsbGJhY2s+IGVycm9yQ2FsbGJhY2spCiB7CiAgICAgU3RyaW5nIGFic29sdXRlUGF0aDsKLSAg ICBpZiAoIXBhdGhUb0Fic29sdXRlUGF0aChiYXNlLCBwYXRoLCBhYnNvbHV0ZVBhdGgpKQorICAg IGlmICghcGF0aFRvQWJzb2x1dGVQYXRoKG1fYXN5bmNGaWxlU3lzdGVtLT50eXBlKCksIGJhc2Us IHBhdGgsIGFic29sdXRlUGF0aCkpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIAogICAgIFN0cmlu ZyBwbGF0Zm9ybVBhdGggPSBtX2FzeW5jRmlsZVN5c3RlbS0+dmlydHVhbFRvUGxhdGZvcm1QYXRo KGFic29sdXRlUGF0aCk7CkBAIC0yMjMsNyArMjIzLDcgQEAgYm9vbCBET01GaWxlU3lzdGVtQmFz ZTo6Z2V0RmlsZShjb25zdCBFbgogYm9vbCBET01GaWxlU3lzdGVtQmFzZTo6Z2V0RGlyZWN0b3J5 KGNvbnN0IEVudHJ5QmFzZSogYmFzZSwgY29uc3QgU3RyaW5nJiBwYXRoLCBQYXNzUmVmUHRyPFdl YktpdEZsYWdzPiBmbGFncywgUGFzc1JlZlB0cjxFbnRyeUNhbGxiYWNrPiBzdWNjZXNzQ2FsbGJh Y2ssIFBhc3NSZWZQdHI8RXJyb3JDYWxsYmFjaz4gZXJyb3JDYWxsYmFjaykKIHsKICAgICBTdHJp bmcgYWJzb2x1dGVQYXRoOwotICAgIGlmICghcGF0aFRvQWJzb2x1dGVQYXRoKGJhc2UsIHBhdGgs IGFic29sdXRlUGF0aCkpCisgICAgaWYgKCFwYXRoVG9BYnNvbHV0ZVBhdGgobV9hc3luY0ZpbGVT eXN0ZW0tPnR5cGUoKSwgYmFzZSwgcGF0aCwgYWJzb2x1dGVQYXRoKSkKICAgICAgICAgcmV0dXJu IGZhbHNlOwogCiAgICAgU3RyaW5nIHBsYXRmb3JtUGF0aCA9IG1fYXN5bmNGaWxlU3lzdGVtLT52 aXJ0dWFsVG9QbGF0Zm9ybVBhdGgoYWJzb2x1dGVQYXRoKTsK 93156 2011-05-11 12:32:02 -0700 2011-05-11 17:29:46 -0700 Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems wk_60583.diff text/plain 3046 zelidrag SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg2MTgxKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTEtMDUtMTAgIFplbGlkcmFn IEhvcm51bmcgIDx6ZWxpZHJhZ0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUHJldmVudGVkIERPTUZpbGVTeXN0ZW1CYXNlIGZy b20gaW1wb3NpbmcgZmlsZSBuYW1pbmcgcmVzdHJpY3Rpb25zIG9uCisgICAgICAgIGV4dGVybmFs IGZpbGUgc3lzdGVtcy4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9NjA1ODMKKworICAgICAgICBUaGVyZSBhcmUgbm8gbmV3IHRlc3RzIGZvciB0aGlz IGNoYW5nZSBzaW5jZSBpdCByZXF1aXJlcyB0aGUgcHJlc2VuY2UKKyAgICAgICAgb2YgZXh0ZXJu YWwgZmlsZSBzeXN0ZW0gcHJvdmlkZXIgd2hpY2ggaXMgc3BlY2lmaWMgdG8gdGhlIHBsYXRmb3Jt IGhvc3QKKyAgICAgICAgcHJvY2Vzcy4gQ2hyb21pdW0gaW1wbGVtZW50YXRpb24gd2lsbCB0ZXN0 IHRoaXMgd2l0aGluIGl0cyBleHRlbnNpb24gQVBJCisgICAgICAgIHRlc3Qgc3VpdGUuCisKKyAg ICAgICAgKiBmaWxlYXBpL0RPTUZpbGVTeXN0ZW1CYXNlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OnBhdGhUb0Fic29sdXRlUGF0aCk6CisgICAgICAgIChXZWJDb3JlOjpET01GaWxlU3lzdGVtQmFz ZTo6Z2V0RmlsZSk6CisgICAgICAgIChXZWJDb3JlOjpET01GaWxlU3lzdGVtQmFzZTo6Z2V0RGly ZWN0b3J5KToKKwogMjAxMS0wNS0xMCAgQnJhZHkgRWlkc29uICA8YmVpZHNvbkBhcHBsZS5jb20+ CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWxpY2UgTGl1LgpJbmRleDogU291cmNlL1dlYkNvcmUv ZmlsZWFwaS9ET01GaWxlU3lzdGVtQmFzZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvZmlsZWFwaS9ET01GaWxlU3lzdGVtQmFzZS5jcHAJKHJldmlzaW9uIDg2MTMzKQorKysgU291 cmNlL1dlYkNvcmUvZmlsZWFwaS9ET01GaWxlU3lzdGVtQmFzZS5jcHAJKHdvcmtpbmcgY29weSkK QEAgLTEzNyw3ICsxMzcsNyBAQCBzdGF0aWMgYm9vbCB2ZXJpZnlBbmRHZXREZXN0aW5hdGlvblBh dGhGCiAgICAgcmV0dXJuIHRydWU7CiB9CiAKLXN0YXRpYyBib29sIHBhdGhUb0Fic29sdXRlUGF0 aChjb25zdCBFbnRyeUJhc2UqIGJhc2UsIFN0cmluZyBwYXRoLCBTdHJpbmcmIGFic29sdXRlUGF0 aCkKK3N0YXRpYyBib29sIHBhdGhUb0Fic29sdXRlUGF0aChBc3luY0ZpbGVTeXN0ZW06OlR5cGUg dHlwZSwgY29uc3QgRW50cnlCYXNlKiBiYXNlLCBTdHJpbmcgcGF0aCwgU3RyaW5nJiBhYnNvbHV0 ZVBhdGgpCiB7CiAgICAgQVNTRVJUKGJhc2UpOwogCkBAIC0xNDUsNyArMTQ1LDcgQEAgc3RhdGlj IGJvb2wgcGF0aFRvQWJzb2x1dGVQYXRoKGNvbnN0IEVudAogICAgICAgICBwYXRoID0gRE9NRmls ZVBhdGg6OmFwcGVuZChiYXNlLT5mdWxsUGF0aCgpLCBwYXRoKTsKICAgICBhYnNvbHV0ZVBhdGgg PSBET01GaWxlUGF0aDo6cmVtb3ZlRXh0cmFQYXJlbnRSZWZlcmVuY2VzKHBhdGgpOwogCi0gICAg aWYgKCFET01GaWxlUGF0aDo6aXNWYWxpZFBhdGgoYWJzb2x1dGVQYXRoKSkKKyAgICBpZiAoKHR5 cGUgPT0gQXN5bmNGaWxlU3lzdGVtOjpUZW1wb3JhcnkgfHwgdHlwZSA9PSBBc3luY0ZpbGVTeXN0 ZW06OlBlcnNpc3RlbnQpICYmICFET01GaWxlUGF0aDo6aXNWYWxpZFBhdGgoYWJzb2x1dGVQYXRo KSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgIHJldHVybiB0cnVlOwogfQpAQCAtMjA4LDcg KzIwOCw3IEBAIGJvb2wgRE9NRmlsZVN5c3RlbUJhc2U6OmdldFBhcmVudChjb25zdCAKIGJvb2wg RE9NRmlsZVN5c3RlbUJhc2U6OmdldEZpbGUoY29uc3QgRW50cnlCYXNlKiBiYXNlLCBjb25zdCBT dHJpbmcmIHBhdGgsIFBhc3NSZWZQdHI8V2ViS2l0RmxhZ3M+IGZsYWdzLCBQYXNzUmVmUHRyPEVu dHJ5Q2FsbGJhY2s+IHN1Y2Nlc3NDYWxsYmFjaywgUGFzc1JlZlB0cjxFcnJvckNhbGxiYWNrPiBl cnJvckNhbGxiYWNrKQogewogICAgIFN0cmluZyBhYnNvbHV0ZVBhdGg7Ci0gICAgaWYgKCFwYXRo VG9BYnNvbHV0ZVBhdGgoYmFzZSwgcGF0aCwgYWJzb2x1dGVQYXRoKSkKKyAgICBpZiAoIXBhdGhU b0Fic29sdXRlUGF0aChtX2FzeW5jRmlsZVN5c3RlbS0+dHlwZSgpLCBiYXNlLCBwYXRoLCBhYnNv bHV0ZVBhdGgpKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAgICBTdHJpbmcgcGxhdGZvcm1Q YXRoID0gbV9hc3luY0ZpbGVTeXN0ZW0tPnZpcnR1YWxUb1BsYXRmb3JtUGF0aChhYnNvbHV0ZVBh dGgpOwpAQCAtMjIzLDcgKzIyMyw3IEBAIGJvb2wgRE9NRmlsZVN5c3RlbUJhc2U6OmdldEZpbGUo Y29uc3QgRW4KIGJvb2wgRE9NRmlsZVN5c3RlbUJhc2U6OmdldERpcmVjdG9yeShjb25zdCBFbnRy eUJhc2UqIGJhc2UsIGNvbnN0IFN0cmluZyYgcGF0aCwgUGFzc1JlZlB0cjxXZWJLaXRGbGFncz4g ZmxhZ3MsIFBhc3NSZWZQdHI8RW50cnlDYWxsYmFjaz4gc3VjY2Vzc0NhbGxiYWNrLCBQYXNzUmVm UHRyPEVycm9yQ2FsbGJhY2s+IGVycm9yQ2FsbGJhY2spCiB7CiAgICAgU3RyaW5nIGFic29sdXRl UGF0aDsKLSAgICBpZiAoIXBhdGhUb0Fic29sdXRlUGF0aChiYXNlLCBwYXRoLCBhYnNvbHV0ZVBh dGgpKQorICAgIGlmICghcGF0aFRvQWJzb2x1dGVQYXRoKG1fYXN5bmNGaWxlU3lzdGVtLT50eXBl KCksIGJhc2UsIHBhdGgsIGFic29sdXRlUGF0aCkpCiAgICAgICAgIHJldHVybiBmYWxzZTsKIAog ICAgIFN0cmluZyBwbGF0Zm9ybVBhdGggPSBtX2FzeW5jRmlsZVN5c3RlbS0+dmlydHVhbFRvUGxh dGZvcm1QYXRoKGFic29sdXRlUGF0aCk7Cg==