Bug 28709 – [V8] Made bindings use Node::setOnEvent functions for Node descendants instead of Node::setAttributeEventListener.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vitaly Repeshko]

WebCore: Make both Node::setAttributeEventListener and Node::setOnEvent forward listener to DOM window when necessary.

[Vitaly Repeshko]

Created attachment 38543 [details]
patch

[Vitaly Repeshko]

Created attachment 38544 [details]
patch: fixed ChangedLog formatting

[Sam Weinig]

I don't understand what this patch is doing if it cannot be tested?

[Vitaly Repeshko]

(In reply to comment #3)
> I don't understand what this patch is doing if it cannot be tested?

Summary of our IRC discussion with Sam:

This issue only hurts V8 bindings since they differ from JSC in how event attribute accessors are implemented.

Node::setAttributeEventListener is an internal utility function and should not be called directly.

A better solution here is to make V8 bindings use an approach similar to JSC and to make the function private. I'll post an updated patch soon.

[Mark Rowe (bdash)]

Comment on attachment 38544 [details]
patch: fixed ChangedLog formatting

Clearing review flag based on latest comment.

[Vitaly Repeshko]

(In reply to comment #4)
> A better solution here is to make V8 bindings use an approach similar to JSC
> and to make the function private. I'll post an updated patch soon.

Sam,

The cleanup part of this turns out be less trivial than changing the visibility modifier of Node::{set,get,clear}AttributeEventListener. These functions are called by classes that are neither from V8 bindings (which I fixed) nor Node descendants. All these calls use statically known event names (i.e. they all look like node->getAttributeEventListener(eventNames().foo)), but some of these event names have no corresponding Node::{setOn,on}Event accessors. So I think Node::{set,get,clear}AttributeEventListener have to stay unchanged.

[Vitaly Repeshko]

Created attachment 38731 [details]
patch: fixed V8 bindings

[Dimitri Glazkov (Google)]

Comment on attachment 38731 [details]
patch: fixed V8 bindings

r=me.

[Eric Seidel (no email)]

Comment on attachment 38731 [details]
patch: fixed V8 bindings

Rejecting patch 38731 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #4)
> > A better solution here is to make V8 bindings use an approach similar to JSC
> > and to make the function private. I'll post an updated patch soon.
> 
> Sam,
> 
> The cleanup part of this turns out be less trivial than changing the visibility
> modifier of Node::{set,get,clear}AttributeEventListener. These functions are
> called by classes that are neither from V8 bindings (which I fixed) nor Node
> descendants. All these calls use statically known event names (i.e. they all
> look like node->getAttributeEventListener(eventNames().foo)), but some of these
> event names have no corresponding Node::{setOn,on}Event accessors. So I think
> Node::{set,get,clear}AttributeEventListener have to stay unchanged.

Ok, that makes sense.  As long as the bindings code doesn't depend on them, I think we will be good.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/47872.

[Eric Seidel (no email)]

Comment on attachment 38731 [details]
patch: fixed V8 bindings

unrelated test crashed.