Bug 60583 – DOMFileSystemBase should not impose file naming restrictions on 'external' filesystems

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Zelidrag Hornung]

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.

[Eric U.]

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

[Zelidrag Hornung]

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 on attachment 93012 [details]
Prevented DOMFileSystemBase from imposing file naming restrictions on external file systems

lgtm, thanks.

[Eric Seidel (no email)]

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

[Kinuko Yasuda]

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]

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

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

The commit-queue encountered the following flaky tests while processing attachment 93156 [details]:

http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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