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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebHTTPBody.h
> @@ -50,6 +50,9 @@ public:
>	   enum { TypeData, TypeFile } type;
>	   WebData data;
>	   WebString filePath;
> +	   long long fileStart;
> +	   long long fileLength;

^^^ do negative values have any meaning here?  i would imagine that fileStart
of 0
and fileLength of -1 means the whole file.  is that your intent?  if so, please

document it.


> +	   time_t fileModificationTime;

^^^ we have been using 'double' for timestamps elsewhere in the API.  there's
only one exception, and that's because i wasn't paying close enough attention
;-)


> @@ -78,6 +81,7 @@ public:
>      // Append to the list of elements.
>      WEBKIT_API void appendData(const WebData&);
>      WEBKIT_API void appendFile(const WebString&);
> +    WEBKIT_API void appendFile(const WebString&, long long fileStart, long
long fileLength, time_t fileModificationTime);

How about naming this new function appendFileRange or appendFileSlice?

Also, do you foresee ever needing additional properties?  E.g., maybe
one day we'll want to pass mimeType info or other timestamps?  Maybe
it would be wise to create a WebFileInfo struct that contains the
lastModified time stamp and pass that as an argument here?  then, we
can extend that in the future.