[webkit-reviews] review denied: [Bug 47310] Implement FileEntrySync.file() in FileSystem API : [Attachment 70787] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 47310: Implement FileEntrySync.file() in FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=47310

Attachment 70787: Patch
https://bugs.webkit.org/attachment.cgi?id=70787&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
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.