IDL compiler: EventHandler for setters

IDL compiler: EventHandler for setters

Ugly special case, handled as generically as possible.
Also some cleanup of previous EventHandler includes special casing.

BUG=239771
NOTRY=true

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

[Nils Barth (inactive), 2013-10-19 11:38:51]

Ugly hack, handled pretty briefly and generically.

[haraken, 2013-10-19 12:41:28]

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:124:
includes.update(v8_types.includes_for_type('EventHandler'))

I guess we should always call
includes.update(v8_types.includes_for_type(idl_type))) in this method. Since the
getter is using the IDL type, there would be no reason to not call
includes.update().

Then you can remove includes.update() here and includes.update() in the 'if
this_is_keep_alive_for_gc' branch.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:213:
includes.add('bindings/v8/V8EventListenerList.h')

Ditto. I think we can just always call
includes.update(v8_types.includes_for_type(idl_type))) in this method. Then you
can remove the includes.add('bindings/v8/V8EventListenerList.h').

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:216:
contents['event_handler_getter_expression'] = 'imp->%s(%s)' %
(cpp_name(attribute), isolated_world)

Not related to your CL, it looks ugly to get an isolated world in generated code
and pass the isolated world to outside. For consistency with other places, we
should instead pass an isolate to outside and have the outside get an isolated
world from the isolate.

Would you fix this in a follow-up CL?

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_types.py:465: if idl_type in
['EventHandler', 'SerializedScriptValue']:

I wonder why this isn't 'if idl_type in INCLUDES_FOR_TYPE'.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:111: if (!imp->document())

I think this check should be moved to core/ if the number of affected attribute
setters are not many. (It wouldn't make sense to do the check only for
EventHandler and Window in the binding layer.) How many attribute setters are
using this check?

[Nils Barth (inactive), 2013-10-21 02:41:38]

Revised, will do 2 followup CLs:
1. Always call includes_for_type for attributes (and move
‘V8EventListenerList.h’ to includes_for_type)
2. Fix isolatedWorldForIsolate(info.GetIsolate()) to instead pass an isolate.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_attributes.py (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:124:
includes.update(v8_types.includes_for_type('EventHandler'))
On 2013/10/19 12:41:29, haraken wrote:
> I guess we should always call
> includes.update(v8_types.includes_for_type(idl_type))) in this method. Since
the
> getter is using the IDL type, there would be no reason to not call
> includes.update().
> 
> Then you can remove includes.update() here and includes.update() in the 'if
> this_is_keep_alive_for_gc' branch.

This would simplify the code.
This involves Perl changes (for consistency), so I'll post in a followup CL.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:213:
includes.add('bindings/v8/V8EventListenerList.h')
On 2013/10/19 12:41:29, haraken wrote:
> 
> Ditto. I think we can just always call
> includes.update(v8_types.includes_for_type(idl_type))) in this method. Then
you
> can remove the includes.add('bindings/v8/V8EventListenerList.h').

Will do (in same "includes for type" followup CL)!

V8EventListenerList.h is only used for setters and methods
(not getters), which is why it's currently here (and in method generator),
but consolidating in includes_for_type simplifies code.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_attributes.py:216:
contents['event_handler_getter_expression'] = 'imp->%s(%s)' %
(cpp_name(attribute), isolated_world)
On 2013/10/19 12:41:29, haraken wrote:
> 
> Not related to your CL, it looks ugly to get an isolated world in generated
code
> and pass the isolated world to outside. For consistency with other places, we
> should instead pass an isolate to outside and have the outside get an isolated
> world from the isolate.
> 
> Would you fix this in a follow-up CL?

Will do.

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_types.py:465: if idl_type in
['EventHandler', 'SerializedScriptValue']:
On 2013/10/19 12:41:29, haraken wrote:
> I wonder why this isn't 'if idl_type in INCLUDES_FOR_TYPE'.

That works; changed!
(With "always use includes_for_type" CL this won't change generated code.)

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:111: if (!imp->document())
On 2013/10/19 12:41:29, haraken wrote:
> 
> I think this check should be moved to core/ if the number of affected
attribute
> setters are not many. (It wouldn't make sense to do the check only for
> EventHandler and Window in the binding layer.)

> How many attribute setters are using this check?

28 attribute setters.

[haraken, 2013-10-21 03:43:52]

LGTM. I'm looking forward to seeing follow-up CLs :)

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:111: if (!imp->document())
On 2013/10/21 02:41:38, Nils Barth wrote:
> On 2013/10/19 12:41:29, haraken wrote:
> > 
> > I think this check should be moved to core/ if the number of affected
> attribute
> > setters are not many. (It wouldn't make sense to do the check only for
> > EventHandler and Window in the binding layer.)
> 
> > How many attribute setters are using this check?
> 
> 28 attribute setters.

OK, then it would make sense to keep the check in the auto-generated code.
Thanks for the investigation.

https://codereview.chromium.org/30493002/diff/200001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestObjectPython.cpp (right):

https://codereview.chromium.org/30493002/diff/200001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestObjectPython.cpp:1355: jsValue =
v8::Null(info.GetIsolate());

Would you move this check to inside transferHiddenDependency and
getEventListener (in a follow-up CL)? We don't much want to add an ad-hoc logic
to auto-generated code.

[Nils Barth (inactive), 2013-10-21 03:58:34]

This finishes special types for setters - thanks!
(Will do followup CLs next.)

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/30493002/diff/100001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:111: if (!imp->document())
On 2013/10/21 03:43:52, haraken wrote:
> On 2013/10/21 02:41:38, Nils Barth wrote:
> > 28 attribute setters.
> 
> OK, then it would make sense to keep the check in the auto-generated code.
> Thanks for the investigation.

Got it, n/p!

https://codereview.chromium.org/30493002/diff/200001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestObjectPython.cpp (right):

https://codereview.chromium.org/30493002/diff/200001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestObjectPython.cpp:1355: jsValue =
v8::Null(info.GetIsolate());
On 2013/10/21 03:43:53, haraken wrote:
> Would you move this check to inside transferHiddenDependency and
> getEventListener (in a follow-up CL)? We don't much want to add an ad-hoc
logic
> to auto-generated code.

Will do!
(That's 3 followups for this CL.)

[commit-bot: I haz the power, 2013-10-21 03:58:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/30493002/240001

[commit-bot: I haz the power, 2013-10-21 03:58:59]

Change committed as 160051