[Webkit-unassigned] [Bug 58443] [GTK] Support the file system API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58443


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #137861|review?                     |review-
               Flag|                            |




--- Comment #36 from Gustavo Noronha (kov) <gns at gnome.org>  2012-04-19 16:43:34 PST ---
(From update of attachment 137861)
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(&performDidReadDirectoryEntriesAndFinishedCallback, 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.