Bug 47310 - Implement FileEntrySync.file() in FileSystem API
Summary: Implement FileEntrySync.file() in FileSystem API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-06 15:34 PDT by Kinuko Yasuda
Modified: 2010-10-15 19:06 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.64 KB, patch)
2010-10-08 12:44 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2010-10-12 18:02 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (15.17 KB, patch)
2010-10-14 15:27 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (16.29 KB, patch)
2010-10-15 16:16 PDT, Kinuko Yasuda
jianli: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-10-06 15:34:40 PDT
Implement FileEntrySync.file() in FileSystem API
Comment 1 Kinuko Yasuda 2010-10-06 17:38:16 PDT
File::create() fails on workers on chromium because Worker's WebKitClient doesn't implement mimeTypeForExtension.  Should we just add the implementation for the method?  (That will proxy the call to the browser to access the mime registry.)
Comment 2 Eric U. 2010-10-06 18:28:45 PDT
(In reply to comment #1)
> File::create() fails on workers on chromium because Worker's WebKitClient doesn't implement mimeTypeForExtension.  Should we just add the implementation for the method?  (That will proxy the call to the browser to access the mime registry.)

Yeah, I guess it just hasn't needed it up to this point.  Unless you see an alternative?
Comment 3 Kinuko Yasuda 2010-10-08 12:44:25 PDT
Created attachment 70276 [details]
Patch
Comment 4 Jian Li 2010-10-08 13:03:07 PDT
Comment on attachment 70276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70276&action=review

> LayoutTests/ChangeLog:12
> +        * fast/filesystem/resources/file-from-file-entry.js: Added.
> +        * fast/filesystem/script-tests/file-from-file-entry.js: Removed.

Why do you move file-from-file-entry.js from script-tests/ to resources/?
Comment 5 Kinuko Yasuda 2010-10-11 12:11:37 PDT
(In reply to comment #4)
> (From update of attachment 70276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70276&action=review
> 
> > LayoutTests/ChangeLog:12
> > +        * fast/filesystem/resources/file-from-file-entry.js: Added.
> > +        * fast/filesystem/script-tests/file-from-file-entry.js: Removed.
> 
> Why do you move file-from-file-entry.js from script-tests/ to resources/?

Because I wanted to use it both for workers and non-workers tests.
Comment 6 Kinuko Yasuda 2010-10-11 12:13:36 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 70276 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=70276&action=review
> > 
> > > LayoutTests/ChangeLog:12
> > > +        * fast/filesystem/resources/file-from-file-entry.js: Added.
> > > +        * fast/filesystem/script-tests/file-from-file-entry.js: Removed.
> > 
> > Why do you move file-from-file-entry.js from script-tests/ to resources/?
> 
> Because I wanted to use it both for workers and non-workers tests.

And I've included workers and resources directories to be synced under chrome/test/data... for chromium's workers tests but haven't done that for script-tests.
Comment 7 Eric U. 2010-10-12 16:27:32 PDT
Comment on attachment 70276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70276&action=review

> LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:17
> +shouldBe("testFile.type", "'text/plain'");

Please add a TODO to check size as well, once that's implemented.  Actually, is there a bug for that yet?
Comment 8 Kinuko Yasuda 2010-10-12 18:02:19 PDT
Created attachment 70578 [details]
Patch

Updated to include the fix for 47563 (pass platform path).
Comment 9 Jian Li 2010-10-12 18:08:15 PDT
Comment on attachment 70578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70578&action=review

> WebCore/fileapi/FileEntrySync.cpp:48
> +    return File::create(filesystem()->asyncFileSystem()->virtualToPlatformPath(m_fullPath));

We're calling AsyncFileSystem::virtualToPlatformPath in FileEntrySync. This seems some sort of confusing.
Comment 10 Kinuko Yasuda 2010-10-14 15:27:07 PDT
Created attachment 70787 [details]
Patch
Comment 11 Kinuko Yasuda 2010-10-14 15:29:43 PDT
(In reply to comment #10)
> Created an attachment (id=70787) [details]
> Patch

(In reply to comment #9)
> (From update of attachment 70578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70578&action=review
> 
> > WebCore/fileapi/FileEntrySync.cpp:48
> > +    return File::create(filesystem()->asyncFileSystem()->virtualToPlatformPath(m_fullPath));
> 
> We're calling AsyncFileSystem::virtualToPlatformPath in FileEntrySync. This seems some sort of confusing.

Added DOMFileSystem{,Sync}::createFile and changed FileEntry{,Sync}::file to call that method (as in createWriter).
Comment 12 Jian Li 2010-10-15 13:49:42 PDT
Comment on attachment 70787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70787&action=review

> LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:1
> +if (this.importScripts) {

Is this resource file only used in worker testing? If so, it would be better to put under filesystem/workers/resources. In addition, you can remove the check for the existence of importScripts.

> LayoutTests/fast/filesystem/resources/file-from-file-entry.js:2
> +    importScripts('../resources/fs-worker-common.js');

Do we need to use "../resources"?

> LayoutTests/fast/filesystem/resources/file-from-file-entry.js:3
> +    importScripts('../resources/fs-test-util.js');

ditto.

> LayoutTests/fast/filesystem/workers/file-from-file-entry-sync-expected.txt:1
> +

Why do we have blank expected result here?

> WebCore/fileapi/DOMFileSystem.cpp:89
> +void DOMFileSystem::createFile(const FileEntry* file, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback>)

Please consider renaming 'file' argument to 'fileEntry' to avoid the confusion with the File being created in this method.

> WebCore/fileapi/DOMFileSystem.h:63
> +    void createFile(const FileEntry* file, PassRefPtr<FileCallback>, PassRefPtr<ErrorCallback>);

Please omit 'file' argument.

> WebCore/fileapi/DOMFileSystemSync.cpp:62
> +PassRefPtr<File> DOMFileSystemSync::createFile(const FileEntrySync* file, ExceptionCode& ec)

ditto.

> WebCore/fileapi/DOMFileSystemSync.h:59
> +    PassRefPtr<File> createFile(const FileEntrySync* file, ExceptionCode&);

ditto.
Comment 13 Kinuko Yasuda 2010-10-15 16:16:42 PDT
Created attachment 70912 [details]
Patch
Comment 14 Kinuko Yasuda 2010-10-15 16:18:25 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=70787&action=review

>> LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:1
>> +if (this.importScripts) {
> 
> Is this resource file only used in worker testing? If so, it would be better to put under filesystem/workers/resources. In addition, you can remove the check for the existence of importScripts.

Makes sense, I put a FIXME comment for now - I'd like to file a separate issue to address this and keep it as is in this patch.

There're some other worker-only resources that are put under filesystem/resources and I want to move them in sync (and it requires chromoium test code change too).

>> LayoutTests/fast/filesystem/resources/file-from-file-entry.js:2
>> +    importScripts('../resources/fs-worker-common.js');
> 
> Do we need to use "../resources"?

Fixed.

>> LayoutTests/fast/filesystem/resources/file-from-file-entry.js:3
>> +    importScripts('../resources/fs-test-util.js');
> 
> ditto.

Removed the line as this file doesn't depend on the file.

>> LayoutTests/fast/filesystem/workers/file-from-file-entry-sync-expected.txt:1
>> +
> 
> Why do we have blank expected result here?

Fixed.

>> WebCore/fileapi/DOMFileSystem.cpp:89
>> +void DOMFileSystem::createFile(const FileEntry* file, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback>)
> 
> Please consider renaming 'file' argument to 'fileEntry' to avoid the confusion with the File being created in this method.

Fixed.

>> WebCore/fileapi/DOMFileSystem.h:63
>> +    void createFile(const FileEntry* file, PassRefPtr<FileCallback>, PassRefPtr<ErrorCallback>);
> 
> Please omit 'file' argument.

Fixed.

>> WebCore/fileapi/DOMFileSystemSync.cpp:62
>> +PassRefPtr<File> DOMFileSystemSync::createFile(const FileEntrySync* file, ExceptionCode& ec)
> 
> ditto.

Fixed.

>> WebCore/fileapi/DOMFileSystemSync.h:59
>> +    PassRefPtr<File> createFile(const FileEntrySync* file, ExceptionCode&);
> 
> ditto.

Fixed.
Comment 15 Jian Li 2010-10-15 17:28:15 PDT
Comment on attachment 70912 [details]
Patch

Looks good. Please fix the issue below before you land it.

View in context: https://bugs.webkit.org/attachment.cgi?id=70912&action=review

> LayoutTests/fast/filesystem/workers/file-from-file-entry.html:7
> +<p id="description">This tests returning File from FileEntrySync in workers.</p>

FileEntrySync => FileEntry
Please also update the expected result file.
Comment 16 Kinuko Yasuda 2010-10-15 19:06:16 PDT
Committed r69907: <http://trac.webkit.org/changeset/69907>