Update Document's event listener type bitfield when adopting a Node

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Update Document's event listener type bitfield when adopting a Node

[Adam Klein]

Created attachment 197189 [details]
Patch

[Darin Adler]

Comment on attachment 197189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197189&action=review

Looks fine.

> Source/WebCore/dom/Node.cpp:2158
> +            Vector<AtomicString> types = listenerMap.eventTypes();

If I had more time I’d study the implementation of EventListenerMap::eventTypes, but is it really necessary to copy a vector of atomic strings? Is there any more efficient way to get at the event type strings, perhaps without allocating memory?

[Adam Klein]

(In reply to comment #2)
> (From update of attachment 197189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197189&action=review
> 
> Looks fine.
> 
> > Source/WebCore/dom/Node.cpp:2158
> > +            Vector<AtomicString> types = listenerMap.eventTypes();
> 
> If I had more time I’d study the implementation of EventListenerMap::eventTypes, but is it really necessary to copy a vector of atomic strings? Is there any more efficient way to get at the event type strings, perhaps without allocating memory?

I poked around for a bit because I had the same thoughts. The solution would be to expose some sort of iterator interface from EventListenerMap to let one iterate over the types without exposing the implementation (as a vector of pairs).

However, I think it's likely not worth doing just for this use, since the normal case is no EventTargetData, or just a few listeners. So the vector isn't likely to be large, even in the rare case when it's allocated.

[WebKit Commit Bot]

Comment on attachment 197189 [details]
Patch

Clearing flags on attachment: 197189

Committed r148072: <http://trac.webkit.org/changeset/148072>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 197189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197189&action=review

>>> Source/WebCore/dom/Node.cpp:2158
>>> +            Vector<AtomicString> types = listenerMap.eventTypes();
>> 
>> If I had more time I’d study the implementation of EventListenerMap::eventTypes, but is it really necessary to copy a vector of atomic strings? Is there any more efficient way to get at the event type strings, perhaps without allocating memory?
> 
> I poked around for a bit because I had the same thoughts. The solution would be to expose some sort of iterator interface from EventListenerMap to let one iterate over the types without exposing the implementation (as a vector of pairs).
> 
> However, I think it's likely not worth doing just for this use, since the normal case is no EventTargetData, or just a few listeners. So the vector isn't likely to be large, even in the rare case when it's allocated.

If the common case is a small vector, then an example of an efficient data structure that does not require memory allocation could be Vector<AtomicString, 4>.

[Adam Klein]

(In reply to comment #6)
> (From update of attachment 197189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197189&action=review
> 
> >>> Source/WebCore/dom/Node.cpp:2158
> >>> +            Vector<AtomicString> types = listenerMap.eventTypes();
> >> 
> >> If I had more time I’d study the implementation of EventListenerMap::eventTypes, but is it really necessary to copy a vector of atomic strings? Is there any more efficient way to get at the event type strings, perhaps without allocating memory?
> > 
> > I poked around for a bit because I had the same thoughts. The solution would be to expose some sort of iterator interface from EventListenerMap to let one iterate over the types without exposing the implementation (as a vector of pairs).
> > 
> > However, I think it's likely not worth doing just for this use, since the normal case is no EventTargetData, or just a few listeners. So the vector isn't likely to be large, even in the rare case when it's allocated.
> 
> If the common case is a small vector, then an example of an efficient data structure that does not require memory allocation could be Vector<AtomicString, 4>.

Seems like a reasonable follow-up, not sure if I'll have time to put it together myself. 2 seems like it might be the right size, to match the sizing in EventListenerMap::m_entries. C++11 would make such changes easier, as one could use "auto" instead of having to update callers of eventTypes().