Bug 36678

Summary: Support using FormData to send a sliced file via XHR.
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore JavaScriptAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dimich, eric, gns, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 36472    
Bug Blocks:    
Attachments:
Description Flags
Proposed Patch
jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Proposed Patch
jianli: commit-queue-
Patch
none
Proposed Patch
dimich: review-, jianli: commit-queue-
Proposed Patch dimich: review+, jianli: commit-queue-

Description Jian Li 2010-03-26 14:52:02 PDT
Add the support to allow FormData.append() to take a Blob and send it correctly via XHR.
Comment 1 Jian Li 2010-03-26 15:35:19 PDT
Created attachment 51787 [details]
Proposed Patch

This depends on 36472. Before it is landed, the compiling queues will show errors for now.
Comment 2 Jian Li 2010-03-26 15:39:22 PDT
Created attachment 51788 [details]
Proposed Patch

Removed the change to project.pbxproj accidentally added into the patch.
Comment 3 WebKit Review Bot 2010-03-26 15:40:20 PDT
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.
Comment 4 Jian Li 2010-03-26 15:40:24 PDT
Created attachment 51789 [details]
Proposed Patch

Sorry, forgot to add it as patch. Try again.
Comment 5 WebKit Review Bot 2010-03-26 15:46:24 PDT
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.
Comment 6 Early Warning System Bot 2010-03-26 15:47:03 PDT
Attachment 51789 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1494001
Comment 7 WebKit Review Bot 2010-03-26 15:53:22 PDT
Attachment 51789 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1495001
Comment 8 WebKit Review Bot 2010-03-26 16:08:53 PDT
Attachment 51789 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1487004
Comment 9 Eric Seidel (no email) 2010-03-26 21:02:09 PDT
Attachment 51789 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1570007
Comment 10 Kinuko Yasuda 2010-03-31 13:16:38 PDT
Created attachment 52204 [details]
Patch
Comment 11 Kinuko Yasuda 2010-03-31 13:19:50 PDT
Comment on attachment 52204 [details]
Patch

Sorry I submitted a patch to a wrong bug...
Comment 12 Early Warning System Bot 2010-03-31 13:23:58 PDT
Attachment 52204 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1640071
Comment 13 Jian Li 2010-03-31 14:35:00 PDT
Created attachment 52211 [details]
Proposed Patch

Updated the patch based on the latest sync so that commit-queue check should function now.
Comment 14 WebKit Review Bot 2010-03-31 14:40:51 PDT
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.
Comment 15 Dmitry Titov 2010-04-09 15:15:54 PDT
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.
Comment 16 Jian Li 2010-04-14 11:27:59 PDT
Created attachment 53347 [details]
Proposed Patch

All fixed.
Comment 17 WebKit Review Bot 2010-04-14 11:29:18 PDT
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.
Comment 18 Dmitry Titov 2010-04-15 13:46:19 PDT
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?
Comment 19 Jian Li 2010-04-15 18:00:15 PDT
Committed as http://trac.webkit.org/changeset/57695.
Comment 20 WebKit Review Bot 2010-04-15 18:08:05 PDT
http://trac.webkit.org/changeset/57695 might have broken Qt Linux Release