[webkit-reviews] review denied: [Bug 36678] Support using FormData to send a sliced file via XHR. : [Attachment 52211] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 36678: Support using FormData to send a sliced file via XHR.
https://bugs.webkit.org/show_bug.cgi?id=36678

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Look good, couple of small things: 

> diff --git
a/LayoutTests/http/tests/local/send-form-data-with-sliced-file.html
b/LayoutTests/http/tests/local/send-form-data-with-sliced-file.html

This looks like a typical script-test. Most (if not all) other script tests
have their .js files in a subdirectory named 'script-tests' and they also have
TEMPLATE.html file there. There is a script in
WebKitTools/Script/make-script-test-erappers which then generates the .html
files using the template. For the consistency, please consider following the
same pattern. If necessary (because of specific template), a
http/tests/local/formdata subdirectory can be created to keep FormData-related
tests.

> diff --git a/WebCore/platform/network/FormData.cpp
b/WebCore/platform/network/FormData.cpp

> @@ -186,9 +188,20 @@ void FormData::appendDOMFormData(const DOMFormData&
domFormData, bool isMultiPar
> +#if ENABLE(BLOB_SLICE)
> +		   String fileName;
> +		   if (value.blob()->isFile())
> +		       fileName = static_cast<File*>(value.blob())->fileName();

> +		   else {
> +		       // If a blob is sliced from a file, it does not have the
filename. In this case, let's produce a unique filename.
> +		       fileName = "Blob" + createCanonicalUUIDString();
> +		       fileName.replace("-", ""); // For safe, remove '-' in
case that certain server does not like it to be in the filename.

May be "// For safety, remove '-' from the filename since some servers may not
like it." ?

> +		   }
> +#else
> +		   String fileName =
static_cast<File*>(value.blob())->fileName();

It seems like a good place to have ASSERT(value.blob()->IsFile());

Doing r- since moving files to script-test might benefit from another look.