Bug 44433 - Add AsyncFileSystem interface for platform-dependent FileSystem API implementation
Summary: Add AsyncFileSystem interface for platform-dependent FileSystem API implement...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 42903 44434 44732
  Show dependency treegraph
 
Reported: 2010-08-23 09:36 PDT by Kinuko Yasuda
Modified: 2010-08-27 14:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (25.80 KB, patch)
2010-08-23 09:40 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (25.52 KB, patch)
2010-08-23 09:44 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (25.52 KB, patch)
2010-08-23 12:00 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (42.40 KB, patch)
2010-08-23 16:37 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (43.35 KB, patch)
2010-08-23 16:48 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
=Patch (rebased; did some cleanup) (37.41 KB, patch)
2010-08-25 11:59 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (40.52 KB, patch)
2010-08-25 15:18 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (46.79 KB, patch)
2010-08-25 18:30 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (39.83 KB, patch)
2010-08-25 21:40 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (39.83 KB, patch)
2010-08-25 22:40 PDT, Kinuko Yasuda
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-08-23 09:36:35 PDT
Add AsyncFileSystem interface that provides an async interface to platform-dependent DOMFileSystem implementation.
Comment 1 Kinuko Yasuda 2010-08-23 09:40:28 PDT
Created attachment 65132 [details]
Patch
Comment 2 Kinuko Yasuda 2010-08-23 09:44:45 PDT
Created attachment 65133 [details]
Patch

Removed unnecessary line
Comment 3 Kinuko Yasuda 2010-08-23 12:00:39 PDT
Created attachment 65146 [details]
Patch

Patch (rebased)
Comment 4 Michael Nordman 2010-08-23 13:37:04 PDT
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?
Comment 5 Kinuko Yasuda 2010-08-23 16:37:19 PDT
Created attachment 65178 [details]
Patch
Comment 6 Kinuko Yasuda 2010-08-23 16:48:59 PDT
Created attachment 65183 [details]
Patch

Patch (rebased)
Comment 7 Kinuko Yasuda 2010-08-23 16:50:24 PDT
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.
Comment 8 Kinuko Yasuda 2010-08-25 11:59:20 PDT
Created attachment 65460 [details]
=Patch (rebased; did some cleanup)
Comment 9 Michael Nordman 2010-08-25 13:25:15 PDT
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 10 Darin Fisher (:fishd, Google) 2010-08-25 13:47:02 PDT
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
Comment 11 Kinuko Yasuda 2010-08-25 15:18:40 PDT
Created attachment 65482 [details]
Patch
Comment 12 Kinuko Yasuda 2010-08-25 15:21:56 PDT
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.
Comment 13 Kinuko Yasuda 2010-08-25 15:26:36 PDT
(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.
Comment 14 Kinuko Yasuda 2010-08-25 18:30:04 PDT
Created attachment 65513 [details]
Patch

Another attempt.  It\'s hard to avoid all the violations but how about something like this.
Comment 15 Kinuko Yasuda 2010-08-25 21:40:55 PDT
Created attachment 65523 [details]
Patch

Removed all the layering hacks.  For now we would just want to let this in for chromium.
Comment 16 Kinuko Yasuda 2010-08-25 22:40:30 PDT
Created attachment 65524 [details]
Patch
Comment 17 Darin Fisher (:fishd, Google) 2010-08-26 23:25:33 PDT
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.
Comment 18 Kinuko Yasuda 2010-08-27 14:15:56 PDT
Committed r66255: <http://trac.webkit.org/changeset/66255>