[webkit-reviews] review denied: [Bug 34652] [chromium] Add the chromium interface to support Blob.slice. : [Attachment 48354] 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 34652: [chromium] Add the chromium interface to support Blob.slice.
https://bugs.webkit.org/show_bug.cgi?id=34652

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebFileInfo.h
...
> +struct WebFileInfo {
> +    // The file path.
> +    WebString filePath;

^^^ I'm sorry... this is a real pedantic nit-pick, but I think it would be
better to leave out the filePath field here.  I say that because this
structure is otherwise similar to the kinds of structures that an OS would
return when 'stat'ing a file in the filesystem.  You can also look at it 
like this:  you can have many paths referring to the same file.  I'd leave
out the filePath field.

Same goes for the fileStart and fileLength fields.  Those do not describe
the file.  They describe a range of a file, so including them in a struct
named WebFileInfo doesn't seem the best to me.

Be sure to add a comment explaining what fileModificationTime of 0.0 means.

Final nit pick:  no need for the "file" prefix as these fields are all
contained in a structure whose name contains the word file.


> +++ b/WebKit/chromium/public/WebHTTPBody.h
...
> +    WEBKIT_API void appendFile(const WebString&); // FIXME: to be removed.
> +    WEBKIT_API void appendFileRange(const WebFileInfo&);

With the range fields folded into the WebFileInfo structure, this method is now

less explicitly about appending a file range.  I would probably do this:

  void appendFile(const WebString& filePath, const WebFileInfo&, long long
fileStart, long long fileLength);

You could also call it appendFileRange, but I didn't do this since I figured
the appendFile method would be deleted.

-Darin