Bug 44433 – Add AsyncFileSystem interface for platform-dependent FileSystem API implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Add AsyncFileSystem interface that provides an async interface to platform-dependent DOMFileSystem implementation.

[Kinuko Yasuda]

Created attachment 65132 [details]
Patch

[Kinuko Yasuda]

Created attachment 65133 [details]
Patch

Removed unnecessary line

[Kinuko Yasuda]

Created attachment 65146 [details]
Patch

Patch (rebased)

[Michael Nordman]

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?

[Kinuko Yasuda]

Created attachment 65178 [details]
Patch

[Kinuko Yasuda]

Created attachment 65183 [details]
Patch

Patch (rebased)

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

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

[Michael Nordman]

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?

[Darin Fisher (:fishd, Google)]

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

[Kinuko Yasuda]

Created attachment 65482 [details]
Patch

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 65513 [details]
Patch

Another attempt.  It\'s hard to avoid all the violations but how about something like this.

[Kinuko Yasuda]

Created attachment 65523 [details]
Patch

Removed all the layering hacks.  For now we would just want to let this in for chromium.

[Kinuko Yasuda]

Created attachment 65524 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[Kinuko Yasuda]

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