Bug 99162

Summary:

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

Product:

WebKit

Reporter:

Dongwoo Joshua Im (dwim) <dw.im>

Component:

WebCore Misc.

Assignee:

Dongwoo Joshua Im (dwim) <dw.im>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

dimich, donggwan.kim, gyuyoung.kim, haraken, kinuko, mifenton, morrita, rakuco, rwlbuis, tonikitoo, tzik, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

79193

Attachments:

Description	Flags
patch	none
patch	none
patch	none
patch	none
Patch	none
Patch	gyuyoung.kim: review+, gyuyoung.kim: commit-queue-

Dongwoo Joshua Im (dwim)

Reported 2012-10-12 04:37:43 PDT

I totally believe AsyncFileSystem::openFileSystem should have FileSystemSynchronousType as a parameter. FileSystemSynchronousType is an important information for AsyncFileSystem. I'll upload patch soon.

Attachments
patch (8.62 KB, patch) 2012-10-12 04:43 PDT, Dongwoo Joshua Im (dwim)	no flags	Details Formatted Diff Diff
patch (8.57 KB, patch) 2012-10-12 04:48 PDT, Dongwoo Joshua Im (dwim)	no flags	Details Formatted Diff Diff
patch (5.14 KB, patch) 2012-10-14 19:30 PDT, Dongwoo Joshua Im (dwim)	no flags	Details Formatted Diff Diff
patch (5.15 KB, patch) 2012-10-15 01:28 PDT, Dongwoo Joshua Im (dwim)	no flags	Details Formatted Diff Diff
Patch (5.41 KB, patch) 2012-10-25 01:05 PDT, Dongwoo Joshua Im (dwim)	no flags	Details Formatted Diff Diff
Patch (5.41 KB, patch) 2012-10-25 01:07 PDT, Dongwoo Joshua Im (dwim)	gyuyoung.kim: review+ gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Dongwoo Joshua Im (dwim)

Comment 1 2012-10-12 04:43:57 PDT

Created attachment 168394 [details] patch

Dongwoo Joshua Im (dwim)

Comment 2 2012-10-12 04:48:40 PDT

Created attachment 168395 [details] patch I've uploaded wrong patch before.

Gyuyoung Kim

Comment 3 2012-10-13 03:46:08 PDT

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)

Comment 4 2012-10-14 18:16:31 PDT

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)

Comment 5 2012-10-14 19:30:00 PDT

Created attachment 168609 [details] patch

Dongwoo Joshua Im (dwim)

Comment 6 2012-10-14 21:58:47 PDT

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)

Comment 7 2012-10-15 01:28:31 PDT

Created attachment 168648 [details] patch

Dongwoo Joshua Im (dwim)

Comment 8 2012-10-16 02:09:12 PDT

Any comments for this patch? This is important patch, kind of start-up patch, for the port implementation.

Gyuyoung Kim

Comment 9 2012-10-16 20:07:43 PDT

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)

Comment 10 2012-10-25 00:16:21 PDT

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

Comment 11 2012-10-25 01:05:19 PDT

Created attachment 170578 [details] Patch

Dongwoo Joshua Im (dwim)

Comment 12 2012-10-25 01:07:56 PDT

Created attachment 170579 [details] Patch

Gyuyoung Kim

Comment 13 2012-11-05 17:06:35 PST

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

Comment 14 2012-11-05 17:07:40 PST

CC'ing Hajime, Kentaro.

Gyuyoung Kim

Comment 15 2012-11-06 17:33:02 PST

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.

Note You need to log in before you can comment on or make changes to this bug.