[webkit-reviews] review denied: [Bug 58443] [GTK] Support the file system API : [Attachment 137861] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 58443: [GTK] Support the file system API
https://bugs.webkit.org/show_bug.cgi?id=58443

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

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137861&action=review


r- just because I'd like to be sure we know what we're doing at
createObjectAndReadMetadataAsync, so I think we need another iteration.

> Source/WebCore/GNUmakefile.list.am:4999
> +webcore_sources += \
>	Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp \
>	Source/WebCore/platform/gtk/AsyncFileSystemGtk.h

Why did you move these to webcore_sources?

> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:131
> +void AsyncFileSystemCallbacksGtk::didReadDirectoryEntries(bool)
> +{
> +    for (size_t i = 0; i < m_directoryEntries.size(); ++i)
> +	   m_callbacks->didReadDirectoryEntry(m_directoryEntries[i].name,
m_directoryEntries[i].isDirectory);
> +    m_directoryEntries.clear();
> +
> +   
m_taskController->postCallbackTask(createCallbackTask(&performDidReadDirectoryE
ntriesAndFinishedCallback, m_callbacks.release()), m_taskMode);
> +}

I think you addressed my concern by removing the recursive listing, now you're
not even trying to read the directories that are found. Is that acceptable?
It's starting to look like it's what's intended from reading the spec and the
code. The spec indicates a call to readEntries returns the 'next block' of
entries, which is why there's a hasMore boolean here - you're not expected to
return all entries at once. We may want to look into that, but I don't think
it's very important right now:
http://www.w3.org/TR/file-system-api/#the-directoryreader-interface

> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:65
> +static FileError::ErrorCode validateFileType(GFile* sourceFile, GFile*
destinationFile)

validateFileTypes sounds like a better choice

> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:242
> +    if (recursive && g_file_test(filePath.get(), G_FILE_TEST_IS_DIR)) {

We can use query_file_type here too, instead of testing the path.

> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:329
> +    ASSERT(type == G_FILE_TYPE_REGULAR || type == G_FILE_TYPE_DIRECTORY);
> +    // We should not allow to access upper directory to create a new file
than base directory
> +    // because of the security issue.
> +    if (type == G_FILE_TYPE_REGULAR && absolutePath == String("/")) {
> +	   helperCallbacks->didFail(FileError::SECURITY_ERR);
> +	   return;
> +    }

I understand the security implications but I am still really doubtful we need
this check. Wouldn't higher level stuff check for someone trying to create /?
The comment as it is is not very useful, I would prefer if it explained why we
need to do this check here.

> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:342
> +    if (error) {
> +	   if (!needToReadMetadata || error->code != G_IO_ERROR_EXISTS) {
> +	       helperCallbacks->didFail(toFileErrorCode(error.get()));
> +	       return;
> +	   }
> +    }

I am not sure I understand. This is very convoluted. Why do we only complain
about the error if it's not a file exists? We should make all of these checks
in 1 if, in any case.

> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:348
> +    helperCallbacks->didSucceed();

I believe you need to report success also after reading metadata.