[Webkit-unassigned] [Bug 58443] [GTK] Support the file system API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Apr 15 10:33:47 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=58443
Gustavo Noronha (kov) <gns at gnome.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #136422|review? |review-
Flag| |
--- Comment #24 from Gustavo Noronha (kov) <gns at gnome.org> 2012-04-15 10:33:46 PST ---
(From update of attachment 136422)
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? =)
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list