IDL compiler: merge and simplify EventHandler non-callable value check

Source/bindings/v8/V8EventListenerList.h

Index: Source/bindings/v8/V8EventListenerList.h
diff --git a/Source/bindings/v8/V8EventListenerList.h b/Source/bindings/v8/V8EventListenerList.h
index 1074983653612b23e81bb4534261ead90aa21e3e..9adddbbb7adbbb8df6d8fc0b2fb9e8043ce49e6f 100644
--- a/Source/bindings/v8/V8EventListenerList.h
+++ b/Source/bindings/v8/V8EventListenerList.h
@@ -52,10 +52,9 @@ public:
     static PassRefPtr<V8EventListener> findWrapper(v8::Local<v8::Value> value, bool isAttribute, v8::Isolate* isolate)
     {
         ASSERT(v8::Context::InContext());
-        // Non-callable input should be treated as null
-        if (!value->IsNull() && !value->IsFunction())
-            value = v8::Null(isolate);
-        if (!value->IsObject())
+        if (!value->IsObject()
+            // Non-callable attribute setter input is treated as null (no wrapper)
+            || (isAttribute && !value->IsFunction()))

[haraken, 2013/10/22 12:05:30]

Looks like you newly added the isAttribute check. Would you elaborate on why you
added the isAttribute check?

[Nils Barth (inactive), 2013/10/22 12:16:51]

We only want to check if it's a function on attribute setters
(not method arguments),
and this makes it consistent with the check below (in findOrCreateWrapper).

The findOrCreateWrapper isAttribute check is required:
without it, we get test failures.
This findWrapper isAttribute check is not required at present
(there are no test failures without it).

However, this makes our assumptions explicit,
and makes the code more consistent.

I.e., we're assuming isAttribute;
in the CG this was being called when passing isAttribute = true.

[haraken, 2013/10/22 12:24:23]

Makes sense. However, this changes web-exposed behavior, so we might want to
have a test.

Your CL changes behavior for a case where isAttribute is false and IsFunction is
true. Who calls findWrapper with isAttribute false?

[Nils Barth (inactive), 2013/10/23 01:43:51]

I looked into this, and found out this is always called with isAttribute ==
false, so I can remove this check,
and in fact the isAttribute parameter!
(In fact, this function is only called in one place,
and the branch that calls that also only occurs one place.)

In more detail:
findWrapper is called only one place,
namely in getEventListener:

if (lookup == ListenerFindOnly)
    return V8EventListenerList::findWrapper(value, isAttribute,
context->GetIsolate());

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

This branch is only taken if lookup == ListenerFindOnly,
...and that only happens 1 place too, namely
removeEventListenerMethod:
V8EventListenerList::getEventListener(args[1], false, ListenerFindOnly)

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

...so this code is only called by a non-attribute.
I've thus added an ASSERT(!isAttribute) in the (lookup == ListenerFindOnly)
branch,
a comment to this effect,
and then we can remove isAttribute from this function and call.

             return 0;
 
         v8::Handle<v8::String> wrapperProperty = getHiddenProperty(isAttribute, isolate);
@@ -94,10 +93,9 @@ template<typename WrapperType>
 PassRefPtr<V8EventListener> V8EventListenerList::findOrCreateWrapper(v8::Local<v8::Value> value, bool isAttribute, v8::Isolate* isolate)
 {
     ASSERT(v8::Context::InContext());
-    // Non-callable attribute setter input should be treated as null
-    if (isAttribute && !value->IsNull() && !value->IsFunction())
-        value = v8::Null(isolate);
-    if (!value->IsObject())
+    if (!value->IsObject()
+        // Non-callable attribute setter input is treated as null (no wrapper)
+        || (isAttribute && !value->IsFunction()))
         return 0;
 
     v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(value);