IDL: reuse more code between CG for methods and constructors

IDL: reuse more code between CG for methods and constructors

The CG for constructors and named constructors had its own simplified
mechanism for generating the actual call to the C++ constructor, rather
than using v8_methods.cpp_value(). Except for constructors with
optional arguments without default values, which used cpp_value() via
v8_methods.generate_argument() to generate the "short-cut" call used
when the function is called without the optional arguments.

With this patch, v8_methods.cpp_value() is used for all calls. This
will make it easier to adjust the handling of arguments later on in the
more complex cases.

Some IDL files that declare named constructors are modified by adding
ConstructorCallWith=Document, which was previously implied for named
constructors.

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

[Jens Widell, 2014-06-17 10:40:21]

Please review.

Cleanup to make other argument handling changes more straight-forward, by not
having (as much) special code for constructors.

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:492: ExecutionContext* scriptContext =
currentExecutionContext(isolate);
Renamed to make [CallWith] handling more uniform; it's named scriptContext in
methods already.

This rename, and the DocumentPtr*/Document& tweaking for named constructors, is
the only CG changes caused by this patch.

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:538: Document* documentPtr =
currentDOMWindow(isolate)->document();
Since this code is here, unconditionally, CallWith=Document is still partially
implied for named constructors. It would be nicer if it was all or nothing;
either making this code conditional, or really making it fully implied (and
revert the IDL file changes from this CL.)

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
Introduced to make [CallWith] handling more uniform. It's a reference in methods
already.

[haraken, 2014-06-17 11:28:05]

Hmm, I'm neutral about this change.

constructor and method are different things. As commented below, if we remove
ConstructorCallWith=Document, can't we remove most of the code duplication we
want to remove?

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:492: ExecutionContext* scriptContext =
currentExecutionContext(isolate);
On 2014/06/17 10:40:20, Jens Lindström wrote:
> Renamed to make [CallWith] handling more uniform; it's named scriptContext in
> methods already.
> 
> This rename, and the DocumentPtr*/Document& tweaking for named constructors,
is
> the only CG changes caused by this patch.

scriptContext sounds unnatural. Shall we rename it to exeuctionContext or
context in other places as well?

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
On 2014/06/17 10:40:20, Jens Lindström wrote:
> Introduced to make [CallWith] handling more uniform. It's a reference in
methods
> already.

Given that we're planning to remove CallWith=Document, shall we keep this code
as is?

https://codereview.chromium.org/340443004/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestInterfaceEventTarget.idl (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestInterfaceEventTarget.idl:33:
ConstructorCallWith=Document,

Actually we should remove CallWith=Document. It's doing almost the same thing as
CallWith=ExecutionContext.

[Jens Widell, 2014-06-17 12:29:41]

On 2014/06/17 11:28:05, haraken wrote:
> Hmm, I'm neutral about this change.

I shall try to sell it better then. ;-)

> constructor and method are different things.

They are different, but they share how they process arguments. Same script code
generates data, same template macros generate code. That already shared code
doesn't fully support constructors, which this patch addresses. And while
addressing it, share the code some more.

Some example situations where we generate broken code without this patch:

- Interface with named constructor and an optional argument (without default):
The code generated for the "argument missing" has no document argument.

- Interface with named constructor and ConstructorCallWith=Document: The call
generated code is createForJSConstructor(*document, document, ...).

- Interface with both constructor and named constructor, and an optional
argument (without default): If the named constructor is called with no
arguments, {{class}}::create() is called instead of
{{class}}::createForJSConstructor(). (Maybe combining constructor and named
constructor is inherently wrong?)

- Constructor with a "callback interface" argument: The argument in the
generated C++ call is "callback" instead of "callback.release()" (the "callback"
variable will be an OwnPtr<>.)

None of this affects current code, and maybe these are situations that are
unlikely to ever occur, but it still feels like having one code path that
handles all the (very similar) cases correctly is better than what we have
today.

And of course: I didn't do this to fix any of the above. I did it because I
wanted to adjust how arguments are handled, and those adjustments wouldn't apply
to constructors unless I duplicated them there, or did this.

