FormData allow empty names to append

FormData allow empty names to append

This CL allows FormData submitted with empty name field to be appended. According to xhr spec https://xhr.spec.whatwg.org/#interface-formdata name field should be appended as it is i.e without checking whether it is empty. 
Firefox and IE behaves according to spec and allow empty names field to be appended. This CL makes Blink to be compatible with spec and other browsers.

BUG=415804
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183660

[prashant.hiremath, 2014-09-26 12:19:18]

prashhir@cisco.com changed reviewers:
+ jsbell@chromium.org

[prashant.hiremath, 2014-09-26 12:19:19]

Please Review

[jsbell, 2014-09-26 16:19:17]

jsbell@chromium.org changed reviewers:
+ sigbjornf@opera.com

[jsbell, 2014-09-26 16:19:19]

lgtm with a suggestion

Were you able to run this exact test in Firefox and IE?

...

Blocking empty names went in with the original implementation:

https://bugs.webkit.org/show_bug.cgi?id=35707

There was a note about pinging the spec author - that must not have been
followed through on. If IE/FF already match the spec and Chrome/Safari don't
then the spec could go either way. I like getting rid of special cases, though.

@sof - what do you think? (and you can OWNERS review if you approve)

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:8:
function runTest() {
Since this all runs synchronously I don't think it needs to wait for body.onload
to fire, so you should be able to remove the wrapping function.

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:20: if
(xhrString.response.indexOf('name=""\r\n\r\nempty') >= 0) {
Can you extract this if (s.indexOf(ss)) { testPassed(...); } else {
testFailed(...); } pattern into a helper function, since it occurs 4 times?

shouldContain(string, substring, message) { ... }

shouldContain(xhrString.response, 'name=""\r\b\r\nempty', 'The formdata of
string type containing an empty name should be echoed correctly');

[jsbell, 2014-09-26 16:21:16]

jsbell@chromium.org changed reviewers:
+ jianli@chromium.org

[jsbell, 2014-09-26 16:21:19]

Oh, hey, that WebKit patch came from a Chromium contributor.

@jianli - any comment?

[sof, 2014-09-26 18:02:48]

On 2014/09/26 16:19:19, jsbell wrote:
> lgtm with a suggestion
> 
> Were you able to run this exact test in Firefox and IE?
> 
> ...
> 
> Blocking empty names went in with the original implementation:
> 
> https://bugs.webkit.org/show_bug.cgi?id=35707
> 
> There was a note about pinging the spec author - that must not have been
> followed through on. If IE/FF already match the spec and Chrome/Safari don't
> then the spec could go either way. I like getting rid of special cases,
though.
> 
> @sof - what do you think? (and you can OWNERS review if you approve)
> 

Putting the issue https://bugs.webkit.org/show_bug.cgi?id=35707#c6 to rest
before changing behavior, would be useful. Perhaps it is intentional that
FormData should behave differently than
https://html.spec.whatwg.org/#constructing-form-data-set , but not clear.

I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=26913

[prashant.hiremath, 2014-09-26 18:08:12]

@jsbell, I have tested the sample code which you have added in
https://code.google.com/p/chromium/issues/detail?id=415804 in both Firefox32 and
IE11.

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:8:
function runTest() {
On 2014/09/26 16:19:19, jsbell wrote:
> Since this all runs synchronously I don't think it needs to wait for
body.onload
> to fire, so you should be able to remove the wrapping function.

Done.

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:20: if
(xhrString.response.indexOf('name=""\r\n\r\nempty') >= 0) {
On 2014/09/26 16:19:18, jsbell wrote:
> Can you extract this if (s.indexOf(ss)) { testPassed(...); } else {
> testFailed(...); } pattern into a helper function, since it occurs 4 times?
> 
> shouldContain(string, substring, message) { ... }
> 
> shouldContain(xhrString.response, 'name=""\r\b\r\nempty', 'The formdata of
> string type containing an empty name should be echoed correctly');

One samall doubt, Do you mean to write helper fn for this file or add this
helper fn in resource folder and use it?

[jsbell, 2014-09-26 18:10:41]

On 2014/09/26 18:08:12, prashant.hiremath wrote:
> One samall doubt, Do you mean to write helper fn for this file or add this
> helper fn in resource folder and use it?

In this file is fine.

[jianli, 2014-09-26 21:32:04]

The spec (https://xhr.spec.whatwg.org/#interface-formdata) does not mention
explicitly how the empty name should be treated. The original patch chose to
submit nothing if the empty name is found in order to be consistent with the
behavior in Form element. For example, you can run the following test in Chrome,
Firefox or IE. All of them do not add anything for the empty name:
  <form name="input" action="http://localhost/sometest" method="post">
  Username: <input type="text" name="">
  <input type="submit" value="Submit">
  </form>

Unless the FormData spec explicitly wants to allow submitting the empty name, we
could consider making the change. At this point, we should ping the spec author
to understand what he thinks and the potential discrepancy from the Form element
handling.

[jsbell, 2014-09-26 22:50:28]

On 2014/09/26 21:32:04, jianli wrote:
> The spec (https://xhr.spec.whatwg.org/#interface-formdata) does not mention
> explicitly how the empty name should be treated.

But if a spec doesn't define special behavior, we shouldn't infer it.

That said, I agree that since there's special behavior for form submission, it's
reasonable to discuss a special case here.

> Unless the FormData spec explicitly wants to allow submitting the empty name,
we
> could consider making the change. At this point, we should ping the spec
author
> to understand what he thinks and the potential discrepancy from the Form
element
> handling.

Agreed, and @sof did that by filing bug. We can wait for AnneVK to weigh in.

[sof, 2014-10-06 09:12:34]

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:13:
xhrString.open('POST',
'http://127.0.0.1:8000/xmlhttprequest/resources/post-echo.cgi', false);
Could you make this be async instead (here and below) ?

[prashant.hiremath, 2014-10-06 12:31:59]

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:13:
xhrString.open('POST',
'http://127.0.0.1:8000/xmlhttprequest/resources/post-echo.cgi', false);
On 2014/10/06 09:12:34, sof wrote:
> Could you make this be async instead (here and below) ?
Actually initially itself I tried making it async, but met with some problems.
As for async tests I need to set self.jsTestIsAsync to true, and once test gets
completed I need to call finishJSTest(). But we will not be sure which response
i.e string or file type response would come first (Correct me if I'm wrong)?. So
either I need to split these two cases into separate files or is there any
alternative, please let me know :)

[sof, 2014-10-06 14:46:48]

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:13:
xhrString.open('POST',
'http://127.0.0.1:8000/xmlhttprequest/resources/post-echo.cgi', false);
On 2014/10/06 12:31:59, prashant.hiremath wrote:
> On 2014/10/06 09:12:34, sof wrote:
> > Could you make this be async instead (here and below) ?
> Actually initially itself I tried making it async, but met with some problems.
> As for async tests I need to set self.jsTestIsAsync to true, and once test
gets
> completed I need to call finishJSTest(). But we will not be sure which
response
> i.e string or file type response would come first (Correct me if I'm wrong)?.
So
> either I need to split these two cases into separate files or is there any
> alternative, please let me know :)

Sure, fast and stable tests are definitely to be preferred :-) The tests in
http/tests/xmlhttprequest/ might be helpful to you here; i.e., you need to chain
the async tests, so as to get stable test outputs.

post-blob-content-type-async.html is one test that does this, but I don't think
you need to push it quite that far in terms of abstraction (but you're welcome
to :) )

(Also, there's no need to prefix "http://127.0.0.1:8000/" to the request URLs,
just the path will do.)

[sof, 2014-10-07 08:59:41]

On 2014/09/26 22:50:28, jsbell wrote:
> On 2014/09/26 21:32:04, jianli wrote:
> > The spec (https://xhr.spec.whatwg.org/#interface-formdata) does not mention
> > explicitly how the empty name should be treated.
> 
> But if a spec doesn't define special behavior, we shouldn't infer it.
> 
> That said, I agree that since there's special behavior for form submission,
it's
> reasonable to discuss a special case here.
> 
> > Unless the FormData spec explicitly wants to allow submitting the empty
name,
> we
> > could consider making the change. At this point, we should ping the spec
> author
> > to understand what he thinks and the potential discrepancy from the Form
> element
> > handling.
> 
> Agreed, and @sof did that by filing bug. We can wait for AnneVK to weigh in.

No immediate conclusion on that bug/request-for-clarification.

We can wait, but an alternative is to align with others for now & follow up on
any spec changes later - wdyt?

[jsbell, 2014-10-07 17:03:45]

On 2014/10/07 08:59:41, sof wrote:
> We can wait, but an alternative is to align with others for now & follow up on
> any spec changes later - wdyt?

If we align with FF/IE, then 3/4 browsers will have the same behavior and it's a
foregone conclusion that the spec will stay as is.

So if we think there's any merit to having the special case we should discuss it
before making the change.

(I'm not a fan of special cases but this one is legitimate to consider since
there's either a special case in the API or a deviation between declarative and
procedural API. I'm glad jianli noticed this!)

[sof, 2014-10-08 09:40:23]

Cc: +arv (just in case he missed it.)

[arv (Not doing code reviews), 2014-10-08 14:05:27]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2014-10-08 14:05:27]

LGTM

Less special cases are an improvement.

[prashant.hiremath, 2014-10-09 10:53:38]

PTAL

[prashant.hiremath, 2014-10-09 10:58:45]

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/1/LayoutTests/http/tests/local...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:13:
xhrString.open('POST',
'http://127.0.0.1:8000/xmlhttprequest/resources/post-echo.cgi', false);
On 2014/10/06 14:46:48, sof wrote:
> (Also, there's no need to prefix "http://127.0.0.1:8000/" to the request URLs,
> just the path will do.)

I tried giving directly /xmlhttprequest/resources/post-echo.cgi' but response
was coming empty, when i print xhrString.response.
I tried giving relative path i.w ../../xmlhttprequest/resources/post-echo.cgi'
but this started printing cgi script content in response, so was not sure what
to do, kept this change to
localhost:8000/xmlhttprequest/resources/post-echo.cgi',

[sof, 2014-10-10 06:27:11]

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:6:
description("Test that we can send empty name in Formdata.");
"Test that empty names can be used in FormData.append()" ?

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:14:
testFailed("the formatdata containing"+ substring + " is failed to echo
correctly");
formatdata => FormData.
is failed => failed.

Also, add a space before the initial (+), and leave out the braces ({}) for the
if statement.

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:18:
var formDataString = new FormData();
Could you wrap this up as firstTest() and then invoke it initially? A bit easier
to work with should you want to leave it out while debugging some issue here
later.

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:18:
var formDataString = new FormData();
The String variable suffix doesn't seem to add much, leave it out (for FormData
and XMLHttpRequest)?

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:22:
xhrString.open('POST',
'http://localhost:8000/xmlhttprequest/resources/post-echo.cgi', true);
local/ do run as local tests (despite being in http/tests/ ..), so sorry if I
confused you. Could you change this back to 127.0.0.1 instead of localhost ?

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:24:
stringResponse = function() {
Just inline this as

xhrString.onload = function () {
  ...
};

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:33:
xhrString.onreadystatechange = stringResponse;
onload is simpler, I think. Then you don't need to check for readyState.

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:37:
var formDataFile = new FormData();
The File variable suffix doesn't seem to add much, leave it out (for FormData
and XMLHttpRequest)?

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:41:
xhrFile.open('POST',
'http://localhost:8000/xmlhttprequest/resources/post-echo.cgi', true);
localhost => 127.0.0.1

https://codereview.chromium.org/609733004/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:43:
fileResponse = function() {
xhr.onload = function () { 
   ...
};

[sof, 2014-10-10 06:28:05]

On 2014/10/08 14:05:27, arv wrote:
> LGTM
> 
> Less special cases are an improvement.

Could you follow up on https://www.w3.org/Bugs/Public/show_bug.cgi?id=26913 with
that conclusion (perhaps with more reasoning)?

[sof, 2014-10-10 06:37:13]

Commit message titles that summarize the change made, are preferable.

So, if you could change this to

 FormData: allow empty names

or something of the kind, that'd be good.

The commit message itself could also be made a bit more focused, just saying
what's now allowed. Along with a spec reference (
https://xhr.spec.whatwg.org/#create-an-entry ? ) why and a compatibility mention
of how other browsers behave.

[prashant.hiremath, 2014-10-10 12:36:35]

PTAL

[sof, 2014-10-10 12:43:39]

Could you add the title to the description also, e.g.,

FormData allow empty names to append

...existing description...

https://codereview.chromium.org/609733004/diff/80001/LayoutTests/http/tests/l...
File LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html
(right):

https://codereview.chromium.org/609733004/diff/80001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:6:
description("Test that we can send empty name in Formdata.append()");
Formdata => FormData

https://codereview.chromium.org/609733004/diff/80001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:14:
testFailed("the Formdata containing" + substring + " failed to echo correctly");
Formdata => FormData

https://codereview.chromium.org/609733004/diff/80001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:25:
'the formdata of string type containing a empty name echoed correctly');
formdata => FormData (same for later occurrences of formdata.)

https://codereview.chromium.org/609733004/diff/80001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/send-form-data-with-empty-name.html:41:
'the formdata of file type containing a empty name echoed correctly.');
Remove final "." for consistency.

[prashant.hiremath, 2014-10-13 05:23:06]

Added issue title in Description, PTAL.

[sof, 2014-10-13 08:43:13]

On 2014/10/13 05:23:06, prashant.hiremath wrote:
> Added issue title in Description, PTAL.

Thanks for iterating on the test so as to get it into a tidy shape, looks fine
to me now.

[prashant.hiremath, 2014-10-13 09:51:48]

On 2014/10/13 08:43:13, sof wrote:
> On 2014/10/13 05:23:06, prashant.hiremath wrote:
> > Added issue title in Description, PTAL.
> 
> Thanks for iterating on the test so as to get it into a tidy shape, looks fine
> to me now.

@sof & @jsbell Thanks for guiding me in this whole process :). Should I commit
changes?

[sof, 2014-10-13 17:41:08]

On 2014/10/13 09:51:48, prashant.hiremath wrote:
> On 2014/10/13 08:43:13, sof wrote:
> > On 2014/10/13 05:23:06, prashant.hiremath wrote:
> > > Added issue title in Description, PTAL.
> > 
> > Thanks for iterating on the test so as to get it into a tidy shape, looks
fine
> > to me now.
> 
> @sof & @jsbell Thanks for guiding me in this whole process :). Should I commit
> changes?

Please do, lgtm.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26913 has now been resolved;
empty names are allowed/admitted.

[jsbell, 2014-10-13 17:42:33]

Thanks for driving resolution of this on the standards front, sof@

[prashant.hiremath, 2014-10-14 02:19:33]

The CQ bit was checked by prashhir@cisco.com

[commit-bot: I haz the power, 2014-10-14 02:20:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/609733004/130001

[commit-bot: I haz the power, 2014-10-14 08:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-14 08:20:47]

Exceeded time limit waiting for builds to trigger.

[prashant.hiremath, 2014-10-14 09:02:42]

The CQ bit was checked by prashhir@cisco.com

[commit-bot: I haz the power, 2014-10-14 09:03:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/609733004/130001

[commit-bot: I haz the power, 2014-10-14 09:48:52]

Committed patchset #4 (id:130001) as 183660