IDL compiler: addEventListener, removeEventListener methods

Source/bindings/templates/methods.cpp

Index: Source/bindings/templates/methods.cpp
diff --git a/Source/bindings/templates/methods.cpp b/Source/bindings/templates/methods.cpp
index c77ffcb5f7356182f9ecb5e61d9412ceb6908244..516520ba517e9f4fd16522e907fe9bfe7aa0dae9 100644
--- a/Source/bindings/templates/methods.cpp
+++ b/Source/bindings/templates/methods.cpp
@@ -3,6 +3,9 @@
 {% filter conditional(method.conditional_string) %}
 static void {{method.name}}{{method.overload_index}}Method{{world_suffix}}(const v8::FunctionCallbackInfo<v8::Value>& info)
 {
+    {% if method.name in ['addEventListener', 'removeEventListener'] %}
+    {{add_remove_event_listener_method(method.name) | indent}}
+    {% else %}
     {% if method.number_of_required_arguments %}
     if (UNLIKELY(info.Length() < {{method.number_of_required_arguments}})) {
         throwTypeError(ExceptionMessages::failedToExecute("{{method.name}}", "{{interface_name}}", ExceptionMessages::notEnoughArguments({{method.number_of_required_arguments}}, info.Length())), info.GetIsolate());
@@ -85,12 +88,40 @@ static void {{method.name}}{{method.overload_index}}Method{{world_suffix}}(const
     {% endif %}
     {% endfor %}{# arguments #}
     {{cpp_method_call(method, method.v8_set_return_value, method.cpp_value) | indent}}
+    {% endif %}{# addEventListener, removeEventListener #}
 }
 {% endfilter %}
 {% endmacro %}
 
 
 {######################################}
+{% macro add_remove_event_listener_method(method_name) %}
+{% set lookup_type, pass_ref_ptr_handling, hidden_dependency_action =

[haraken, 2013/11/13 08:33:53]

Actually I don't understand why we need these complexity.

[Nils Barth (inactive), 2013/11/13 11:33:29]

Clarified by:
* adding a comment, and
* making the template parameters correspond to the whole value
(function name etc.), rather than just the differing part.
Does this make it clearer?

The code for addEventListener and removeEventListener is almost the same,
differing only in 3 places.
Thus it's implemented as a single template.

The Perl code makes this very terse by making auxiliary variables
for just the portions that differ, so
ListenerFindOrCreate/ListenerFindOnly becomes:
my $lookupType = ($name eq "addEventListener") ? "OrCreate" : "Only";
ListenerFind$lookupType

Relevant Perl code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[haraken, 2013/11/13 11:37:46]

Can't we somehow get rid of the distinction between ('OrCreate', '', 'create')
and ('Only', '.get()', 'remove') ? Refactoring in the event listener code would
be needed.

[Nils Barth (inactive), 2013/11/14 03:01:53]

We probably need to generate *some* different code for addEventListener
vs. removeEventListener, but it should just be a function call in v8/,
not this long generated code.
We can definitely simplify this code by moving it to v8/ or other C++
changes, which would also reduce the number of includes.

[haraken, 2013/11/14 04:33:02]

Yeah, I haven't yet checked the code of event listeners, I think that we should
clean up the code generator with C++ side changes.

+    ('OrCreate', '', 'create')
+    if method_name.startswith('add') else
+    ('Only', '.get()', 'remove')
+%}
+EventTarget* impl = {{v8_class_name}}::toNative(info.Holder());
+if (DOMWindow* window = impl->toDOMWindow()) {
+    ExceptionState es(info.GetIsolate());
+    if (!BindingSecurity::shouldAllowAccessToFrame(window->frame(), es)) {
+        es.throwIfNeeded();
+        return;
+    }
+    if (!window->document())
+        return;
+}
+RefPtr<EventListener> listener = V8EventListenerList::getEventListener(info[1], false, ListenerFind{{lookup_type}});
+if (listener) {
+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithNullCheck>, stringResource, info[0]);

[haraken, 2013/11/13 08:33:53]

stringResource => eventName ?

[Nils Barth (inactive), 2013/11/13 11:33:29]

Done.

+    impl->{{method_name}}(stringResource, listener{{pass_ref_ptr_handling}}, info[2]->BooleanValue());
+    if (!impl->toNode())
+        {{hidden_dependency_action}}HiddenDependency(info.Holder(), info[1], {{v8_class_name}}::eventListenerCacheIndex, info.GetIsolate());

[haraken, 2013/11/13 08:33:53]

Probably we need to refactor *HiddenDependency code in V8Utilities.cpp.

- "Hidden dependency" sounds weird to me. As far as I read the code in
V8Utilities, hidden dependency is just an anonymous version of hidden values.
(i.e., in case of hidden values, we need to specify a property name when adding
a hidden value. In case of hidden dependency, we can add hidden values without
property names.)

- Can createHiddenDependency be called with an index other than
eventListenerCacheIndex? If no, we can rename things as follows:

-- eventListenerCacheIndex => anonymousHiddenValues
-- {create,remove,transfer}HiddenDependency =>
{set,unset,move}AnonymousHiddenValues

[Nils Barth (inactive), 2013/11/13 11:33:29]

Got it; added to TODO, and will look into.

Quick note:
*HiddenDependency code is only used for EventListener,
and it's clearly hacky, so I think we can sensibly make
this simpler and at least more explicitly named.

[haraken, 2013/11/14 04:33:02]

Regarding hidden dependency, kouhei-san might have thoughts about how to
refactor. So you can ask him offline.

[Nils Barth (inactive), 2013/11/14 05:47:20]

Thanks, will do!

+}
+{% endmacro %}
+
+
+{######################################}
 {% macro cpp_method_call(method, v8_set_return_value, cpp_value) %}
 {% if method.is_call_with_script_state %}
 ScriptState* currentState = ScriptState::current();