Bug 60583 - DOMFileSystemBase should not impose file naming restrictions on 'external' filesystems
Summary: DOMFileSystemBase should not impose file naming restrictions on 'external' fi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-10 14:34 PDT by Zelidrag Hornung
Modified: 2011-05-11 19:41 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Zelidrag Hornung 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.
Comment 1 Eric U. 2011-05-10 14:37:05 PDT
Agreed; that seems like the kind of thing that should really be delegated.
Comment 2 Zelidrag Hornung 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
Comment 3 Kinuko Yasuda 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.
Comment 4 Eric Seidel (no email) 2011-05-10 22:24:45 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
Comment 5 Kinuko Yasuda 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.
Comment 6 Kinuko Yasuda 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.
Comment 7 Zelidrag Hornung 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
Comment 8 Zelidrag Hornung 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
Comment 9 WebKit Commit Bot 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.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2011-05-11 17:29:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2011-05-11 19:41:55 PDT
http://trac.webkit.org/changeset/86291 might have broken GTK Linux 32-bit Release