I totally believe AsyncFileSystem::openFileSystem should have FileSystemSynchronousType as a parameter. FileSystemSynchronousType is an important information for AsyncFileSystem. I'll upload patch soon.
Created attachment 168394 [details] patch
Created attachment 168395 [details] patch I've uploaded wrong patch before.
Comment on attachment 168395 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=168395&action=review > Source/WebCore/ChangeLog:8 > + I totally believe AsyncFileSystem::openFileSystem should have FileSystemSynchronousType as a parameter. Hmm, I think "I totally believe" is not proper changelog comment. We have to write fact or description only.
IMO, LocalFileSystem should be restructured little bit. Worker and non-worker case should be distinguished, and async and sync case should be, as well. Chromium port already have done that kind of implementation in their own file.
Created attachment 168609 [details] patch
AsyncFileSystem::openFileSystem should be called only when FileSystem should work asynchronously. IMO, we need SyncFileSystem to handle synchronous mode for the worker. What do you think?
Created attachment 168648 [details] patch
Any comments for this patch? This is important patch, kind of start-up patch, for the port implementation.
Comment on attachment 168648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=168648&action=review > Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:81 > +static void openFileSystemForWorker(ScriptExecutionContext*, const String&, const String&, FileSystemType, bool, PassOwnPtr<AsyncFileSystemCallbacks>, FileSystemSynchronousType) I wonder if this function is related to WORKERS functionality. If so, you need to use #if ENABLE(WORKERS) macro. One more thing, there is openFileSystemForWorker() in chromium port. How is relationship between this and the function. Is there no conflict or can we share it ?
(In reply to comment #9) > (From update of attachment 168648 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168648&action=review > > > Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:81 > > +static void openFileSystemForWorker(ScriptExecutionContext*, const String&, const String&, FileSystemType, bool, PassOwnPtr<AsyncFileSystemCallbacks>, FileSystemSynchronousType) > > I wonder if this function is related to WORKERS functionality. If so, you need to use #if ENABLE(WORKERS) macro. One more thing, there is openFileSystemForWorker() in chromium port. How is relationship between this and the function. Is there no conflict or can we share it ? I will try to use the flag there. There will be no conflict with chromium port at all. But I think I need to talk with chromium guys, I think we can share some part which they already implemented.
Created attachment 170578 [details] Patch
Created attachment 170579 [details] Patch
Comment on attachment 170579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170579&action=review Looks make sense. If there is no comment anymore, I would like to land this tomorrow. > Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:105 > + ASSERT_NOT_REACHED(); UNUSED_PARAM(synchronousType) ?
CC'ing Hajime, Kentaro.
Comment on attachment 170579 [details] Patch There are still some talk whether WebKit continues to support file system. So, I think this patch needs to be wait until finishing the decision.