Support FormData on WorkerGlobalScope.

Support FormData on WorkerGlobalScope.

The spec defines FormData as being provided in worker contexts,

  http://www.w3.org/TR/XMLHttpRequest/#interface-formdata
  http://xhr.spec.whatwg.org/#interface-formdata

Follow suit and provide an argument-less FormData constructor and the
interface object on WorkerGlobalScope.

R=arv@chromium.org,nbarth@chromium.org
BUG=360546
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171294

[sof, 2014-04-07 13:03:42]

Please take a look.

An open question here is if http://www.chromium.org/blink#trivial-changes
applies wrt http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process

[arv (Not doing code reviews), 2014-04-07 21:16:38]

LGTM with nits

https://codereview.chromium.org/227483008/diff/1/LayoutTests/fast/workers/res...
File LayoutTests/fast/workers/resources/worker-formdata.js (right):

https://codereview.chromium.org/227483008/diff/1/LayoutTests/fast/workers/res...
LayoutTests/fast/workers/resources/worker-formdata.js:12: shouldBeTrue("'append'
in formData");
I always prefer more specific tests than `in`. Can you do hasOwnProperty of
FormData.prototype instead?

https://codereview.chromium.org/227483008/diff/1/LayoutTests/http/tests/xmlht...
File LayoutTests/http/tests/xmlhttprequest/resources/post-formdata.js (right):

https://codereview.chromium.org/227483008/diff/1/LayoutTests/http/tests/xmlht...
LayoutTests/http/tests/xmlhttprequest/resources/post-formdata.js:59: if
(this.self)
This is always true.

window === self
window === this
window.self === window
window.window === window

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.idl
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.id...
Source/core/html/FormData.idl:32: Constructor([Default=Undefined] optional
HTMLFormElement form),
Can we skip [Default=Undefined] and just declare a constructor with
`HTMLFormElement* form = 0` instead?

[sof, 2014-04-08 15:10:53]

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.idl
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.id...
Source/core/html/FormData.idl:32: Constructor([Default=Undefined] optional
HTMLFormElement form),
On 2014/04/07 21:16:38, arv wrote:
> Can we skip [Default=Undefined] and just declare a constructor with
> `HTMLFormElement* form = 0` instead?

Ought to, but non-default optional constructor args aren't handled correctly by
the code generator. Addressing.

[sof, 2014-04-08 19:36:50]

Many thanks for the review.

The handling of optional, non-default constructor arguments wasn't quite in
place, so I've addressed that on this CL. Cc:ed some bindings owners that might
have time to for a look. The generated code for this optional case isn't as
minimal as it could be, but given that this isn't a very common case, I
considered that acceptable.

https://codereview.chromium.org/227483008/diff/1/LayoutTests/fast/workers/res...
File LayoutTests/fast/workers/resources/worker-formdata.js (right):

https://codereview.chromium.org/227483008/diff/1/LayoutTests/fast/workers/res...
LayoutTests/fast/workers/resources/worker-formdata.js:12: shouldBeTrue("'append'
in formData");
On 2014/04/07 21:16:38, arv wrote:
> I always prefer more specific tests than `in`. Can you do hasOwnProperty of
> FormData.prototype instead?

Noted; done.

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.idl
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/227483008/diff/1/Source/core/html/FormData.id...
Source/core/html/FormData.idl:32: Constructor([Default=Undefined] optional
HTMLFormElement form),
On 2014/04/07 21:16:38, arv wrote:
> Can we skip [Default=Undefined] and just declare a constructor with
> `HTMLFormElement* form = 0` instead?

That's simpler, changed to do so. Also removed the passing of ExecutionContext
to the constructor, it wasn't serving an effective purpose.

[Nils Barth (inactive), 2014-04-09 02:00:27]

Various suggestions for Python + Jinja;
semantics seem fine.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:491: this_cpp_value =
v8_methods.cpp_value(interface, constructor, index, for_constructor=True)
Could you inline this expression, as it's not being used elsewhere?

Per:
Jinja: Style: Python: One dictionary display per context, inline expressions
http://www.chromium.org/developers/jinja#TOC-One-dictionary-display-per-conte...

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:183: def cpp_value(interface, method,
number_of_arguments, for_constructor=False):
I don't think for_constructor is necessary:
you can just check method.is_constructor

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:217: cpp_method_name = "%s::create" %
v8_utilities.cpp_name(interface)
I think you can push this into v8_utilities.scoped_name:
just add:
if definition.is_constructor:
    return '%s::%s' % (cpp_name(interface), 'create')
...to it.

Also, single quotes please.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:202: {{ref_ptr}}<{{cpp_class}}> impl =
{{cpp_value}};
Can you reuse the |result| branch here, instead of adding a separate |impl|
branch?
(I think you just need to change |impl| to |result| in constructor macros
below.)
Even if not, could you move this branch down (below void) so analogy with
impl/result is closer?

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:406: {# Emit code to associate wrapper
with constructed object. #}
Could you remove the "Emit code to", since that's what templates do?

[Nils Barth (inactive), 2014-04-09 02:12:03]

One other note: shouldn't you be *adding* a test case, not replacing one?

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceConstructor2.idl (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceConstructor2.idl:44: optional DOMString
optionalStringArg,
Shouldn't you *add* a new test case, rather than *replacing* an existing one?
...or are you replacing all uses of
[Default=Undefined] optional DOMString
?

[sof, 2014-04-09 07:31:59]

Not to make named constructors feel left out, I also added support for these.
Please take a look, hope I'm not extending this CL too far.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:491: this_cpp_value =
v8_methods.cpp_value(interface, constructor, index, for_constructor=True)
On 2014/04/09 02:00:27, Nils Barth wrote:
> Could you inline this expression, as it's not being used elsewhere?
> 
> Per:
> Jinja: Style: Python: One dictionary display per context, inline expressions
>
http://www.chromium.org/developers/jinja#TOC-One-dictionary-display-per-conte...

Done + sorted the fields.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:183: def cpp_value(interface, method,
number_of_arguments, for_constructor=False):
On 2014/04/09 02:00:27, Nils Barth wrote:
> I don't think for_constructor is necessary:
> you can just check method.is_constructor

'method' is an IdlOperation/IdlAttribute/.. object, so that wouldn't quite work.
I've switched to doing a bit of type testing with .name, however - hope that
addresses the concern.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:217: cpp_method_name = "%s::create" %
v8_utilities.cpp_name(interface)
On 2014/04/09 02:00:27, Nils Barth wrote:
> I think you can push this into v8_utilities.scoped_name:
> just add:
> if definition.is_constructor:
>     return '%s::%s' % (cpp_name(interface), 'create')
> ...to it.
> 
> Also, single quotes please.

For the same reasons as the above comment, attribute isn't present on this
object. Have tweaked the code a bit to allow reuse of scoped_name() though,
again consulting .name.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:202: {{ref_ptr}}<{{cpp_class}}> impl =
{{cpp_value}};
On 2014/04/09 02:00:27, Nils Barth wrote:
> Can you reuse the |result| branch here, instead of adding a separate |impl|
> branch?
> (I think you just need to change |impl| to |result| in constructor macros
> below.)
> Even if not, could you move this branch down (below void) so analogy with
> impl/result is closer?

I've moved it down a bit. There is a mismatch between (local) names and their
mapped types here to prevent your suggestion from immediately being applicable.
I don't see significant value in trying to make the 'result' disjuntion even
longer and sharing one line of code.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:406: {# Emit code to associate wrapper
with constructed object. #}
On 2014/04/09 02:00:27, Nils Barth wrote:
> Could you remove the "Emit code to", since that's what templates do?

Removed comment.

Extended the use of this macro - the named constructor callbacks also emit its
content.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceConstructor2.idl (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceConstructor2.idl:44: optional DOMString
optionalStringArg,
On 2014/04/09 02:12:03, Nils Barth wrote:
> Shouldn't you *add* a new test case, rather than *replacing* an existing one?
> ...or are you replacing all uses of
> [Default=Undefined] optional DOMString
> ?

That makes good sense :) Thought there was coverage elsewhere of that combo, but
not so.

[Nils Barth (inactive), 2014-04-09 08:34:33]

On 2014/04/09 07:31:59, sof wrote:
> Not to make named constructors feel left out, I also added support for these.
> Please take a look, hope I'm not extending this CL too far.

You might want to split out the CG changes into a separate CL;
that would keep "new functionality" and "CG changes" clearly separate.

(I'm ok with committing as is, given that we've been through this on review,
but might be clearer in general to post separate dependent CLs?)

[Nils Barth (inactive), 2014-04-09 09:36:38]

Thanks for revisions!
A few nits, otherwise LGTM for bindings/.

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:183: def cpp_value(interface, method,
number_of_arguments, for_constructor=False):
On 2014/04/09 07:31:59, sof wrote:
> 'method' is an IdlOperation/IdlAttribute/.. object, so that wouldn't quite
work.

Got it!

> I've switched to doing a bit of type testing with .name, however - hope that
> addresses the concern.

It does -- much clearer, thanks!

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:530: constructor['name'] =
extended_attributes['NamedConstructor']
As it's 2+ new values, could you use update instead?
constructor.update({
    'name': extended_attributes['NamedConstructor'],
    'is_named_constructor': True,
})

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:209: this_union_arguments =
method.idl_type and method.idl_type.union_arguments
Is this |method.idl_type| check necessary?
(It looks a bit weird.)

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:118: if definition.name in
('Constructor', 'NamedConstructor'):
How about joining with previous condition, as you're using the same result?
if definition.is_static or definition.name in ('Constructor',
'NamedConstructor'):

...right?

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/template...
Source/bindings/templates/methods.cpp:409: {% set constructor_suffix =
'Constructor' if constructor.is_named_constructor else '' %}
Maybe a bit clearer is:
{% set constructor_class = v8_class + 'Constructor' if
constructor.is_named_constructor
                           else v8_class %}
(Prefer to have full names in values.)

(Fixing the ugly naming of named constructors is on my todo list.)

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceConstructor2.idl (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceConstructor2.idl:45: optional DOMString
optionalStringArg,
You mean to add another one here, right?
I.e., keep the existing 2 lines as is, add
optional DOMString optionalStringArg,
...as the last argument.

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceNamedConstructor.idl (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceNamedConstructor.idl:36: optional
DOMString optionalStringArg,
Could you move this to the bottom?
(Generally optional arguments w/o defaults should go at the end,
so that optional arguments w/ defaults can be implemented as required
arguments. This isn't required per spec, but it's more realistic.)

[sof, 2014-04-09 09:40:47]

On 2014/04/09 08:34:33, Nils Barth wrote:
> On 2014/04/09 07:31:59, sof wrote:
> > Not to make named constructors feel left out, I also added support for
these.
> > Please take a look, hope I'm not extending this CL too far.
> 
> You might want to split out the CG changes into a separate CL;
> that would keep "new functionality" and "CG changes" clearly separate.
> 
> (I'm ok with committing as is, given that we've been through this on review,
> but might be clearer in general to post separate dependent CLs?)

ok, let's keep them separate - split out the bindings/ changes as
https://codereview.chromium.org/229373006/.

Will address the feedback added here (just now - thanks :) ) via it.

[Nils Barth (inactive), 2014-04-09 10:00:08]

On 2014/04/09 09:40:47, sof wrote:
> ok, let's keep them separate - split out the bindings/ changes as
> https://codereview.chromium.org/229373006/.
> 
> Will address the feedback added here (just now - thanks :) ) via it.

Thanks sof!

[sof, 2014-04-09 10:53:15]

Thanks again; followed up on these on https://codereview.chromium.org/229373006/

(Will rebase this CL once it is available. We're done here now, I think.)

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:530: constructor['name'] =
extended_attributes['NamedConstructor']
On 2014/04/09 09:36:38, Nils Barth wrote:
> As it's 2+ new values, could you use update instead?
> constructor.update({
>     'name': extended_attributes['NamedConstructor'],
>     'is_named_constructor': True,
> })

Done.

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:209: this_union_arguments =
method.idl_type and method.idl_type.union_arguments
On 2014/04/09 09:36:38, Nils Barth wrote:
> Is this |method.idl_type| check necessary?
> (It looks a bit weird.)

Yes, IdlOperations for constructors has idl_type as None is why.

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:118: if definition.name in
('Constructor', 'NamedConstructor'):
On 2014/04/09 09:36:38, Nils Barth wrote:
> How about joining with previous condition, as you're using the same result?
> if definition.is_static or definition.name in ('Constructor',
> 'NamedConstructor'):
> 
> ...right?

Done.

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/template...
Source/bindings/templates/methods.cpp:409: {% set constructor_suffix =
'Constructor' if constructor.is_named_constructor else '' %}
On 2014/04/09 09:36:38, Nils Barth wrote:
> Maybe a bit clearer is:
> {% set constructor_class = v8_class + 'Constructor' if
> constructor.is_named_constructor
>                            else v8_class %}
> (Prefer to have full names in values.)
> 
> (Fixing the ugly naming of named constructors is on my todo list.)

Certainly would be clearer that way; done.

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceConstructor2.idl (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceConstructor2.idl:45: optional DOMString
optionalStringArg,
On 2014/04/09 09:36:38, Nils Barth wrote:
> You mean to add another one here, right?
> I.e., keep the existing 2 lines as is, add
> optional DOMString optionalStringArg,
> ...as the last argument.

Moved it down (and changed naming of 3rd arg back to previous.)

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestInterfaceNamedConstructor.idl (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestInterfaceNamedConstructor.idl:36: optional
DOMString optionalStringArg,
On 2014/04/09 09:36:38, Nils Barth wrote:
> Could you move this to the bottom?
> (Generally optional arguments w/o defaults should go at the end,
> so that optional arguments w/ defaults can be implemented as required
> arguments. This isn't required per spec, but it's more realistic.)

Done.

[Nils Barth (inactive), 2014-04-10 01:55:59]

Thanks for revisions (in separate CL)!

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/227483008/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:209: this_union_arguments =
method.idl_type and method.idl_type.union_arguments
On 2014/04/09 10:53:16, sof wrote:
> On 2014/04/09 09:36:38, Nils Barth wrote:
> > Is this |method.idl_type| check necessary?
> > (It looks a bit weird.)
> 
> Yes, IdlOperations for constructors has idl_type as None is why.

Got it, I suspected there was something like that - thanks!
I'll see if I can fix this by ensuring that constructors also have
an idl_type (namely the interface type);
no action needed by you here.

[Nils Barth (inactive), 2014-04-10 02:16:22]

On 2014/04/09 10:53:15, sof wrote:
> (Will rebase this CL once it is available. We're done here now, I think.)

Why wait? >;)

It's possible to rebase the CL off a branch even before it lands, using the
"branch off a branch" workflow:
http://www.chromium.org/developers/how-tos/get-the-code#TOC-Branch-off-a-branch
...specifically the "splitting up a CL" process:
http://www.chromium.org/developers/how-tos/get-the-code#TOC-Splitting-up-a-CL

(...which I wrote for this occasion.)

Admittedly, in this case this CL looks done and just needs rebasing to the CG
CL,
but if you'd like to get the rebased CL up sooner (and have some git fun),
try it out!

[sof, 2014-04-10 16:24:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-10 16:24:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/227483008/80001

[commit-bot: I haz the power, 2014-04-10 17:00:15]

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

[commit-bot: I haz the power, 2014-04-10 17:00:15]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[sof, 2014-04-10 17:56:49]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-10 17:56:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/227483008/80001

[commit-bot: I haz the power, 2014-04-10 18:27:00]

Change committed as 171294