> As commented below, if we remove
> ConstructorCallWith=Document, can't we remove most of the code duplication we
> want to remove?

This wasn't so much about removing current code duplication, but rather
extending argument handling without needing to introduce code duplication to
support constructors as well.

Given HTMLOptionElement's named Option constructor, it doesn't seem reasonable
to assume that constructors only have "simple" argument lists (which is exactly
the assumption that the current code makes.)

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
On 2014/06/17 11:28:05, haraken wrote:
> On 2014/06/17 10:40:20, Jens Lindström wrote:
> > Introduced to make [CallWith] handling more uniform. It's a reference in
> methods
> > already.
> 
> Given that we're planning to remove CallWith=Document, shall we keep this code
> as is?

Yeah, maybe. Would you prefer I upload a patch that does that first?

You're talking about replacing all uses of Document with ExecutionContext and
let the C++ implementation fetch the document instead, here? As is talked about
in a FIXME in IDLExtendedAttributes.txt?

[haraken, 2014-06-17 12:38:54]

Thanks for the details; I'm convinced :)

LGTM.

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:492: ExecutionContext* scriptContext =
currentExecutionContext(isolate);
On 2014/06/17 11:28:05, haraken wrote:
> On 2014/06/17 10:40:20, Jens Lindström wrote:
> > Renamed to make [CallWith] handling more uniform; it's named scriptContext
in
> > methods already.
> > 
> > This rename, and the DocumentPtr*/Document& tweaking for named constructors,
> is
> > the only CG changes caused by this patch.
> 
> scriptContext sounds unnatural. Shall we rename it to exeuctionContext or
> context in other places as well?

Let's use |executionContext| consistently.

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
On 2014/06/17 12:29:41, Jens Lindström wrote:
> On 2014/06/17 11:28:05, haraken wrote:
> > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > Introduced to make [CallWith] handling more uniform. It's a reference in
> > methods
> > > already.
> > 
> > Given that we're planning to remove CallWith=Document, shall we keep this
code
> > as is?
> 
> Yeah, maybe. Would you prefer I upload a patch that does that first?
> 
> You're talking about replacing all uses of Document with ExecutionContext and
> let the C++ implementation fetch the document instead, here? As is talked
about
> in a FIXME in IDLExtendedAttributes.txt?

Yes. A follow-up CL is fine :)

[Jens Widell, 2014-06-17 12:53:16]

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:492: ExecutionContext* scriptContext =
currentExecutionContext(isolate);
On 2014/06/17 12:38:53, haraken wrote:
> On 2014/06/17 11:28:05, haraken wrote:
> > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > Renamed to make [CallWith] handling more uniform; it's named scriptContext
> in
> > > methods already.
> > > 
> > > This rename, and the DocumentPtr*/Document& tweaking for named
constructors,
> > is
> > > the only CG changes caused by this patch.
> > 
> > scriptContext sounds unnatural. Shall we rename it to exeuctionContext or
> > context in other places as well?
> 
> Let's use |executionContext| consistently.

Done.

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:538: Document* documentPtr =
currentDOMWindow(isolate)->document();
On 2014/06/17 10:40:20, Jens Lindström wrote:
> Since this code is here, unconditionally, CallWith=Document is still partially
> implied for named constructors. It would be nicer if it was all or nothing;
> either making this code conditional, or really making it fully implied (and
> revert the IDL file changes from this CL.)

This code uses 'currentDOMWindow(isolate)->document()' and regular constructors
use 'toDocument(currentExecutionContext(isolate))'. Are those two different in
any way?

[haraken, 2014-06-17 12:57:40]

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:538: Document* documentPtr =
currentDOMWindow(isolate)->document();
On 2014/06/17 12:53:16, Jens Lindström wrote:
> On 2014/06/17 10:40:20, Jens Lindström wrote:
> > Since this code is here, unconditionally, CallWith=Document is still
partially
> > implied for named constructors. It would be nicer if it was all or nothing;
> > either making this code conditional, or really making it fully implied (and
> > revert the IDL file changes from this CL.)
> 
> This code uses 'currentDOMWindow(isolate)->document()' and regular
constructors
> use 'toDocument(currentExecutionContext(isolate))'. Are those two different in
> any way?

