Bug 25496 - Switch V8EventListenerList to a hashtable implementation
Summary: Switch V8EventListenerList to a hashtable implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-30 16:08 PDT by Antony Sargent
Modified: 2009-05-05 22:23 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Antony Sargent 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
Comment 1 Antony Sargent 2009-04-30 16:15:07 PDT
Created attachment 29925 [details]
patch
Comment 2 Antony Sargent 2009-04-30 18:43:21 PDT
Created attachment 29933 [details]
patch v2

A few fixes to comply with WebKit style guidelines.
Comment 3 Antony Sargent 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.
Comment 4 Antony Sargent 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.
Comment 5 Dimitri Glazkov (Google) 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(...
Comment 6 Antony Sargent 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.
Comment 7 Eric Seidel (no email) 2009-05-04 16:41:24 PDT
Dimitri - Would you like to land this, or should I?
Comment 8 Eric Seidel (no email) 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