Bug 47310 – Implement FileEntrySync.file() in FileSystem API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Implement FileEntrySync.file() in FileSystem API

[Kinuko Yasuda]

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

[Eric U.]

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

[Kinuko Yasuda]

Created attachment 70276 [details]
Patch

[Jian Li]

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/?

[Kinuko Yasuda]

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

[Kinuko Yasuda]

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

[Eric U.]

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?

[Kinuko Yasuda]

Created attachment 70578 [details]
Patch

Updated to include the fix for 47563 (pass platform path).

[Jian Li]

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.

[Kinuko Yasuda]

Created attachment 70787 [details]
Patch

[Kinuko Yasuda]

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

[Jian Li]

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.

[Kinuko Yasuda]

Created attachment 70912 [details]
Patch

[Kinuko Yasuda]

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.

[Jian Li]

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.

[Kinuko Yasuda]

Committed r69907: <http://trac.webkit.org/changeset/69907>