IDL compiler: move EventHandler null check out of generated bindings

IDL compiler: move EventHandler null check out of generated bindings

Per request at:
https://codereview.chromium.org/30493002/#msg4
...this moves some code out of generated bindings and into bindings/v8.

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

[Nils Barth (inactive), 2013-10-22 01:46:37]

Very incremental change, just moving test from generated bindings
into v8/bindings.
Some questions on further code simplification.

(I'm skipping getEventListener and going down to findWrapper and
findOrCreateWrapper, as that's needed in other cases
and that's what's called anyway.)

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
File Source/bindings/v8/V8EventListenerList.cpp (right):

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
Source/bindings/v8/V8EventListenerList.cpp:45: v8::Isolate* isolate =
context->GetIsolate();
We should actually just pass an isolate in here, AFAICT;
we always have an isolate at the call site.
(For now making a local variable for clarity.)

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
Source/bindings/v8/V8EventListenerList.h:56: if (!value->IsNull() &&
!value->IsFunction())
We should be able to combine these tests, right?
(And likewise below.)
Is v8::Null an object?
Assuming not, it's easy to combine these as:
if (!value->IsObject() || !value->IsFunction())

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8Utilitie...
File Source/bindings/v8/V8Utilities.cpp (right):

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8Utilitie...
Source/bindings/v8/V8Utilities.cpp:153: if (!newValue->IsNull() &&
!newValue->IsFunction())
We can merge these checks, right?
As:
if (!newValue->IsNull() && newValue->IsFunction() && !newValue->IsUndefined())

[haraken, 2013-10-22 02:05:14]

LGTM. Let's make the condition merge in a follow-up.

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8EventLis...
Source/bindings/v8/V8EventListenerList.h:56: if (!value->IsNull() &&
!value->IsFunction())
On 2013/10/22 01:46:38, Nils Barth wrote:
> We should be able to combine these tests, right?
> (And likewise below.)
> Is v8::Null an object?
> Assuming not, it's easy to combine these as:
> if (!value->IsObject() || !value->IsFunction())

You're right.

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8Utilitie...
File Source/bindings/v8/V8Utilities.cpp (right):

https://codereview.chromium.org/32323012/diff/1/Source/bindings/v8/V8Utilitie...
Source/bindings/v8/V8Utilities.cpp:153: if (!newValue->IsNull() &&
!newValue->IsFunction())
On 2013/10/22 01:46:38, Nils Barth wrote:
> We can merge these checks, right?
> As:
> if (!newValue->IsNull() && newValue->IsFunction() && !newValue->IsUndefined())

Right.

[Nils Barth (inactive), 2013-10-22 02:08:55]

On 2013/10/22 02:05:14, haraken wrote:
> LGTM. Let's make the condition merge in a follow-up.

Thanks, and got it -- will do!

[commit-bot: I haz the power, 2013-10-22 02:09:05]

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

[commit-bot: I haz the power, 2013-10-22 03:38:21]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Nils Barth (inactive), 2013-10-22 07:29:29]

Fixed test failures by adding one more condition,
differentiating use by setters (here) from addEvent/removeEvent use.

https://codereview.chromium.org/32323012/diff/190001/Source/bindings/v8/V8Eve...
File Source/bindings/v8/V8EventListenerList.cpp (right):

https://codereview.chromium.org/32323012/diff/190001/Source/bindings/v8/V8Eve...
Source/bindings/v8/V8EventListenerList.cpp:49: return
V8EventListenerList::findOrCreateWrapper<V8EventListener>(value, isAttribute,
isolate);
This call fails if we don't add the isAttribute check.

https://codereview.chromium.org/32323012/diff/190001/Source/bindings/v8/V8Eve...
File Source/bindings/v8/V8EventListenerList.h (right):

https://codereview.chromium.org/32323012/diff/190001/Source/bindings/v8/V8Eve...
Source/bindings/v8/V8EventListenerList.h:98: if (isAttribute && !value->IsNull()
&& !value->IsFunction())
Note the isAttribute check.
isAttribute is true for setters (here),
but false for the other findOrCreateWrapper call,
via getEventListener, used in the IDL methods
addEventListener and removeEventListener.

See Perl CG, or the two places this occurs in EventTarget.idl,
code at V8EventTarget.cpp:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

[Nils Barth (inactive), 2013-10-22 07:30:30]

On 2013/10/22 03:38:21, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

These are real failures, b/c initial patch broke the IDL methods
EventTarget::addEvent and EventTarget::removeEvent.
Fixed by making change only apply to setters, not methods.

The following 4 tests failed, but now work in revised CL:
fast/dom/exception-getting-event-handler.html
fast/events/dispatch-to-handle-event.html
fast/forms/checkValidity-cancel.html
inspector/elements/event-listener-sidebar.html

[commit-bot: I haz the power, 2013-10-22 07:30:49]

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

[commit-bot: I haz the power, 2013-10-22 11:05:59]

Change committed as 160183