[chromium] Add WebKit API for HTML5 FileSystem API

• Component: WebKit API
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Add asynchronous FileSystem interface to WebKit API.
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html

[Kinuko Yasuda]

Created attachment 62887 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 62887 [details]
Patch

WebKit/chromium/ChangeLog:16
 +          * public/WebGetFileFlags.h: Added.
if you haven't done so already, please ask darin if he's ok with the names of these files.

WebKit/chromium/public/WebAsyncFileSystem.h:44
 +  class WebAsyncFileSystem {
it looks to me like this class is not referenced from anywhere, and all its methods are stubbed out. i think it might be better to move this .h file to a patch that implements or uses this class.

WebKit/chromium/public/WebFSCallbacks.h:42
 +  class WebFSCallbacks {
same comment.

WebKit/chromium/public/WebFileError.h:34
 +  #include "WebCommon.h"
is this include needed?

WebKit/chromium/public/WebFileError.h:47
 +      enum Code {
how about renaming Code to FileErrorCode?

WebKit/chromium/public/WebFileError.h:50
 +          // NO_MODIFICATION_ALLOWED_ERR FileError in HTML5 File API.
i don't think you need a comment for every error code. i think it should be enough to mention once that these are the FileError codes from the HTML5 File API.

WebKit/chromium/public/WebGetFileFlags.h:39
 +      // If |create| is true, WebAsyncFileSystem.getFile or getDirectory creates a file or directory if it was not previously there.
nit: s/WebAsyncFileSystem.getFile/WebAsyncFileSystem::getFile()/, s/getDirectory/getDirectory()/.

[Kinuko Yasuda]

Created attachment 63011 [details]
Patch (centralized APIs in a single file)

[Dumitru Daniliuc]

Comment on attachment 63011 [details]
Patch (centralized APIs in a single file)

r=me.

WebKit/chromium/public/WebAsyncFileSystem.h:41
 +  class WebFSCallbacks;
need this?

WebKit/chromium/public/WebAsyncFileSystem.h:48
 +      // FileError code defined for HTML5 File API.
nit: s/code/codes/.

[Darin Fisher (:fishd, Google)]

Comment on attachment 63011 [details]
Patch (centralized APIs in a single file)

Hold the presses, I have some feedback that I'm writing up now...

[Darin Fisher (:fishd, Google)]

Comment on attachment 63011 [details]
Patch (centralized APIs in a single file)

WebKit/chromium/public/WebAsyncFileSystem.h:50
 +          SuccessError = 0,
nit: in the WebKit API we use the following format for enums:

  enum Foo {
      FooBar,
      FooBaz,
  };

So, these should be re-worded like so:

  enum Error {
      ErrorNone,
      ErrorNoModificationAllowed,
      ErrorNotFound,
      ...
  };

One more note:  It seems a bit odd for these error codes, which are generic
file system error codes, to be defined here only for use with WebAsyncFileSystem.
We also have WebFileSystem, and it seems like these could be useful for it as
well.  In fact, it seems like we should try to unify WebFileSystem and WebAsyncFileSystem
so that they expose similar APIs, with the exception of WebFileSystem containing only
synchronous methods whereas WebAsyncFileSystem contains asynchronous methods.
how about just merging these asynchronous methods into WebFileSystem?

WebKit/chromium/public/WebAsyncFileSystem.h:71
 +          virtual ~Callbacks() { }
can this destructor be made protected?  i'm assuming that the
embedder who receives a Callbacks pointer is not expected to
delete it, so making it protected would enforce that.

WebKit/chromium/public/WebAsyncFileSystem.h:86
 +      virtual void getFile(const WebString& path, GetFileFlags flags, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
it is not clear what getFile or getDirectory do.  what result is
returned to Callbacks?

WebKit/chromium/public/WebAsyncFileSystem.h:81
 +  
it would be very helpful to document how each of these methods is expected
to work.  what callbacks get called and with what information?

WebKit/chromium/public/WebAsyncFileSystem.h:80
 +      virtual void requestFileSystem(bool persistent, long long size, const WebSecurityOrigin&, WebFrame*, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
this seems like a function that should be on WebFrameClient since it is
specific to a WebFrame.  policy functions generally go on WebFrameClient.
The WebSecurityOrigin can be retrieved from the WebFrame, so it does not
need to be an explicit parameter.  since requestFileSystem is an async
method, you could just have a method on WebFrame that is called to complete
the filesystem request.  you'd need to have some way of identifying the
filesystem request.  or, you could define a separate callback interface
for just this one thing (e.g., like WebFileChooserCompletion).

WebKit/chromium/public/WebAsyncFileSystem.h:64
 +          bool create;
how about just expressing this as a bit-mask?  then define enums for it?

WebKit/chromium/public/WebKitClient.h:268
 +      virtual WebAsyncFileSystem* asyncFileSystem() { return 0; }
the name of this method suggests that a WebAsyncFileSystem is not created
each time this method is called.  so, you should probably make WebAsyncFileSystem's
destructor protected so that it is clear that WebKit code should not be deleting
a WebAsyncFileSystem pointer.

[Kinuko Yasuda]

Thanks for the comments.

(In reply to comment #6)
> (From update of attachment 63011 [details])
> well.  In fact, it seems like we should try to unify WebFileSystem and WebAsyncFileSystem
> so that they expose similar APIs, with the exception of WebFileSystem containing only
> synchronous methods whereas WebAsyncFileSystem contains asynchronous methods.
> how about just merging these asynchronous methods into WebFileSystem?

I was afraid mixing up APIs with different security assumptions in a single module would be confusing.
WebFileSystem API has more generic usage than WebAsync one - it implements APIs that are used for misc internal file operations in WebKit.  Basically they don't work in a sandbox environment except for a few APIs that are used for File API and require prior permission on the target paths.

Assumptions for HTML5 FileSystem API are different from WebSyncFileSystem - it's primarily being added for FileSystem API and is going to work only if the target paths are under a certain directory.  In that sense what we're going to add is not a generic async filesystem API but 'HTML5FileSystem'.   Or are we planning to use it for other purpose?

I agree that it would look more natural to have synchronous API and asynchronous API together but at least it would need very clear API documentation in that case.

> WebKit/chromium/public/WebAsyncFileSystem.h:71
>  +          virtual ~Callbacks() { }
> can this destructor be made protected?  i'm assuming that the
> embedder who receives a Callbacks pointer is not expected to
> delete it, so making it protected would enforce that.

Hmm... I thought that just giving the ownership of the pointer to the embedder and having it delete would be easier, but it may not look clean as an exposed API.  In either way I'm going to add clearer comments.

[Kinuko Yasuda]

Created attachment 63139 [details]
Patch (updated; not yet merged into WebFileSystem)

[Michael Nordman]

Comment on attachment 63139 [details]
Patch (updated; not yet merged into WebFileSystem)

Not sure merging with WebFileSystem is a feature? Do we expect to be able to call WebFileSystem::openFile(path, OpenForRead) to retrieve a FileHandle to a file in the HTML5FileSystem, or call WebFileSystem::makeAllDirectories(x) to a location in the HTML5FileSystem? I don't think thats where we were heading. It may be less confusing to have these segregated. Many of the general purpose interfaces in WebFileSystem are crippled in child processes and fully enabled in the main browser process. In comparison, the HTML5FileSytem APIs only need to be available in child processes.


WebKit/chromium/public/WebFileSystemCallback.h:37
 +  template <typename T> class WebVector;
Looks like WebVector isn't needed here.

WebKit/chromium/public/WebFileSystemCallback.h:43
 +      // accepted.
nit: since no line is too long in webkit/webcore... consider unwrapping this comment line

WebKit/chromium/public/WebFileSystemCallback.h:50
 +      virtual ~WebFileSystemCallback() {}
The ownership semantics for this 'callback' class should probably be the same as for the WebAsyncFileSystem::Callbacks, whichever way that tree falls.


WebKit/chromium/public/WebAsyncFileSystem.h:66
 +          virtual void onSuccessMetadata(WebFileInfo info) = 0;
Should this be 
virtual void onSuccessMetadata(const WebFileInfo& info) = 0;

WebKit/chromium/public/WebAsyncFileSystem.h:114
 +      virtual void getDirectory(const WebString& path, bool exclusive, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
Should this be createDirectory() for symmetry with createFile()?

WebKit/chromium/public/WebAsyncFileSystem.h:119
 +      virtual void isFile(const WebString& path, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
I wonder if isDirectory and isFile could/should be subsumed by getMetaData?

[Michael Nordman]

Following up on our discussion... other name ideas for this interface could be WebVirtualFileSystem or WebSandboxedFileSystem or WebProtectedFileSystem.

Also I have a question about the form of the 'path' parameters that will be presented to this interface by renderers. Are those full os file system paths or are they more opaque? Full os paths will contain the os username (on windows at least) which goes against having child processes sandboxed so I'm not sure that's the best option?

We could devise a naming scheme for the Web<Something>FileSystem that protects against leaking the os username when accessing the html5 file system, but also allows expressing full os paths (if/when we need to)? A simple prefix could suffice, if a valid prefix isn't present the operation fails.

html5 <origin_id>/perm/
html5 <origin_id>/temp/
os c:/passwords.txt
os \\filer\home\michaeln

[Kinuko Yasuda]

Created attachment 64183 [details]
Patch (cleaned up minor issues)

[Kinuko Yasuda]

Updated the patch for minor clean ups.   For now I haven't changed the code for bigger issues - e.g. whether we should merge this into WebFileSystem or not and what name the interface has to be.

(In reply to comment #9)
> (From update of attachment 63139 [details])
> Not sure merging with WebFileSystem is a feature? Do we expect to be able to call WebFileSystem::openFile(path, OpenForRead) to retrieve a FileHandle to a file in the HTML5FileSystem, or call WebFileSystem::makeAllDirectories(x) to a location in the HTML5FileSystem? I don't think thats where we were heading. It may be less confusing to have these segregated. Many of the general purpose interfaces in WebFileSystem are crippled in child processes and fully enabled in the main browser process. In comparison, the HTML5FileSytem APIs only need to be available in child processes.

> WebKit/chromium/public/WebFileSystemCallback.h:37
>  +  template <typename T> class WebVector;
> Looks like WebVector isn't needed here.

Removed.

> WebKit/chromium/public/WebFileSystemCallback.h:43
>  +      // accepted.
> nit: since no line is too long in webkit/webcore... consider unwrapping this comment line

Fixed.

> WebKit/chromium/public/WebFileSystemCallback.h:50
>  +      virtual ~WebFileSystemCallback() {}
> The ownership semantics for this 'callback' class should probably be the same as for the WebAsyncFileSystem::Callbacks, whichever way that tree falls.
> 
> 
> WebKit/chromium/public/WebAsyncFileSystem.h:66
>  +          virtual void onSuccessMetadata(WebFileInfo info) = 0;
> Should this be 
> virtual void onSuccessMetadata(const WebFileInfo& info) = 0;

Fixed.

> WebKit/chromium/public/WebAsyncFileSystem.h:114
>  +      virtual void getDirectory(const WebString& path, bool exclusive, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
> Should this be createDirectory() for symmetry with createFile()?

Exactly!  Fixed.

> WebKit/chromium/public/WebAsyncFileSystem.h:119
>  +      virtual void isFile(const WebString& path, Callbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
> I wonder if isDirectory and isFile could/should be subsumed by getMetaData?

Making them integrated into getMetadata would make sense too but I wonder if they may have a little bit different usages.   In this patch I changed their names to fileExists and directoryExists.

[Kinuko Yasuda]

Created attachment 64291 [details]
Patch

[Kinuko Yasuda]

Added readDirectory method and made some more minor fixes.

[Michael Nordman]

This interface looks good to me. I do wonder if word smithing the name of WebAsyncFileSystem would help. Let's see what darin, being the keeper of webkipAPI consistency, has to say.

[Kinuko Yasuda]

Per our offline discussion, fishd, ericu, michaeln and kinuko agreed to:
1. have separate WebKit API for HTML5 FileSystem API from existing WebFileSystem
2. rename existing WebFileSystem to WebFileUtilities
3. expose inner classes (Callbacks, Entry and Error) to top level classes and prefix them with WebFileSystem (or WebFile)

[Kinuko Yasuda]

(In reply to comment #16)
> Per our offline discussion, fishd, ericu, michaeln and kinuko agreed to:
> 1. have separate WebKit API for HTML5 FileSystem API from existing WebFileSystem
> 2. rename existing WebFileSystem to WebFileUtilities
> 3. expose inner classes (Callbacks, Entry and Error) to top level classes and prefix them with WebFileSystem (or WebFile)

4. have the renderer pass absolute/sanitized paths to this API

[Darin Fisher (:fishd, Google)]

Comment on attachment 64291 [details]
Patch

(getting this out of the review queue since there is a new version coming...)

[Kinuko Yasuda]

Created attachment 64658 [details]
Patch

Uploading a new patch - this patch also does the latter half of issue 44077, i.e. replaces the WebFileSystem interface with new one.  All the inner classes were exported except for a new enum WebFileSystem::Type; for this one I was not sure if we want to put it outside of the WebFileSystem.

[WebKit Review Bot]

Attachment 64658 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3722337

[Darin Fisher (:fishd, Google)]

Comment on attachment 64658 [details]
Patch

looking good... just a few more suggestions:


WebKit/chromium/public/WebFileError.h:35
 +  class WebFileError {
why not just declare an enum named WebFileError?  the webkit api declares
plain old enums in other cases.

enum WebFileError {
    WebFileErrorNone = 0,
    WebFileErrorNoModificationAllowed = 7,
    ...
};
WebKit/chromium/public/WebFileSystem.h:42
 +      enum Type {
I like this as an inner enum too.  No need to explicitly set the values
to 0 and 1 since those are the defaults.

WebKit/chromium/public/WebFileSystemCallbacks.h:47
 +      virtual void onSuccess() = 0;
nit: we use the naming scheme didFoo instead of onFoo in the webkit api.
it is also nice to try to make the names read as a phrase.  instead of
"on success blah", which isn't good grammar, you'd write "did finish
doing blah" or something like that.

so how about changing these to the following:

  didSucceed()
  didFail(WebFileError);
  didReadMetadata(const WebFileInfo&);
  didReadDirectory(const WebVector<WebFileSystemEntry>&, bool hasMore);
  didOpenFileSystem(const WebString& name, const WebString& rootPath);

i'm not sure if didOpenFileSystem is the best name.  i was trying to
come up with a good verb.  didRequestFileSystem doesn't seem quite right
because the verb should describe the result of the request.  i guess
the result of requestFileSystem is to be able to use the file system,
which is sort of what "opening" the file system might mean.  maybe we
should rename "requestFileSystem" to "openFileSystem".

i appended a "hasMore" parameter to didReadDirectory to suggest that it
would be called again when hasMore is true.

WebKit/chromium/public/WebFileSystemEntry.h:38
 +  struct WebFileSystemEntry {
i'm curious why you went with WebFileSystemEntry instead of WebFileEntry.
on one hand, it seems nice to scope *Entry to WebFileSystem since it
represents an entry in the file system.  on the other hand, the spec
just uses "FileEntry" for this.  and, WebFileEntry seems descriptive
enough.

WebKit/chromium/public/WebFrameClient.h:339
 +          WebFrame*, WebFileSystem::Type, long long size,
i suspect that you need a #include "WebFileSystem.h" up above.

WebKit/chromium/public/WebFrameClient.h:339
 +          WebFrame*, WebFileSystem::Type, long long size,
it would be good to describe this "size" parameter in the comments.

WebKit/chromium/public/WebKitClient.h:266
 +      // HTML5 FileSystem ----------------------------------------------------
the filesystem spec is not strictly speaking part of html5.  may be better
to drop the "HTML5" from the comment.

WebKit/chromium/public/WebFileSystem.h:61
 +      virtual void getMetadata(const WebString& path, WebFileSystemCallbacks* callbacks) { WEBKIT_ASSERT_NOT_REACHED(); }
maybe "readMetadata" instead of "getMetadata"?  this is about reading the
filesystem in a sense.

[Kinuko Yasuda]

Created attachment 64735 [details]
Patch

[Kinuko Yasuda]

Created attachment 64740 [details]
Patch

Removed unnecessary parameter names.

[Kinuko Yasuda]

Created attachment 64741 [details]
Patch

Changed requestFileSystem in WebFrameClient to openFileSystem.

[WebKit Review Bot]

Attachment 64735 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3746366

[Kinuko Yasuda]

Thanks for reviewing.   All fixed as suggested.  (Please ignore the chromium build failure for now.)

(In reply to comment #21)
> WebKit/chromium/public/WebFileSystemCallbacks.h:47
>  +      virtual void onSuccess() = 0;
> nit: we use the naming scheme didFoo instead of onFoo in the webkit api.
> it is also nice to try to make the names read as a phrase.  instead of
> "on success blah", which isn't good grammar, you'd write "did finish
> doing blah" or something like that.

Changed the method names to did + verb.

> i'm not sure if didOpenFileSystem is the best name.  i was trying to
> come up with a good verb.  didRequestFileSystem doesn't seem quite right
> because the verb should describe the result of the request.  i guess
> the result of requestFileSystem is to be able to use the file system,
> which is sort of what "opening" the file system might mean.  maybe we
> should rename "requestFileSystem" to "openFileSystem".

openFileSystem sounds good to me.  (Confirmed that the verb sounds relevant to what it does to @ericu too)
I changed WebFrameClient::requestFileSystem to openFileSystem as well.

> WebKit/chromium/public/WebFileSystemEntry.h:38
>  +  struct WebFileSystemEntry {
> i'm curious why you went with WebFileSystemEntry instead of WebFileEntry.
> on one hand, it seems nice to scope *Entry to WebFileSystem since it
> represents an entry in the file system.  on the other hand, the spec
> just uses "FileEntry" for this.  and, WebFileEntry seems descriptive
> enough.

Renamed it to WebFileEntry... no one would love longer names when shorter one is enough.
(FileEntry in the spec specifically means the entry is a regular file but not a directory, so it has a bit different meaning.)

[WebKit Review Bot]

Attachment 64741 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3705355

[Kinuko Yasuda]

Created attachment 64797 [details]
Patch

Reuploading to see if it compiles

[WebKit Review Bot]

Attachment 64797 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3769390

[Darin Fisher (:fishd, Google)]

Comment on attachment 64797 [details]
Patch

WebKit/chromium/public/WebFrameClient.h:46
 +  class WebFileSystemCallbacks;
nit: this is also forward declared in WebFileSystem.h, so you don't
need it here.


note, your comment about FileEntry not corresponding to a directory in
the spec makes me reconsider my previous suggestion of changing
WebFileSystemEntry to WebFileEntry.  maybe your original name was
better.

[Kinuko Yasuda]

Committed r65718: <http://trac.webkit.org/changeset/65718>