Summary: | Implement FileEntrySync.file() in FileSystem API | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ericu, jianli, levin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Kinuko Yasuda
2010-10-06 15:34:40 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.) (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? Created attachment 70276 [details]
Patch
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/? (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. (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 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? Created attachment 70578 [details]
Patch
Updated to include the fix for 47563 (pass platform path).
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. Created attachment 70787 [details]
Patch
(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 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. Created attachment 70912 [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. 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 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. Committed r69907: <http://trac.webkit.org/changeset/69907> |