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

[bugzilla-daemon at webkit.org]

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





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

Thank you for your thoughtful review. :)

>> Source/WebCore/GNUmakefile.list.am:4999
>>  	Source/WebCore/platform/gtk/AsyncFileSystemGtk.h
> 
> Why did you move these to webcore_sources?

Because of build-break for WK2. AsyncFileSystemGtk.cpp uses temporaryPathPrefix & persistentPathPrefix, but they are defined in AsyncFileSystem.cpp that included in webcore_sources.

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

> 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?
In my humble opinion, yes it is. We don't need to actually read directory & do something here. we just gather directory entries and return them to JS layer. That's enough. :)

> 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.
I think there is no problem too, though we return all directory entries at once. The spec is just saying that readEntries might not return all entries for a single call, so users need to check if the returned entries is empty array. In our case, we can return all entries at once and then return empty block, so it seems suitable to the spec.
But I'm still not understand clearly what the situation is when we need to read directories more here. I guess we may need to check 'hasmore' for multithread model(workers)...

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:65
>> +static FileError::ErrorCode validateFileType(GFile* sourceFile, GFile* destinationFile)
> 
> validateFileTypes sounds like a better choice

Done.

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

Done.

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

I've updated the comment but couldn't find a better place for this. :\

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

Done.

>> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:348
>> +    helperCallbacks->didSucceed();
> 
> I believe you need to report success also after reading metadata.

Actually we don't need to call didSucceed for reading metadata. didReadMetadata works similar to didSucceed for readMetadata api. You can refer the comment in AsyncFileSystem.h. :)
It says..
> AsyncFileSystemCallbacks::didReadMetadata() is called when the operation is completed successfully. AsyncFileSystemCallbacks::didFail() is called otherwise.

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