IDL: Generate all constructor callbacks with the same template macro

IDL: Generate all constructor callbacks with the same template macro

Regular constructors and named constructors are similar enough that using
separate template macros to generate them mostly leads to duplicated code.

The actual difference is now more visible, since it takes the explicit
form

  {% if constructor.is_named_constructor %}
  // the difference
  {% endif %}

in the shared macro.

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

[Jens Widell, 2014-06-25 07:36:53]

PTAL

[Jens Widell, 2014-06-25 07:50:42]

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

https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:506: {% if
constructor.number_of_required_arguments and not constructor.overload_index %}
Using 'constructor.number_of_required_arguments' instead of 'interface_length'
made a difference for named constructors with non-zero required arguments, since
'interface_length' only takes regular constructors into account.

In practice, there are named constructors with non-zero required arguments
outside of the bindings tests.

[haraken, 2014-06-25 09:43:11]

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

https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:501: {% endif %}

This block is fine in this CL, but we might want to merge the block with
constructorCallback in interface.cpp. (We should add the TRACE_EVENT macro and
UserCounter to the named constructor. If we can share the block with
constructorCallback, the issue will be resolved.)

https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:506: {% if
constructor.number_of_required_arguments and not constructor.overload_index %}
On 2014/06/25 07:50:42, Jens Lindström wrote:
> Using 'constructor.number_of_required_arguments' instead of 'interface_length'
> made a difference for named constructors with non-zero required arguments,
since
> 'interface_length' only takes regular constructors into account.
> 
> In practice, there are named constructors with non-zero required arguments
> outside of the bindings tests.

What is the spec saying?

interface.cpp is still using interface_length, but is it OK?

[Jens Widell, 2014-06-25 10:55:37]

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

https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:501: {% endif %}
On 2014/06/25 09:43:11, haraken wrote:
> 
> This block is fine in this CL, but we might want to merge the block with
> constructorCallback in interface.cpp. (We should add the TRACE_EVENT macro and
> UserCounter to the named constructor. If we can share the block with
> constructorCallback, the issue will be resolved.)

Ah, I was wondering why these things applied only to named constructors.

I'll sort it out in a follow-up.

https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
Source/bindings/templates/methods.cpp:506: {% if
constructor.number_of_required_arguments and not constructor.overload_index %}
On 2014/06/25 09:43:11, haraken wrote:
> On 2014/06/25 07:50:42, Jens Lindström wrote:
> > Using 'constructor.number_of_required_arguments' instead of
'interface_length'
> > made a difference for named constructors with non-zero required arguments,
> since
> > 'interface_length' only takes regular constructors into account.
> > 
> > In practice, there are named constructors with non-zero required arguments
> > outside of the bindings tests.
> 
> What is the spec saying?
> 
> interface.cpp is still using interface_length, but is it OK?

The key here is that regular constructors (or custom constructors, or event
constructors) and 'interface_length' all apply to the interface object.

Named constructors is a separate case, and their number of required arguments
affects the length property of a different object. (I don't think that object's
length property is ever actually set, so it's probably effectively hard-coded
zero. Good thing no real named constructors have any required arguments.)

And also, 'interface_length' is specifically "Function.length for the interface
object". That's the number of required arguments of the shortest overload, and
was thus always technically incorrect to use for arity checks for overloaded
regular constructors. Each constructor's 'number_of_required_arguments' is the
right value for the arity check. But the arity check is of course disabled here
for overloads, since it's performed implicitly by the overload resolution, so
this incorrectness was inconsequential.

[haraken, 2014-06-25 10:58:47]

On 2014/06/25 10:55:37, Jens Lindström wrote:
>
https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
> Source/bindings/templates/methods.cpp:501: {% endif %}
> On 2014/06/25 09:43:11, haraken wrote:
> > 
> > This block is fine in this CL, but we might want to merge the block with
> > constructorCallback in interface.cpp. (We should add the TRACE_EVENT macro
and
> > UserCounter to the named constructor. If we can share the block with
> > constructorCallback, the issue will be resolved.)
> 
> Ah, I was wondering why these things applied only to named constructors.
> 
> I'll sort it out in a follow-up.
> 
>
https://codereview.chromium.org/349523004/diff/20001/Source/bindings/template...
> Source/bindings/templates/methods.cpp:506: {% if
> constructor.number_of_required_arguments and not constructor.overload_index %}
> On 2014/06/25 09:43:11, haraken wrote:
> > On 2014/06/25 07:50:42, Jens Lindström wrote:
> > > Using 'constructor.number_of_required_arguments' instead of
> 'interface_length'
> > > made a difference for named constructors with non-zero required arguments,
> > since
> > > 'interface_length' only takes regular constructors into account.
> > > 
> > > In practice, there are named constructors with non-zero required arguments
> > > outside of the bindings tests.
> > 
> > What is the spec saying?
> > 
> > interface.cpp is still using interface_length, but is it OK?
> 
> The key here is that regular constructors (or custom constructors, or event
> constructors) and 'interface_length' all apply to the interface object.
> 
> Named constructors is a separate case, and their number of required arguments
> affects the length property of a different object. (I don't think that
object's
> length property is ever actually set, so it's probably effectively hard-coded
> zero. Good thing no real named constructors have any required arguments.)
> 
> And also, 'interface_length' is specifically "Function.length for the
interface
> object". That's the number of required arguments of the shortest overload, and
> was thus always technically incorrect to use for arity checks for overloaded
> regular constructors. Each constructor's 'number_of_required_arguments' is the
> right value for the arity check. But the arity check is of course disabled
here
> for overloads, since it's performed implicitly by the overload resolution, so
> this incorrectness was inconsequential.

Thanks for the clarification. LGTM.

[Jens Widell, 2014-06-25 11:02:05]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-25 11:02:37]

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

[commit-bot: I haz the power, 2014-06-25 14:04:20]

Change committed as 176920