Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(10)

Issue 79059: Switch V8EventListenerList to a hashtable implementation... (Closed)

Created:
11 years, 9 months ago by asargent_no_longer_on_chrome
Modified:
9 years, 8 months ago
Reviewers:
Mike Belshe, iposva
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Switch V8EventListenerList to a hashtable implementation This avoids some tricky issues with event listener removal in the current implmentation and has slightly better performance. BUG=9775

Patch Set 1 #

Total comments: 4

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 6

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -71 lines) Patch
M third_party/WebKit/WebCore/bindings/v8/V8EventListenerList.h View 1 2 3 4 5 6 1 chunk +42 lines, -14 lines 0 comments Download
M third_party/WebKit/WebCore/bindings/v8/V8EventListenerList.cpp View 1 2 3 4 5 6 1 chunk +107 lines, -54 lines 0 comments Download
M third_party/WebKit/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M webkit/port/bindings/v8/v8_proxy.h View 1 2 3 4 5 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
asargent_no_longer_on_chrome
11 years, 9 months ago (2009-04-17 22:57:09 UTC) #1
asargent_no_longer_on_chrome
I updated V8EventListener.{h,cpp} and added some comments based on offline discussion with Mike.
11 years, 9 months ago (2009-04-24 18:25:37 UTC) #2
Mike Belshe
lgtm. Just a few questions. Looks like rietveld is confused and cached some of my ...
11 years, 9 months ago (2009-04-27 23:19:02 UTC) #3
asargent_no_longer_on_chrome
11 years, 9 months ago (2009-04-29 23:24:25 UTC) #4
http://codereview.chromium.org/79059/diff/5009/5012
File third_party/WebKit/WebCore/bindings/v8/V8EventListenerList.cpp (right):

http://codereview.chromium.org/79059/diff/5009/5012#newcode54
Line 54: if (m_iter != m_list->m_table.end()) {
On 2009/04/27 23:19:03, Mike Belshe wrote:
> should this be a while loop that finds the next non-null result?

In the V8EventListenerList::remove function, we pull out vectors if they become
empty, so there will never be an empty one.

The constructor for the iterator initializes m_iter to the begin() for the
hashmap, so it will always either point to a non-empty vector or be at end(). If
it points to a non-empty vector but m_vectorIndex references the last element of
that vector, we need to increment m_iter and set m_vectorIndex to 0.

I had forgotten that last part of setting m_vectorIndex to 0, and have added it
in with my newest upload.

http://codereview.chromium.org/79059/diff/5009/5012#newcode89
Line 89: m_vectorIndex = 0;
On 2009/04/27 23:19:03, Mike Belshe wrote:
> should this be:
> 
> m_vectorIndex = (*m_iter).second->size() - 1; 
> 
> ?

No, since m_iter is now set to m_table.end(), meaning "just past the end of
m_table". I could have any arbitrary sentinel value for the m_vectorIndex in
this case, but 0 turns out to be convenient for keeping code in the constructor
and operator++ simple.

http://codereview.chromium.org/79059/diff/5009/5012#newcode156
Line 156: v8::HandleScope handleScope;
On 2009/04/27 23:19:03, Mike Belshe wrote:
> Do we need a handlescope here?  I don't see any handles being created?  Or
maybe
> GetIdentityHash() requires it?

Good catch - I've removed it.

Powered by Google App Engine
This is Rietveld 408576698