[webkit-reviews] review denied: [Bug 44360] Add Chromium API for FileWriter : [Attachment 65014] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Eric U.
<ericu at chromium.org>'s request for review:
Bug 44360: Add Chromium API for FileWriter
https://bugs.webkit.org/show_bug.cgi?id=44360

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebFileWriter.h:48
 +	virtual void setFileWriterClient(WebFileWriterClient *client) = 0;
normally client interfaces are set at construction time.  so the method
used to create a WebFileWriter should be parameterized by a
WebFileWriterClient.
that way clients are immutable for the lifetime of an object.  that helps
keep code simpler.  any reason not to conform this code to that pattern?

WebKit/chromium/public/WebFileWriter.h:50
 +	virtual void write(const WebString& path, long long position, const
WebURL& blobURL) = 0;
so a WebFileWriter instance can be used to modify multiple files?
is it intended to be used more like a global service then?  why
not instantiate a WebFileWriter from a file path?

WebKit/chromium/public/WebFileWriter.h:52
 +	virtual void abort() = 0;
cancel is the more common verb in the API (see WebURLLoader)

WebKit/chromium/public/WebFileWriterClient.h:43
 +	virtual void didWrite(long long) = 0;
please give this 'long long' parameter a name since it isn't obvious
what this parameter is used for.

WebKit/chromium/public/WebFileWriterClient.h:41
 +	virtual ~WebFileWriterClient() { }
if you do not expect people to delete a WebFileWriterClient
through this interface then make this destructor protected.
(normally, we don't delete *Client pointers.)