The issue I want to address is just that we have both
[ConstructorCallWith=ExecutionContext] and [ConstructorCallWith=Document], and
both [CallWith=ExecutionContext] and [CallWith=Document]. Given that
ExecutionContext is the same thing as Document, I want to remove either of them.
(I haven't looked at the generate code in details; so I'm not sure which should
be removed.)

[Jens Widell, 2014-06-17 13:04:31]

On 2014/06/17 12:57:40, haraken wrote:
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:538: Document* documentPtr =
> currentDOMWindow(isolate)->document();
> On 2014/06/17 12:53:16, Jens Lindström wrote:
> > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > Since this code is here, unconditionally, CallWith=Document is still
> partially
> > > implied for named constructors. It would be nicer if it was all or
nothing;
> > > either making this code conditional, or really making it fully implied
(and
> > > revert the IDL file changes from this CL.)
> > 
> > This code uses 'currentDOMWindow(isolate)->document()' and regular
> constructors
> > use 'toDocument(currentExecutionContext(isolate))'. Are those two different
in
> > any way?
> 
> The issue I want to address is just that we have both
> [ConstructorCallWith=ExecutionContext] and [ConstructorCallWith=Document], and
> both [CallWith=ExecutionContext] and [CallWith=Document]. Given that
> ExecutionContext is the same thing as Document, I want to remove either of
them.
> (I haven't looked at the generate code in details; so I'm not sure which
should
> be removed.)

We don't actually have [CallWith=Document]. Only [ConstructorCallWith] has
Document as a possible value.

And yeah, I'm with you on that; I just wanted to confirm that those two ways to
go from v8::Isolate to Document are equivalent.

[haraken, 2014-06-17 13:08:13]

On 2014/06/17 13:04:31, Jens Lindström wrote:
> On 2014/06/17 12:57:40, haraken wrote:
> >
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> > File Source/bindings/templates/methods.cpp (right):
> > 
> >
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> > Source/bindings/templates/methods.cpp:538: Document* documentPtr =
> > currentDOMWindow(isolate)->document();
> > On 2014/06/17 12:53:16, Jens Lindström wrote:
> > > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > > Since this code is here, unconditionally, CallWith=Document is still
> > partially
> > > > implied for named constructors. It would be nicer if it was all or
> nothing;
> > > > either making this code conditional, or really making it fully implied
> (and
> > > > revert the IDL file changes from this CL.)
> > > 
> > > This code uses 'currentDOMWindow(isolate)->document()' and regular
> > constructors
> > > use 'toDocument(currentExecutionContext(isolate))'. Are those two
different
> in
> > > any way?
> > 
> > The issue I want to address is just that we have both
> > [ConstructorCallWith=ExecutionContext] and [ConstructorCallWith=Document],
and
> > both [CallWith=ExecutionContext] and [CallWith=Document]. Given that
> > ExecutionContext is the same thing as Document, I want to remove either of
> them.
> > (I haven't looked at the generate code in details; so I'm not sure which
> should
> > be removed.)
> 
> We don't actually have [CallWith=Document]. Only [ConstructorCallWith] has
> Document as a possible value.
> 
> And yeah, I'm with you on that; I just wanted to confirm that those two ways
to
> go from v8::Isolate to Document are equivalent.

sorry; I missed your point. Yes, that's equivalent.

[Jens Widell, 2014-06-17 13:31:02]

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
On 2014/06/17 12:38:53, haraken wrote:
> On 2014/06/17 12:29:41, Jens Lindström wrote:
> > On 2014/06/17 11:28:05, haraken wrote:
> > > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > > Introduced to make [CallWith] handling more uniform. It's a reference in
> > > methods
> > > > already.
> > > 
> > > Given that we're planning to remove CallWith=Document, shall we keep this
> code
> > > as is?
> > 
> > Yeah, maybe. Would you prefer I upload a patch that does that first?
> > 
> > You're talking about replacing all uses of Document with ExecutionContext
and
> > let the C++ implementation fetch the document instead, here? As is talked
> about
> > in a FIXME in IDLExtendedAttributes.txt?
> 
> Yes. A follow-up CL is fine :)

