Switch V8EventListenerList to a hashtable implementation...

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

[asargent_no_longer_on_chrome, 2009-04-24 18:25:37]

I updated V8EventListener.{h,cpp} and added some comments based on offline
discussion with Mike.

[Mike Belshe, 2009-04-27 23:19:02]

lgtm.

Just a few questions.

Looks like rietveld is confused and cached some of my comments from before. 
Ignore the first 4 comments in this email :-)

mike

http://codereview.chromium.org/79059/diff/1/4
File third_party/WebKit/WebCore/bindings/v8/V8EventListenerList.cpp (right):

http://codereview.chromium.org/79059/diff/1/4#newcode183
Line 183: }
nit: no {}

http://codereview.chromium.org/79059/diff/1/3
File third_party/WebKit/WebCore/bindings/v8/V8EventListenerList.h (right):

http://codereview.chromium.org/79059/diff/1/3#newcode42
Line 42: class V8EventListenerListIterator {
Is this class used?  Looks like it is not?

http://codereview.chromium.org/79059/diff/1/3#newcode51
Line 51: // for startOrEnd, true means "start" and false means "end"
I don't understand the comment.  Does this mean start at the beginning or at the
end?

http://codereview.chromium.org/79059/diff/1/3#newcode79
Line 79: Vector<V8EventListener*>* m_buckets[NUM_BUCKETS];
I'm not sure why we use the buckets.
Couldn't you just have:
  HashMap<V8EventListener*, int>  // listener to its hashcode
  HashMap<int, V8EventListner*>   // hashcode to listener

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()) {
should this be a while loop that finds the next non-null result?

http://codereview.chromium.org/79059/diff/5009/5012#newcode89
Line 89: m_vectorIndex = 0;
should this be:

m_vectorIndex = (*m_iter).second->size() - 1; 

?

http://codereview.chromium.org/79059/diff/5009/5012#newcode156
Line 156: v8::HandleScope handleScope;
Do we need a handlescope here?  I don't see any handles being created?  Or maybe
GetIdentityHash() requires it?

[asargent_no_longer_on_chrome, 2009-04-29 23:24:25]

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.