Bug 44433

Created attachment 65132 [details] Patch

Created attachment 65133 [details] Patch Removed unnecessary line

Created attachment 65146 [details] Patch Patch (rebased)

Comment on attachment 65146 [details] Patch Some drive by comments. WebCore/platform/AsyncFileSystemCallbacks.h:48 + virtual void didOpenFileSystem(const String& name, const String& rootPath) = 0; If this callback was void didOpenFileSystem(PassOwnPtr<AsyncFileSystem> result) could we avoid the need for the AsyncFileSystemFactory class? Effectively the openFileSystem() method is a factory method that returns asyncly. WebCore/storage/DOMFileSystem.h:69 + DOMFileSystem(ScriptExecutionContext*, const String& name, AsyncFileSystem*); Should this be PassOwnPtr<AFS> ? WebCore/storage/DOMFileSystem.h:50 + static PassRefPtr<DOMFileSystem> create(ScriptExecutionContext* context, const String& name, AsyncFileSystem* asyncFileSystem) PassOwnPtr<AFS> here too?

Created attachment 65178 [details] Patch

Created attachment 65183 [details] Patch Patch (rebased)

Thanks for your comments. (In reply to comment #4) > (From update of attachment 65146 [details]) > WebCore/platform/AsyncFileSystemCallbacks.h:48 > + virtual void didOpenFileSystem(const String& name, const String& rootPath) = 0; > If this callback was void didOpenFileSystem(PassOwnPtr<AsyncFileSystem> result) could we avoid the need for the AsyncFileSystemFactory class? Effectively the openFileSystem() method is a factory method that returns asyncly. Well, because I wanted to put AsyncFileSystem under platform there're some layering issues around it. For non-chromium version I'm planning to add a thread version of AsyncFileSystem implementation but it reguires high-level classes (e.g. ScriptExecutionContext) and thus needs to be created (or set via factory) at higher level. I removed the factory class and changed the code to create AsyncFileSystem before calling openFileSystem. I also included LocalFileSystem class in the patch to give some idea how AsyncFileSystem is going to be created in non-chromium cases. (I tried to make this layering look prettier but haven't succeeded yet...) > WebCore/storage/DOMFileSystem.h:69 > + DOMFileSystem(ScriptExecutionContext*, const String& name, AsyncFileSystem*); > Should this be PassOwnPtr<AFS> ? Fixed. > WebCore/storage/DOMFileSystem.h:50 > + static PassRefPtr<DOMFileSystem> create(ScriptExecutionContext* context, const String& name, AsyncFileSystem* asyncFileSystem) > PassOwnPtr<AFS> here too? Fixed.

Created attachment 65460 [details] =Patch (rebased; did some cleanup)

Comment on attachment 65460 [details] =Patch (rebased; did some cleanup) This looks pretty good to me. WebCore/platform/AsyncFileSystem.h:65 + virtual void move(const String& srcPath, const String& destPath, PassOwnPtr<AsyncFileSystemCallbacks>) = 0; Should we add comments to indicate which of the callbacks is invoked on success and on error? I guess its usually didSucceed() or didFail(int error), maybe a general comment to that affect and than call out those that deviate from that. WebCore/platform/AsyncFileSystemCallbacks.h:54 + virtual void didReadDirectoryEntry(const String& name, bool isDirectory) = 0; What calls is didReadDirectoryEntry(...) and didReadDirectoryChunkDone(...) in response to?

Comment on attachment 65460 [details] =Patch (rebased; did some cleanup) WebCore/storage/LocalFileSystem.cpp:72 + AsyncFileSystem::openFileSystem(m_basePath, context->securityOrigin()->databaseIdentifier(), type, new FileSystemCallbacks(successCallback, errorCallback, context, asyncFileSystem.release())); it seems a bit awkward that openFileSystem is a static method and not an instance method on AsyncFileSystem. you have an instance of AsyncFileSystem here. WebCore/storage/LocalFileSystem.h:52 + void requestFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type type, long long size, PassRefPtr<FileSystemCallback>, PassRefPtr<ErrorCallback>); nit: leave off the "type" parameter name here

Created attachment 65482 [details] Patch

Thanks, (In reply to comment #9) > (From update of attachment 65460 [details]) > This looks pretty good to me. > > WebCore/platform/AsyncFileSystem.h:65 > + virtual void move(const String& srcPath, const String& destPath, PassOwnPtr<AsyncFileSystemCallbacks>) = 0; > Should we add comments to indicate which of the callbacks is invoked on success and on error? I guess its usually didSucceed() or didFail(int error), maybe a general comment to that affect and than call out those that deviate from that. Added comments. > WebCore/platform/AsyncFileSystemCallbacks.h:54 > + virtual void didReadDirectoryEntry(const String& name, bool isDirectory) = 0; > What calls is didReadDirectoryEntry(...) and didReadDirectoryChunkDone(...) in response to? Added AsyncFileSystem::readDirectory()... it was missing in the previous patch.

(In reply to comment #10) > (From update of attachment 65460 [details]) > WebCore/storage/LocalFileSystem.cpp:72 > + AsyncFileSystem::openFileSystem(m_basePath, context->securityOrigin()->databaseIdentifier(), type, new FileSystemCallbacks(successCallback, errorCallback, context, asyncFileSystem.release())); > it seems a bit awkward that openFileSystem is a static method and not an instance > method on AsyncFileSystem. you have an instance of AsyncFileSystem here. That's true... I was afraid it might complicate the ownership of AsyncFileSystem. Changed the method to virtual method. > WebCore/storage/LocalFileSystem.h:52 > + void requestFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type type, long long size, PassRefPtr<FileSystemCallback>, PassRefPtr<ErrorCallback>); > nit: leave off the "type" parameter name here Fixed.

Created attachment 65513 [details] Patch Another attempt. It\'s hard to avoid all the violations but how about something like this.

Created attachment 65523 [details] Patch Removed all the layering hacks. For now we would just want to let this in for chromium.

Created attachment 65524 [details] Patch

Comment on attachment 65524 [details] Patch WebCore/platform/AsyncFileSystem.h:62 + static void openFileSystem(const String& basePath, const String& storageIdentifier, Type, PassOwnPtr<AsyncFileSystemCallbacks>); I don't understand the comment about passing ownership of 'this' since this method is static. There is no 'this' pointer. WebCore/platform/AsyncFileSystemCallbacks.h:57 + virtual void didReadDirectoryChunkDone(bool hasMore) = 0; The name of this method is a bit awkward sounding. Perhaps didReadDirectoryEntries would be a better name? Pluralizing Entry suggests a "chunk of entries", which seems similar to what you were trying to get at with using Chunk in the name. Also, the Done bit may be redundant with the hasMore parameter, so leaving it as didReadDirectoryEntries seems reasonable to me. It just informs the callbacks that entries were read and more may be read soon. Works for me! R=me w/ these two changes.

Committed r66255: <http://trac.webkit.org/changeset/66255>