Hmm. Replacing ConstructorCallWith=Document with ExecutionContext is not really
as nice as I would have hoped. There's a bunch of them where the ::create()
method is also used by other C++ code, and in that case passing in a Document is
the natural thing, I think.

If we're adding a bunch of create(ExecutionContext*, ...) functions that just
extract the document and call a create(Document&, ...) function, I wonder if it
doesn't make more sense to have that as functionality in the script/generated
code instead.

And there are constructors that are called with an ExecutionContext* today where
a Document& makes no sense, e.g. FileReader() which is usable in worker
contexts, so dropping ConstructorCallWith=ExecutionContext instead doesn't seem
viable.

[Jens Widell, 2014-06-17 13:31:36]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-17 13:32:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/340443004/20001

[haraken, 2014-06-17 14:15:04]

On 2014/06/17 13:31:02, Jens Lindström wrote:
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/340443004/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:540: Document& document = *documentPtr;
> On 2014/06/17 12:38:53, haraken wrote:
> > On 2014/06/17 12:29:41, Jens Lindström wrote:
> > > On 2014/06/17 11:28:05, haraken wrote:
> > > > On 2014/06/17 10:40:20, Jens Lindström wrote:
> > > > > Introduced to make [CallWith] handling more uniform. It's a reference
in
> > > > methods
> > > > > already.
> > > > 
> > > > Given that we're planning to remove CallWith=Document, shall we keep
this
> > code
> > > > as is?
> > > 
> > > Yeah, maybe. Would you prefer I upload a patch that does that first?
> > > 
> > > You're talking about replacing all uses of Document with ExecutionContext
> and
> > > let the C++ implementation fetch the document instead, here? As is talked
> > about
> > > in a FIXME in IDLExtendedAttributes.txt?
> > 
> > Yes. A follow-up CL is fine :)
> 
> Hmm. Replacing ConstructorCallWith=Document with ExecutionContext is not
really
> as nice as I would have hoped. There's a bunch of them where the ::create()
> method is also used by other C++ code, and in that case passing in a Document
is
> the natural thing, I think.
> 
> If we're adding a bunch of create(ExecutionContext*, ...) functions that just
> extract the document and call a create(Document&, ...) function, I wonder if
it
> doesn't make more sense to have that as functionality in the script/generated
> code instead.
> 
> And there are constructors that are called with an ExecutionContext* today
where
> a Document& makes no sense, e.g. FileReader() which is usable in worker
> contexts, so dropping ConstructorCallWith=ExecutionContext instead doesn't
seem
> viable.

Looked at generated code. I agree that we cannot drop
[ConstructorCallWith=ExecutionContext]. I'm OK with dropping
[ConstructorCallWith=Document], even if it ends up increasing binding code. It's
a bit confusing to have two ways to do the same thing. I think the benefit of
unifying the ways and simplifying the IDL attribute would be larger.

[commit-bot: I haz the power, 2014-06-17 15:46:24]

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

[commit-bot: I haz the power, 2014-06-17 15:46:25]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Nils Barth (inactive), 2014-06-18 02:44:53]

To briefly concur with Jens:
as much as possible, we'd like to share a single code path for
constructors and methods
(both in Python and Jinja),
especially for argument handling.
There's been lots of duplicated code, which has made hacking
on these trickier (need to duplicate changes at least).

There are differences between constructors and methods,
but if we share as much code as possible, it's clearer
what exactly those differences are, and it's much easier
to hack on.

[Jens Widell, 2014-06-18 05:47:27]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-18 05:47:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/340443004/20001

[commit-bot: I haz the power, 2014-06-18 05:48:31]

Change committed as 176390