Unify the GlobalEventHandlers list on the C++ side

Unify the GlobalEventHandlers list on the C++ side

Several changes need to be made in sync to achieve this.

Teach the IDL compiler to generate static calls for EventTarget,
allowing GlobalEventHandlers.h to be the single list of the event
attributes on the C++ side.

The handling of the 5 special event handler attributes on body and
frameset was very strange, with both virtual getters/setters in C++
and overrides at the IDL level. Remove the former mechanism, and fix
up the later to fill the job it was clearly added to do.

Also fix some style issues in EventTarget.h to please presubmit.tributes on the C++ side.

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

[philipj_slow, 2013-10-23 13:41:48]

I'm not exactly a Perl pro, so before I spend any time making this pretty and
adding new tests, I'd like to check that this is the direction you had in mind.

[Nils Barth (inactive), 2013-10-24 02:41:13]

On 2013/10/23 13:41:48, philipj wrote:
> I'm not exactly a Perl pro, so before I spend any time making this pretty and
> adding new tests, I'd like to check that this is the direction you had in
mind.

I think this is what we want to do (Adam, haraken?): static calls, not imp->...

I understand that it's not polished;
I've a few notes on simplifying the control flow & avoiding some duplication
(I've just been rewriting this, so I've just been through this),
but otherwise the Perl seems fine.

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

Some comments on simplifying logic;
understand it's not polished yet, just wanted to highlight.

https://codereview.chromium.org/37263002/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/37263002/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:1979: } else {
You might be able to avoid having multiple branches
by having a $setterFunctionName variable?

https://codereview.chromium.org/37263002/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:1980: if
(!InheritsInterface($interface, "Node")) {
I think you can make this section simpler by having a list of setter
arguments, so you don't have to write out:
V8EventListenerList::findOrCreateWrapper<V8ErrorHandler>(jsValue, true,
info.GetIsolate()),
isolatedWorldForIsolate(info.GetIsolate())
...several times.
Alternatively, you could at least have $wrapper and $isolatedWorld.

[philipj_slow, 2013-10-24 20:28:51]

Thanks Nils, I've cleaned it up a bit, added tests and verified that all of the
real generated bindings (not the tests) were identical before and after this
refactoring.

I'll soon upload the C++ changes that are needed to make it actually compile and
work.

[philipj_slow, 2013-10-26 05:02:04]

I consider this done now, PTAL.

[abarth-chromium, 2013-10-26 15:49:48]

The C++ parts look good.  We should have Nils or Haraken review the changes to
the code generator.

[haraken, 2013-10-26 17:03:14]

LGTM. Great change, thanks!

https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
Source/core/events/EventTarget.h:163: void
setOn##attribute(PassRefPtr<EventListener> listener, DOMWrapperWorld*
isolatedWorld = 0) { setAttributeEventListener(EventTypeNames::attribute,
listener, isolatedWorld); } \

Not related to your CL, it wouldn't be a good thing to set |isolatedWorld| to 0
by default. We should always explicitly pass some |isolatedWorld|.

https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
Source/core/events/EventTarget.h:167: static void setOn##attribute(EventTarget*
eventTarget, PassRefPtr<EventListener> listener, DOMWrapperWorld* isolatedWorld
= 0) { eventTarget->setAttributeEventListener(EventTypeNames::attribute,
listener, isolatedWorld); } \

Ditto.

[haraken, 2013-10-26 17:04:14]

On 2013/10/26 17:03:14, haraken wrote:
> LGTM. Great change, thanks!
> 
>
https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
> File Source/core/events/EventTarget.h (right):
> 
>
https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
> Source/core/events/EventTarget.h:163: void
> setOn##attribute(PassRefPtr<EventListener> listener, DOMWrapperWorld*
> isolatedWorld = 0) { setAttributeEventListener(EventTypeNames::attribute,
> listener, isolatedWorld); } \
> 
> Not related to your CL, it wouldn't be a good thing to set |isolatedWorld| to
0
> by default. We should always explicitly pass some |isolatedWorld|.
> 
>
https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
> Source/core/events/EventTarget.h:167: static void
setOn##attribute(EventTarget*
> eventTarget, PassRefPtr<EventListener> listener, DOMWrapperWorld*
isolatedWorld
> = 0) { eventTarget->setAttributeEventListener(EventTypeNames::attribute,
> listener, isolatedWorld); } \
> 
> Ditto.

You don't need to fix it in this CL. A follow-up CL is fine.

[commit-bot: I haz the power, 2013-10-26 18:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001

[commit-bot: I haz the power, 2013-10-26 19:08:06]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-26 19:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001

[philipj_slow, 2013-10-26 20:27:49]

https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/37263002/diff/110001/Source/core/events/Event...
Source/core/events/EventTarget.h:163: void
setOn##attribute(PassRefPtr<EventListener> listener, DOMWrapperWorld*
isolatedWorld = 0) { setAttributeEventListener(EventTypeNames::attribute,
listener, isolatedWorld); } \
On 2013/10/26 17:03:15, haraken wrote:
> 
> Not related to your CL, it wouldn't be a good thing to set |isolatedWorld| to
0
> by default. We should always explicitly pass some |isolatedWorld|.

Created https://codereview.chromium.org/45973006/

[commit-bot: I haz the power, 2013-10-26 20:27:58]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-26 20:32:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001

[commit-bot: I haz the power, 2013-10-26 21:31:17]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-27 03:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/37263002/110001

[commit-bot: I haz the power, 2013-10-27 14:54:32]

Change committed as 160676