Bug 25496

Summary:

Switch V8EventListenerList to a hashtable implementation

Product:

WebKit

Reporter:

Antony Sargent <asargent>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
patch	none
patch v2	none
patch v3	dglazkov: review+
patch v4	none

Antony Sargent

Reported 2009-04-30 16:08:10 PDT

This avoids some tricky issues with event listener removal in the current implmentation and has slightly better performance. http://codereview.chromium.org/79059

Attachments
patch (12.02 KB, patch) 2009-04-30 16:15 PDT, Antony Sargent	no flags	Details Formatted Diff Diff
patch v2 (12.01 KB, patch) 2009-04-30 18:43 PDT, Antony Sargent	no flags	Details Formatted Diff Diff
patch v3 (12.45 KB, patch) 2009-05-01 15:45 PDT, Antony Sargent	dglazkov: review+	Details Formatted Diff Diff
patch v4 (11.61 KB, patch) 2009-05-04 13:48 PDT, Antony Sargent	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Antony Sargent

Comment 1 2009-04-30 16:15:07 PDT

Created attachment 29925 [details] patch

Antony Sargent

Comment 2 2009-04-30 18:43:21 PDT

Created attachment 29933 [details] patch v2 A few fixes to comply with WebKit style guidelines.

Antony Sargent

Comment 3 2009-05-01 10:58:44 PDT

I'll be uploading a new version in a little while with one more small change based on offline discussion with dglazkov.

Antony Sargent

Comment 4 2009-05-01 15:45:43 PDT

Created attachment 29950 [details] patch v3 Changed the iterator from being a nested class to it's own standalone class.

Dimitri Glazkov (Google)

Comment 5 2009-05-01 18:46:21 PDT

Comment on attachment 29950 [details] patch v3 You did good! Just a few comments below. > + No new functionality so no new tests. > + > + * bindings/v8/V8EventListenerList.cpp: > + (WebCore::V8EventListenerListIterator::V8EventListenerListIterator): > + (WebCore::V8EventListenerListIterator::~V8EventListenerListIterator): > + (WebCore::V8EventListenerListIterator::operator++): > + (WebCore::V8EventListenerListIterator::operator==): > + (WebCore::V8EventListenerListIterator::operator!=): > + (WebCore::V8EventListenerListIterator::operator*): > + (WebCore::V8EventListenerListIterator::seekToEnd): > + (WebCore::V8EventListenerList::V8EventListenerList): > + (WebCore::V8EventListenerList::~V8EventListenerList): > + (WebCore::V8EventListenerList::begin): > + (WebCore::V8EventListenerList::end): > + (WebCore::getKey): > + (WebCore::V8EventListenerList::add): > + (WebCore::V8EventListenerList::remove): > + (WebCore::V8EventListenerList::clear): > + (WebCore::V8EventListenerList::find): No need to leave all these hanging. Can just say: + * bindings/v8/V8EventListenerList.cpp: Added V8EventListenerListIterator. > +int getKey(v8::Local<v8::Object> object) static int getKey(...

Antony Sargent

Comment 6 2009-05-04 13:48:18 PDT

Created attachment 29995 [details] patch v4 New Patch in response to Dmitri's last comments. Contains less verbose ChangeLog comment, and switched getKey function to static.

Eric Seidel (no email)

Comment 7 2009-05-04 16:41:24 PDT

Dimitri - Would you like to land this, or should I?

Eric Seidel (no email)

Comment 8 2009-05-05 22:23:44 PDT

Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/v8/V8EventListenerList.cpp M WebCore/bindings/v8/V8EventListenerList.h M WebCore/bindings/v8/WorkerContextExecutionProxy.cpp Committed r43277

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