Simplify EventTarget bindings generation (2)

Simplify EventTarget bindings generation (2)

This further simplifies EventTarget bindings generation.

The goal (for now) is to only have a special case for the
{{hidden_dependency_action}}; everything else should be pretty easy
to eliminate (except maybe the ListenerFindOnly case).

The main change in this CL is making the argument handling regular.
(Also correctly handles EventListener as a callback interface.)

This also breaks up the big custom generation into smaller pieces,
so each piece can be eliminated separately.

Followup to:
Simplify EventTarget bindings generation
https://codereview.chromium.org/201603002/

R=haraken
BUG=345503
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169601

[Nils Barth (inactive), 2014-03-18 07:20:35]

More simplification of EventTarget.

This one is a bit big because I've broken up the custom
generation into small pieces, and then can kill the small
pieces in follow-ups.

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

https://codereview.chromium.org/203143002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:40: {% if interface_name == 'EventTarget'
and
Moved down, so now arguments are always generated normally.

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

https://codereview.chromium.org/203143002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:80: {% macro
hidden_dependency_action(method_name) %}
Much simpler, because now only the really special behavior.

https://codereview.chromium.org/203143002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:167: {# FIXME: remove EventListener
special case
This is really ugly, and it's all because we want to pass in
ListenerFindOnly for removeEventListener;
otherwise it would be easy to handle in v8_types.
I'd like to eliminate this ASAP.

[Nils Barth (inactive), 2014-03-18 07:23:25]

For reference, here's the diff.

Differences:
* Set local variables regularly.
* Early call if called without optional boolean, as usual.
* Replace if (listener) { ... } nesting with if (!listener) return,
  which is flatter and simplifies template a lot.

V8EventTarget.cpp:
@@ -93,13 +93,22 @@
         if (!window->document())
             return;
     }
-    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());
+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>, type,
info[0]);
+    V8TRYCATCH_VOID(RefPtr<EventListener>, listener,
V8EventListenerList::getEventListener(info[1], false, ListenerFindOrCreate));
+    if (UNLIKELY(info.Length() <= 2)) {
+        if (!listener)
+            return;
+        impl->addEventListener(type, listener);
         if (!impl->toNode())
             addHiddenValueToArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
+        return;
     }
+    V8TRYCATCH_VOID(bool, useCapture, info[2]->BooleanValue());
+    if (!listener)
+        return;
+    impl->addEventListener(type, listener, useCapture);
+    if (!impl->toNode())
+        addHiddenValueToArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
 }
 
 static void addEventListenerMethodCallback(const
v8::FunctionCallbackInfo<v8::Value>& info)
@@ -126,13 +135,22 @@
         if (!window->document())
             return;
     }
-    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());
+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>, type,
info[0]);
+    V8TRYCATCH_VOID(RefPtr<EventListener>, listener,
V8EventListenerList::getEventListener(info[1], false, ListenerFindOnly));
+    if (UNLIKELY(info.Length() <= 2)) {
+        if (!listener)
+            return;
+        impl->removeEventListener(type, listener.get());
         if (!impl->toNode())
             removeHiddenValueFromArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
+        return;
     }
+    V8TRYCATCH_VOID(bool, useCapture, info[2]->BooleanValue());
+    if (!listener)
+        return;
+    impl->removeEventListener(type, listener.get(), useCapture);
+    if (!impl->toNode())
+        removeHiddenValueFromArray(info.Holder(), info[1],
V8EventTarget::eventListenerCacheIndex, info.GetIsolate());
 }
 
 static void removeEventListenerMethodCallback(const
v8::FunctionCallbackInfo<v8::Value>& info)

[haraken, 2014-03-18 11:10:57]

The change looks OK but I'm not really happy about this CL because it introduces
a lot of FIXMEs. LGTM if you're planning to fix the FIXMEs in follow-up CLs.

https://codereview.chromium.org/203143002/diff/1/Source/core/events/EventTarg...
File Source/core/events/EventTarget.idl (right):

