[Webkit-unassigned] [Bug 43151] [chromium] Add WebKit API for HTML5 FileSystem API

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 17 22:47:21 PDT 2010


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
  Attachment #64658|review?                     |review-
               Flag|                            |

--- Comment #21 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-08-17 22:47:20 PST ---
(From update of attachment 64658)
looking good... just a few more suggestions:

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

 +      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:

  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.

 +  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

 +          WebFrame*, WebFileSystem::Type, long long size,
i suspect that you need a #include "WebFileSystem.h" up above.

 +          WebFrame*, WebFileSystem::Type, long long size,
it would be good to describe this "size" parameter in the comments.

 +      // HTML5 FileSystem ----------------------------------------------------
the filesystem spec is not strictly speaking part of html5.  may be better
to drop the "HTML5" from the comment.

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

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list