Bug 60583

Summary:

DOMFileSystemBase should not impose file naming restrictions on 'external' filesystems

Product:

WebKit

Reporter:

Zelidrag Hornung <zelidrag>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, ericu, eric, kinuko, levin, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Other

Attachments:

Description	Flags
Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems	eric: review-
Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems	none

Zelidrag Hornung

Reported 2011-05-10 14:34:54 PDT

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.

Attachments
Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems (2.71 KB, patch) 2011-05-10 14:54 PDT, Zelidrag Hornung	eric: review-	Details Formatted Diff Diff
Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems (2.97 KB, patch) 2011-05-11 12:32 PDT, Zelidrag Hornung	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Eric U.

Comment 1 2011-05-10 14:37:05 PDT

Agreed; that seems like the kind of thing that should really be delegated.

Zelidrag Hornung

Comment 2 2011-05-10 14:54:42 PDT

Created attachment 93012 [details] Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems

Kinuko Yasuda

Comment 3 2011-05-10 22:04:45 PDT

Comment on attachment 93012 [details] Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems lgtm, thanks.

Eric Seidel (no email)

Comment 4 2011-05-10 22:24:45 PDT

Kinuko Yasuda

Comment 5 2011-05-10 22:34:09 PDT

Comment on attachment 93012 [details] 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.

Kinuko Yasuda

Comment 6 2011-05-10 23:02:43 PDT

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

Zelidrag Hornung

Comment 7 2011-05-11 12:32:02 PDT

Created attachment 93156 [details] Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems addressed comments from the previous review cycle

Zelidrag Hornung

Comment 8 2011-05-11 13:29:31 PDT

Comment on attachment 93012 [details] 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

WebKit Commit Bot

Comment 9 2011-05-11 17:28:13 PDT

The commit-queue encountered the following flaky tests while processing attachment 93156 [details]: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 10 2011-05-11 17:29:46 PDT

Comment on attachment 93156 [details] Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems Clearing flags on attachment: 93156 Committed r86291: <http://trac.webkit.org/changeset/86291>

WebKit Commit Bot

Comment 11 2011-05-11 17:29:51 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 12 2011-05-11 19:41:55 PDT

http://trac.webkit.org/changeset/86291 might have broken GTK Linux 32-bit Release

Note You need to log in before you can comment on or make changes to this bug.