Summary: | Update Document's event listener type bitfield when adopting a Node | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||
Component: | New Bugs | Assignee: | Adam Klein <adamk> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, darin, rniwa | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Adam Klein
2013-04-09 17:08:09 PDT
Created attachment 197189 [details]
Patch
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? (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. Comment on attachment 197189 [details] Patch Clearing flags on attachment: 197189 Committed r148072: <http://trac.webkit.org/changeset/148072> All reviewed patches have been landed. Closing bug. 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>. (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(). |