[webkit-reviews] review denied: [Bug 38228] [chromium] Add WebFileSystem interface and hook up with all FileSystem methods : [Attachment 54730] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jian Li
<jianli at chromium.org>'s request for review:
Bug 38228: [chromium] Add WebFileSystem interface and hook up with all
FileSystem methods
https://bugs.webkit.org/show_bug.cgi?id=38228

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebFileSystem.h:61
 +	virtual void closeFile(FileHandle&) = 0;
so closeFile will reset the FileHandle to a special invalid value, but there
does not appear to be any method here to initialize a FileHandle to hold the
invalid value.	it would be nice if closeFile had some documentation describing
this subtle behavior.

WebKit/chromium/public/WebFileSystem.h:49
 +	virtual bool fileExists(const WebString& path) = 0;
please provide default implementations for all of these methods.  that way you
can implement ChromiumBridge methods in terms of these without having to land
something on the Chromium side.  in general, any method implemented by the
embedder should have a default implementation in the WebKit API.

WebKit/chromium/src/ChromiumBridge.cpp:328
 +	return webKitClient()->filePathToURL(path);
you can modify all of the existing file-related ChromiumBridge methods to first
null-test fileSystem().  if that is null, then fallback to the old methods.