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

Thu Apr 19 00:17:26 PDT 2012

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

--- Comment #26 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2012-04-19 00:17:26 PST ---
(From update of attachment 136422)
View in context: https://bugs.webkit.org/attachment.cgi?id=136422&action=review

Thanks for the your kind 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.

Removed it.

>> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:136
>> +    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.

Done like chromium's way.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:84
>> +
> 
> In addition to this check we should also have an ASSERT and check for type not being one we're expecting.

Inserted ASSERT in below else clause.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:92
>> +        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.

Done.

>> 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::*".

Replaced the way how we get the file type, with using g_file_query_file_type.

>> 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.

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

Yeh. It looks reasonable but failed on fast/filesystem/op-move.html if we do like that. It might be an issue of the testcase itself. I think we need to discuss with the original author of it more.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:120
>> +            helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR);
> 
> How about PathExistsError for this one?

Ditto.

>> 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.

Done. I added a new function toFileErrorCode which converts GError to FileError to check what happened.

>> 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?

I think it's a better approach. Done :)

>> 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.

We're going to use g_file_query_file_type glib utility function to just get the file type.

>> 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?

Right. It looks there is a logical bug. Done.

>> 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.

Done. We're going to use g_file_query_file_type to get the file type.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:191
>> +    }
> 
> This is the same as the code above, the same comments apply, and consider making this a helper function, I think.

Done. Added a helper function, validateFileType.

>> 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.

Done with adding toFileErrorCode.

>> 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?

Done.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:252
>> +        }
> 
> That's more like it =). Here's the seed for our helper function!

Yep.

>> 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.

Done.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:313
>> +    }
> 
> 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?

We should prevent creating a new file in upper directory than baseFileSystemPath(logically '/'). Some users (maybe contents) may try to access upper directory by using relative path like '..'. But we should not allow it becuase of the security issue I think. If that kind of invalid path comes in, a higher level (DOMFileSystemBase) converts it to '/', so we here get '/'. If we don't deal with it here. we might face some failures.

>> 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)

I think it needs more discussion. :\

>> 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

Replaced with g_file_query_file_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.

Done.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:408
>> +    }
> 
> 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 ...

Done.

>> 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

Done.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:446
>> +}
> 
> 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?

Done. There is a test for this, but it works without any problem. :p I guess the already existing file affects the test case result. My current implementation might not be perfect now since less information what this API does. Let me watch this...

>> Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:64
>> +#endif // AsyncFileSystemCallbacks_h
> 
> Name is wrong here.

Done

>> LayoutTests/platform/gtk/Skipped:287
>> +# Passed but we should enable file-system & mutation-observers at the same time.
> 
> Anything stops us from doing that? =)

Removed as well. :)

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