Simplify EventTarget bindings generation

Simplify EventTarget bindings generation

EventTarget has special-cased bindings generation, with several bugs.

This CL corrects and simplifies it in a few ways:
1. Remove duplicate |EventTarget* imp| initialization
Currently EventListener has |EventTarget* imp| initialized twice.
(With different names, |impl| and |imp|, so this compiles.)

Also rearranges the template code so that the special case
is more narrowly focused (instead of overall),
and check the interface name, not just method names,
which clarifies the special case.
(This also means we can remove the test cases,
as it's just a special-case for one interface.)

This does *not* change web behavior:
Unlike all other methods, addEventListener and removeEventListener do not check arguments length.
This is apparently required for legacy calls,
so I've opened a separate bug for this:
https://code.google.com/p/chromium/issues/detail?id=353484

Previously EventTarget skipped this, due to having an overall special case,
instead of just special-casing the method call itself;
we now have an explicit special-case in the template.

This was hit by 3 tests:
fast/dom/node-legacy-event-listener.html
fast/dom/XMLHttpRequest-legacy-event-listener.html
fast/dom/Window/window-legacy-event-listener.html
(More details at the bug.)

I've added more detailed tests on behavior, and other argument handling.

Follow-up CL:
* further reduce special case (use usual argument handling)

TBR=haraken
BUG=345503
BUG=353484
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169506

[Nils Barth (inactive), 2014-03-18 03:34:46]

Simplification and correction for our least favorite special case (EventTarget).

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

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:9: {% if method.name in
['addEventListener', 'removeEventListener',
This if statement lists all the methods in EventTarget;
more correct and clearer to check |interface_name|.

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:13: {% if method.name in
['addEventListener', 'removeEventListener'] %}
This {% if %} block meant that addEventListener/removeEventListener
skipped all other usual clauses,
notably the argument length check.
Moving this down to just before the method call makes the scope
much narrower and simpler.

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

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:59: {% macro
event_target_check_security_for_frame() %}
Existing name was completely unclear
(just listed applicable methods);
this clarifies what's happening.

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:83: imp->{{method_name}}(eventName,
{{listener}}, info[2]->BooleanValue());
|impl| => |imp| for consistency.

[Nils Barth (inactive), 2014-03-18 03:36:14]

For reference, here's the exact change in output.
The content is simply:
* Always check arguments (|info|) length.
* ...and do this at the top.
* Rename |impl| => |imp| (consistency).

V8EventTarget.cpp:
@@ -79,8 +79,13 @@
 static void addEventListenerMethod(const v8::FunctionCallbackInfo<v8::Value>&
info)
 {
     ExceptionState exceptionState(ExceptionState::ExecutionContext,
"addEventListener", "EventTarget", info.Holder(), info.GetIsolate());
-    EventTarget* impl = V8EventTarget::toNative(info.Holder());
-    if (DOMWindow* window = impl->toDOMWindow()) {
+    if (UNLIKELY(info.Length() < 2)) {
+        exceptionState.throwTypeError(ExceptionMessages::notEnoughArguments(2,
info.Length()));
+        exceptionState.throwIfNeeded();
+        return;
+    }
+    EventTarget* imp = V8EventTarget::toNative(info.Holder());
+    if (DOMWindow* window = imp->toDOMWindow()) {
         if (!BindingSecurity::shouldAllowAccessToFrame(info.GetIsolate(),
window->frame(), exceptionState)) {
             exceptionState.throwIfNeeded();
             return;
@@ -91,8 +96,8 @@
     RefPtr<EventListener> listener =
V8EventListenerList::getEventListener(info[1], false, ListenerFindOrCreate);
     if (listener) {
         V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>,
eventName, info[0]);
-        impl->addEventListener(eventName, listener, info[2]->BooleanValue());
-        if (!impl->toNode())
+        imp->addEventListener(eventName, listener, info[2]->BooleanValue());
+        if (!imp->toNode())
             addHiddenValueToArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
     }
 }
@@ -107,8 +112,13 @@
 static void removeEventListenerMethod(const
v8::FunctionCallbackInfo<v8::Value>& info)
 {
     ExceptionState exceptionState(ExceptionState::ExecutionContext,
"removeEventListener", "EventTarget", info.Holder(), info.GetIsolate());
-    EventTarget* impl = V8EventTarget::toNative(info.Holder());
-    if (DOMWindow* window = impl->toDOMWindow()) {
+    if (UNLIKELY(info.Length() < 2)) {
+        exceptionState.throwTypeError(ExceptionMessages::notEnoughArguments(2,
info.Length()));
+        exceptionState.throwIfNeeded();
+        return;
+    }
+    EventTarget* imp = V8EventTarget::toNative(info.Holder());
+    if (DOMWindow* window = imp->toDOMWindow()) {
         if (!BindingSecurity::shouldAllowAccessToFrame(info.GetIsolate(),
window->frame(), exceptionState)) {
             exceptionState.throwIfNeeded();
             return;
@@ -119,8 +129,8 @@
     RefPtr<EventListener> listener =
V8EventListenerList::getEventListener(info[1], false, ListenerFindOnly);
     if (listener) {
         V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>,
eventName, info[0]);
-        impl->removeEventListener(eventName, listener.get(),
info[2]->BooleanValue());
-        if (!impl->toNode())
+        imp->removeEventListener(eventName, listener.get(),
info[2]->BooleanValue());
+        if (!imp->toNode())
             removeHiddenValueFromArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
     }
 }
@@ -135,8 +145,13 @@
 static void dispatchEventMethod(const v8::FunctionCallbackInfo<v8::Value>&
info)
 {
     ExceptionState exceptionState(ExceptionState::ExecutionContext,
"dispatchEvent", "EventTarget", info.Holder(), info.GetIsolate());
-    EventTarget* impl = V8EventTarget::toNative(info.Holder());
-    if (DOMWindow* window = impl->toDOMWindow()) {
+    if (UNLIKELY(info.Length() < 1)) {
+        exceptionState.throwTypeError(ExceptionMessages::notEnoughArguments(1,
info.Length()));
+        exceptionState.throwIfNeeded();
+        return;
+    }
+    EventTarget* imp = V8EventTarget::toNative(info.Holder());
+    if (DOMWindow* window = imp->toDOMWindow()) {
         if (!BindingSecurity::shouldAllowAccessToFrame(info.GetIsolate(),
window->frame(), exceptionState)) {
             exceptionState.throwIfNeeded();
             return;
@@ -144,12 +159,6 @@
         if (!window->document())
             return;
     }
-    if (UNLIKELY(info.Length() < 1)) {
-        exceptionState.throwTypeError(ExceptionMessages::notEnoughArguments(1,
info.Length()));
-        exceptionState.throwIfNeeded();
-        return;
-    }
-    EventTarget* imp = V8EventTarget::toNative(info.Holder());
     V8TRYCATCH_VOID(Event*, event,
V8Event::toNativeWithTypeCheck(info.GetIsolate(), info[0]));
     bool result = imp->dispatchEvent(event, exceptionState);
     if (exceptionState.throwIfNeeded())

[haraken, 2014-03-18 03:43:57]

LGTM

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

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:60: {# FIXME: use the existing
shouldAllowAccessToFrame check if possible. #}

We should consider fixing this FIXME in a follow-up.

[Nils Barth (inactive), 2014-03-18 04:47:03]

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

https://codereview.chromium.org/201603002/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:60: {# FIXME: use the existing
shouldAllowAccessToFrame check if possible. #}
On 2014/03/18 03:43:57, haraken wrote:
> We should consider fixing this FIXME in a follow-up.

Yup, I'll look at it!

[Nils Barth (inactive), 2014-03-18 04:47:08]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-18 04:47:15]

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

[commit-bot: I haz the power, 2014-03-18 04:48:35]

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

[commit-bot: I haz the power, 2014-03-18 04:48:36]

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

[Nils Barth (inactive), 2014-03-18 05:04:58]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-18 05:05:05]

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

[commit-bot: I haz the power, 2014-03-18 05:16:34]

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

[commit-bot: I haz the power, 2014-03-18 05:16:35]

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

[Nils Barth (inactive), 2014-03-18 06:07:43]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-18 06:07:45]

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

[commit-bot: I haz the power, 2014-03-18 06:10:42]

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

[commit-bot: I haz the power, 2014-03-18 06:10:43]

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

[Nils Barth (inactive), 2014-03-18 06:23:32]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-18 06:23:38]

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

[commit-bot: I haz the power, 2014-03-18 06:39:47]

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

[commit-bot: I haz the power, 2014-03-18 06:39:47]

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

[Nils Barth (inactive), 2014-03-18 09:12:52]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-18 09:12:58]

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

[commit-bot: I haz the power, 2014-03-18 10:02:16]

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

[commit-bot: I haz the power, 2014-03-18 10:02:17]

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

[Nils Barth (inactive), 2014-03-19 01:09:00]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:09:07]

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

[commit-bot: I haz the power, 2014-03-19 02:44:30]

Change committed as 169506