Bug 28709

Summary:

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

Product:

WebKit

Reporter:

Vitaly Repeshko <vitalyr>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	none
patch: fixed ChangedLog formatting	none
patch: fixed V8 bindings	dglazkov: review+, eric: commit-queue+

Vitaly Repeshko

Reported 2009-08-25 05:49:18 PDT

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

Attachments
patch (13.98 KB, patch) 2009-08-25 05:51 PDT, Vitaly Repeshko	no flags	Details Formatted Diff Diff
patch: fixed ChangedLog formatting (14.01 KB, patch) 2009-08-25 05:58 PDT, Vitaly Repeshko	no flags	Details Formatted Diff Diff
patch: fixed V8 bindings (16.57 KB, patch) 2009-08-28 06:51 PDT, Vitaly Repeshko	dglazkov: review+ eric: commit-queue+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Vitaly Repeshko

Comment 1 2009-08-25 05:51:56 PDT

Created attachment 38543 [details] patch

Vitaly Repeshko

Comment 2 2009-08-25 05:58:07 PDT

Created attachment 38544 [details] patch: fixed ChangedLog formatting

Sam Weinig

Comment 3 2009-08-25 06:39:13 PDT

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

Vitaly Repeshko

Comment 4 2009-08-25 07:52:21 PDT

(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 5 2009-08-25 08:38:03 PDT

Comment on attachment 38544 [details] patch: fixed ChangedLog formatting Clearing review flag based on latest comment.

Vitaly Repeshko

Comment 6 2009-08-28 06:49:27 PDT

(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

Comment 7 2009-08-28 06:51:37 PDT

Created attachment 38731 [details] patch: fixed V8 bindings

Dimitri Glazkov (Google)

Comment 8 2009-08-28 08:58:26 PDT

Comment on attachment 38731 [details] patch: fixed V8 bindings r=me.

Eric Seidel (no email)

Comment 9 2009-08-28 09:05:25 PDT

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

Comment 10 2009-08-28 10:30:32 PDT

(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)

Comment 11 2009-08-28 12:05:03 PDT

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

Eric Seidel (no email)

Comment 12 2009-08-28 18:36:47 PDT

Comment on attachment 38731 [details] patch: fixed V8 bindings unrelated test crashed.

Note You need to log in before you can comment on or make changes to this bug.