Bug 36678 – Support using FormData to send a sliced file via XHR.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

Add the support to allow FormData.append() to take a Blob and send it correctly via XHR.

[Jian Li]

Created attachment 51787 [details]
Proposed Patch

This depends on 36472. Before it is landed, the compiling queues will show errors for now.

[Jian Li]

Created attachment 51788 [details]
Proposed Patch

Removed the change to project.pbxproj accidentally added into the patch.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/FormDataList.h:41:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 18 files


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

[Jian Li]

Created attachment 51789 [details]
Proposed Patch

Sorry, forgot to add it as patch. Try again.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/FormDataList.h:41:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 18 files


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

[Early Warning System Bot]

Attachment 51789 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1494001

[WebKit Review Bot]

Attachment 51789 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1495001

[WebKit Review Bot]

Attachment 51789 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1487004

[Eric Seidel (no email)]

Attachment 51789 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1570007

[Kinuko Yasuda]

Created attachment 52204 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 52204 [details]
Patch

Sorry I submitted a patch to a wrong bug...

[Early Warning System Bot]

Attachment 52204 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1640071

[Jian Li]

Created attachment 52211 [details]
Proposed Patch

Updated the patch based on the latest sync so that commit-queue check should function now.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/FormDataList.h:41:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 18 files


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

[Dmitry Titov]

Comment on attachment 52211 [details]
Proposed Patch

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.

[Jian Li]

Created attachment 53347 [details]
Proposed Patch

All fixed.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Skipping non-existent file: LayoutTests/http/tests/local/resources/send-form-data.js
WARNING: Skipping non-existent file: LayoutTests/http/tests/local/send-form-data-expected.txt
WebCore/html/FormDataList.h:41:  More than one command on the same line  [whitespace/newline] [4]
WARNING: Skipping non-existent file: LayoutTests/http/tests/local/send-form-data.html
Total errors found: 1 in 20 files


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

[Dmitry Titov]

Comment on attachment 53347 [details]
Proposed Patch

r=me

tiny nit:

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog

> +        Move common functionalities to test sending FormData into a helper file

s/functionalities/functionality/ 

Also, the style bot complained because the change happens around old code with style violation in it. Perhaps it would be nice to have a separate patch to fix the style of that code?

[Jian Li]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/57695 might have broken Qt Linux Release