Bug 99162 – AsyncFileSystem::openFileSystem should be called only when need to handle the filesystem asynchronous fashion.

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Dongwoo Joshua Im (dwim)]

I totally believe AsyncFileSystem::openFileSystem should have FileSystemSynchronousType as a parameter.
FileSystemSynchronousType is an important information for AsyncFileSystem.

I'll upload patch soon.

[Dongwoo Joshua Im (dwim)]

Created attachment 168394 [details]
patch

[Dongwoo Joshua Im (dwim)]

Created attachment 168395 [details]
patch

I've uploaded wrong patch before.

[Gyuyoung Kim]

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.

[Dongwoo Joshua Im (dwim)]

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.

[Dongwoo Joshua Im (dwim)]

Created attachment 168609 [details]
patch

[Dongwoo Joshua Im (dwim)]

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?

[Dongwoo Joshua Im (dwim)]

Created attachment 168648 [details]
patch

[Dongwoo Joshua Im (dwim)]

Any comments for this patch?

This is important patch, kind of start-up patch, for the port implementation.

[Gyuyoung Kim]

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 ?

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 170578 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 170579 [details]
Patch

[Gyuyoung Kim]

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

[Gyuyoung Kim]

CC'ing Hajime, Kentaro.

[Gyuyoung Kim]

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.