Bug 43151

Created attachment 62887 [details] Patch

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

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

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

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

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.

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.

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

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?

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

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

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.

Created attachment 64291 [details] Patch

Added readDirectory method and made some more minor fixes.

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.

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)

(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

Comment on attachment 64291 [details] Patch (getting this out of the review queue since there is a new version coming...)

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.

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

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.

Created attachment 64735 [details] Patch

Created attachment 64740 [details] Patch Removed unnecessary parameter names.

Created attachment 64741 [details] Patch Changed requestFileSystem in WebFrameClient to openFileSystem.

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

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

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

Created attachment 64797 [details] Patch Reuploading to see if it compiles

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

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.

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