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: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Other | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 79193 | ||||||||||||||||
Attachments: |
|
Description
Dongwoo Joshua Im (dwim)
2012-10-12 04:37:43 PDT
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.
|