[webkit-reviews] review denied: [Bug 58443] [GTK] Support the file system API : [Attachment 136422] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Apr 15 10:33:45 PDT 2012
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 136422: Patch
https://bugs.webkit.org/attachment.cgi?id=136422&action=review
------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136422&action=review
> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:127
> +void AsyncFileSystemCallbacksGtk::didReadDirectoryEntries(bool hasMore)
This hasMore parameter seems to be completely useless. It doesn't seem to be
used outside of this class, and is used in the others because of the way they
manage the list of directories that still need to be read is different.
> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:136
> + Vector<DirectoryEntry>::iterator end = m_directoryEntries.end();
> + for (Vector<DirectoryEntry>::iterator it = m_directoryEntries.begin();
it != end; ++it)
> + m_callbacks->didReadDirectoryEntry(it->name, it->isDirectory);
> +
> + m_directoryEntries.clear();
This makes mse wonder. The way we're handling things is when we find a
directory, we call didReadDirectoryEntry with isDirectory as true, and append
the entry to m_directoryEntries. But we got the end iterator before the loop.
I'm pretty sure the end iterator we got there won't be moved as new entries are
added, so we will end up clearing them here and not reading them, am I missing
something? Would using the strategy adopted by Chromium be any better? It seems
simpler.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:84
> + if (originString == String("null"))
> + return String();
> +
In addition to this check we should also have an ASSERT and check for type not
being one we're expecting.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:92
> + if (type() == Temporary)
> + result.append(temporaryPathPrefix);
> + if (type() == Persistent)
> + result.append(persistentPathPrefix);
This would be best as else if. You can use an ASSERT + empty return on the else
clause if you prefer that instead of checking/asserting above.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:103
> + GRefPtr<GFileInfo> sourceInfo =
adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0,
0));
We don't need to query all attributes if we're only going to use name, and
type. Make this "standard::*".
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:109
> + GRefPtr<GFileInfo> destinationInfo =
adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE,
0, 0));
Ditto.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:115
> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR);
Would type mismatch make more sense here?
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-typemismatcherror
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:120
> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR);
How about PathExistsError for this one?
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:126
> + helperCallbacks->didFail(FileError::NOT_FOUND_ERR);
This would be much better if we actually used the error to check what happened
and were able to report a more informative error.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:147
> + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(sourceDirectory), 0,
error));
This does not use GIO... I am here pondering if that may be problematic in the
future, but I don't think there are any reasons to believe source/destination
here would be outside of the main file system, is there? Still I would prefer
if we used g_file_enumerate_children instead for this function, how's that?
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:155
> + GRefPtr<GFileInfo> sourceInfo =
adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0,
error));
Looks like we only need G_FILE_ATTRIBUTE_STANDARD_TYPE here.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:160
> + return;
Hmm. So we found a directory, then we copy recursively and return? This would
make no sense if we're trying to copy everything. You probably forgot to do the
error check before returning?
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:179
> + GRefPtr<GFileInfo> destinationInfo =
adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE,
0, 0));
G_FILE_ATTRIBUTE_STANDARD_TYPE seems to be enough here as well.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:191
> + if (destinationInfo) {
> + GFileType destinationFileType =
g_file_info_get_file_type(destinationInfo.get());
> + if (sourceFileType != destinationFileType) {
> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR);
> + return;
> + }
> + if (destinationFileType == G_FILE_TYPE_DIRECTORY
> + && !g_file_delete(destinationFile.get(), 0, 0)) {
> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR);
> + return;
> + }
> + }
This is the same as the code above, the same comments apply, and consider
making this a helper function, I think.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:197
> + helperCallbacks->didFail(FileError::NOT_FOUND_ERR);
Better error reporting would be great =) maybe have a helper function that
translates GIOErrors to FileErrors.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:223
> + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(file), 0,
error));
Same here, enumerate children perhaps?
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:252
> + if (error) {
> + FileError::ErrorCode errorCode;
> + switch (error->code) {
> + case G_IO_ERROR_NOT_EMPTY:
> + errorCode = FileError::INVALID_MODIFICATION_ERR;
> + break;
> + case G_IO_ERROR_NOT_FOUND:
> + default:
> + errorCode = FileError::NOT_FOUND_ERR;
> + }
That's more like it =). Here's the seed for our helper function!
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:280
> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(),
"*", G_FILE_QUERY_INFO_NONE, 0, 0));
"standard::*,time::*" looks right for here.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:313
> + if (type == G_FILE_TYPE_REGULAR && absolutePath == String("/")) {
> + helperCallbacks->didFail(FileError::SECURITY_ERR);
> + return;
> + }
Why do we need this check? Is this denying creating the root directory (of the
filesystem hierarchy)? Wouldn't that be checked at a higher level?
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:328
> + errorCode = FileError::INVALID_MODIFICATION_ERR;
PathExistsError seems to be a better choice here.
(http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-pathexistserror)
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:362
> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(object.get(),
"*", G_FILE_QUERY_INFO_NONE, 0, 0));
G_FILE_ATTRIBUTE_STANDARD_TYPE
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:400
> + GRefPtr<GFileInfo> fileInfo =
adoptGRef(g_file_query_info(child.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0));
Ditto.
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:408
> + } else {
> + callbacks->didFail(FileError::NOT_FOUND_ERR);
> + return;
> + }
Instead of checking for !error it would be better to check for error and remove
the indentation for the normal case.
if (error) {
callbacks->didFail(FileError::NOT_FOUND_ERR);
return;
}
...rest of code here ...
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:424
> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(),
"*", G_FILE_QUERY_INFO_NONE, 0, 0));
G_FILE_ATTRIBUTE_STANDARD_SIZE
> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:446
> +void AsyncFileSystemGtk::createSnapshotFileAndReadMetadata(const String&
path, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
> +{
> + // FIXME: Is this api really same with readMetadata?
> + readMetadata(path, callbacks);
> +}
I'm pretty sure it isn't. File creation is certainly involved here. It seems to
be called by DOMFileSystemSync::createFile and DOMFileSystem::createFile. The
tests we have do not exercise this?
> Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:64
> +#endif // AsyncFileSystemCallbacks_h
Name is wrong here.
> LayoutTests/platform/gtk/Skipped:287
> +# Passed but we should enable file-system & mutation-observers at the same
time.
Anything stops us from doing that? =)
More information about the webkit-reviews
mailing list