[webkit-reviews] review denied: [Bug 43151] [chromium] Add WebKit API for HTML5 FileSystem API : [Attachment 64658] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 43151: [chromium] Add WebKit API for HTML5 FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=43151

Attachment 64658: Patch
https://bugs.webkit.org/attachment.cgi?id=64658&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.