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

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

Followup to:
IDL compiler: move EventHandler null check out of generated bindings
https://codereview.chromium.org/32323012/

Cleanup, merges two if checks into one in several places.
Also adds check for isAttribute one place where we're assuming it.

BUG=239771
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160304

[haraken, 2013-10-22 12:05:29]

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
Source/bindings/v8/V8EventListenerList.h:57: || (isAttribute &&
!value->IsFunction()))

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

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Util...
File Source/bindings/v8/V8Utilities.cpp (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Util...
Source/bindings/v8/V8Utilities.cpp:155: && !newValue->IsUndefined())

If IsFunction() is true, both IsNull() and IsUndefined() should be false. Thus,
I think only the IsFunction() check would be enough.

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

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
Source/bindings/v8/V8EventListenerList.h:57: || (isAttribute &&
!value->IsFunction()))
On 2013/10/22 12:05:30, haraken wrote:
> Looks like you newly added the isAttribute check. Would you elaborate on why
you
> added the isAttribute check?

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.

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Util...
File Source/bindings/v8/V8Utilities.cpp (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Util...
Source/bindings/v8/V8Utilities.cpp:155: && !newValue->IsUndefined())
On 2013/10/22 12:05:30, haraken wrote:
> If IsFunction() is true, both IsNull() and IsUndefined() should be false.
Thus,
> I think only the IsFunction() check would be enough.

Thanks, changed!
(I wasn't sure about the relation between the various tests.)

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

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
Source/bindings/v8/V8EventListenerList.h:57: || (isAttribute &&
!value->IsFunction()))
On 2013/10/22 12:16:51, Nils Barth wrote:
> On 2013/10/22 12:05:30, haraken wrote:
> > Looks like you newly added the isAttribute check. Would you elaborate on why
> you
> > added the isAttribute check?
> 
> 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.

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]

Simplified patch, based on looking into how this is called.

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/34393005/diff/30001/Source/bindings/v8/V8Even...
Source/bindings/v8/V8EventListenerList.h:57: || (isAttribute &&
!value->IsFunction()))
On 2013/10/22 12:24:23, haraken wrote:
> Your CL changes behavior for a case where isAttribute is false and IsFunction
is
> true. Who calls findWrapper with isAttribute false?

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.

[haraken, 2013-10-23 03:28:41]

LGTM. Looks even better!

[commit-bot: I haz the power, 2013-10-23 04:28:51]

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

[commit-bot: I haz the power, 2013-10-23 05:32:47]

Change committed as 160304