https://codereview.chromium.org/203143002/diff/1/Source/core/events/EventTarg...
Source/core/events/EventTarget.idl:24: void
addEventListener([TreatNullAs=NullString] DOMString type,

How is this change related to this CL?

[Nils Barth (inactive), 2014-03-19 02:02:46]

On 2014/03/18 11:10:57, haraken wrote:
> The change looks OK but I'm not really happy about this CL because it
introduces
> a lot of FIXMEs. LGTM if you're planning to fix the FIXMEs in follow-up CLs.

Agreed; need to break this up into several CLs to do incrementally,
but don't want this custom code to hang around for long;
will fix next!

The V8 -> C++ custom code in this CL is *really* ugly, so I've already fixed it
in a followup:
Replace custom EventTarget toNative call with [ToNativeArg] extended attribute
https://codereview.chromium.org/204213002/

Once these both land, there are a few minor FIXMEs (get() call and a NULL
check),
which I'll fix, but it's not too ugly.

[Nils Barth (inactive), 2014-03-19 02:02:56]

(One clarification.)

https://codereview.chromium.org/203143002/diff/1/Source/core/events/EventTarg...
File Source/core/events/EventTarget.idl (right):

https://codereview.chromium.org/203143002/diff/1/Source/core/events/EventTarg...
Source/core/events/EventTarget.idl:24: void
addEventListener([TreatNullAs=NullString] DOMString type,
On 2014/03/18 11:10:57, haraken wrote:
> How is this change related to this CL?

This was previously hard-coded, as:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>, eventName,
info[0]);

The |WithNullCheck| in:
V8StringResource<WithNullCheck>
...is generated automatically by [TreatNullAs=NullString]
...so adding the (correct) extended attribute removes the custom
code from the CG.

[Nils Barth (inactive), 2014-03-19 03:28:34]

On 2014/03/19 02:02:46, Nils Barth wrote:
> The V8 -> C++ custom code in this CL is *really* ugly, so I've already fixed
it
> in a followup:
> Replace custom EventTarget toNative call with [ToNativeArg] extended attribute
> https://codereview.chromium.org/204213002/

Another follow-up:
Explicitly mark first 2 args of addEventListener/removeEventListener as optional
https://codereview.chromium.org/203603005/

[Nils Barth (inactive), 2014-03-19 04:31:29]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 04:31:31]

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

[commit-bot: I haz the power, 2014-03-19 05:44:58]

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

[commit-bot: I haz the power, 2014-03-19 05:44:58]

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

[Nils Barth (inactive), 2014-03-19 06:28:50]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 06:28:58]

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

[Nils Barth (inactive), 2014-03-19 06:34:53]

The CQ bit was unchecked by nbarth@chromium.org

[Nils Barth (inactive), 2014-03-19 06:59:40]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 06:59:41]

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

[Nils Barth (inactive), 2014-03-19 07:00:24]

The CQ bit was unchecked by nbarth@chromium.org

[Nils Barth (inactive), 2014-03-19 08:59:12]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:59:20]

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

[commit-bot: I haz the power, 2014-03-19 09:00:40]

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

[commit-bot: I haz the power, 2014-03-19 09:00:41]

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

[Nils Barth (inactive), 2014-03-19 09:02:07]

Updated to correctly treat EventListener as callback interface, per haraken's
observation:
https://codereview.chromium.org/204213002/diff/1/Source/bindings/tests/result...

It's now not as bad, but I'll need to sync followup CLs to this.

[Nils Barth (inactive), 2014-03-19 09:02:20]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 09:02:32]

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

[commit-bot: I haz the power, 2014-03-19 10:05:12]

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

[commit-bot: I haz the power, 2014-03-19 10:05:13]

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

[Nils Barth (inactive), 2014-03-19 10:06:13]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-19 10:06:21]

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

[commit-bot: I haz the power, 2014-03-19 11:17:25]

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

[commit-bot: I haz the power, 2014-03-19 11:17:25]

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

[Nils Barth (inactive), 2014-03-20 00:37:33]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:37:43]

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

[commit-bot: I haz the power, 2014-03-20 03:46:39]

Change committed as 169601