Bug 34652 – [chromium] Add the chromium interface to support Blob.slice.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jian Li]

In order to commit patch 32993 (Blob.slice support) without breaking chromium build, we need to add the chromium interface first.

[Jian Li]

Created attachment 48241 [details]
Proposed Patch

[Dmitry Titov]

Darin Fisher is the guy to look at chromium/public change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48241 [details]
Proposed Patch

> +++ 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.

[Jian Li]

Created attachment 48350 [details]
Proposed Patch

All fixed. Thanks.

[WebKit Review Bot]

Attachment 48350 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebHTTPBody.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jian Li]

Created attachment 48354 [details]
Proposed Patch

Fixed style error.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48354 [details]
Proposed Patch

> +++ 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

[Jian Li]

Created attachment 48429 [details]
Proposed Patch

All fixed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48429 [details]
Proposed Patch

> +++ b/WebKit/chromium/public/WebFileInfo.h
...
> +#include "WebString.h"

^^^ you can nuke this include now.


> +++ b/WebKit/chromium/public/WebHTTPBody.h

> +        long long fileStart;
> +        long long fileLength; // -1 means the whole file.

^^^ nit: -1 means to the end of the file.  it would only mean the whole file
if fileStart were 0.


> +    WEBKIT_API void appendFile(const WebString&); // FIXME: to be removed.
> +    WEBKIT_API void appendFile(const WebString&, long long fileStart, long long fileLength, const WebFileInfo&);

^^^ please document the magic value of -1 here as well for fileLength.


otherwise, LGTM.  R=me, assuming you fixup these nits before committing.

[Jian Li]

Committed as http://trac.webkit.org/changeset/54565.