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

Source/bindings/v8/V8EventListenerList.h

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 34 matching lines @@
     ListenerFindOnly,
     ListenerFindOrCreate,
 };
 
 // This is a container for V8EventListener objects that uses hidden properties of v8::Object to speed up lookups.
 class V8EventListenerList {
 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);
         return doFindWrapper(v8::Local<v8::Object>::Cast(value), wrapperProperty, isolate);
     }
 
     template<typename WrapperType>
     static PassRefPtr<V8EventListener> findOrCreateWrapper(v8::Local<v8::Value>, bool isAttribute, v8::Isolate*);
 
     static void clearWrapper(v8::Handle<v8::Object> listenerObject, bool isAttribute, v8::Isolate* isolate)
@@ skipping 18 matching lines @@
     static inline v8::Handle<v8::String> getHiddenProperty(bool isAttribute, v8::Isolate* isolate)
     {
         return isAttribute ? V8HiddenPropertyName::attributeListener(isolate) : V8HiddenPropertyName::listener(isolate);
     }
 };
 
 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);
     v8::Handle<v8::String> wrapperProperty = getHiddenProperty(isAttribute, isolate);
 
     V8EventListener* wrapper = doFindWrapper(object, wrapperProperty, isolate);
     if (wrapper)
         return wrapper;
 
     RefPtr<V8EventListener> wrapperPtr = WrapperType::create(object, isAttribute, isolate);
     if (wrapperPtr)
         object->SetHiddenValue(wrapperProperty, v8::External::New(wrapperPtr.get()));
 
     return wrapperPtr;
 }
 
 } // namespace WebCore
 
 #endif // V8EventListenerList_h