Simplify and optimize ChildListMutationScope
Created attachment 165167 [details] Patch
Created attachment 165169 [details] Patch
Comment on attachment 165169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165169&action=review This looks right. > Source/WebCore/dom/ChildListMutationScope.cpp:70 > + if (!m_observers) > + return; I would move this check to ChildListMutationScope so that it can be inlined into ContainerNode member functions if I were you. > Source/WebCore/dom/ChildListMutationScope.cpp:115 > m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling.release(), m_nextSibling.release())); Nit: Wrong indentation (I know this is an existing code but...) > Source/WebCore/dom/ChildListMutationScope.cpp:149 > + delete this; I'm not a big fun of manually calling delete like this. > Source/WebCore/dom/ChildListMutationScope.h:96 > + typedef HashMap<Node*, MutationAccumulator*> AccumulatorMap; Can we use HashMap<Node*, OwnPtr<MutationAccumulator> > instead so as to avoid manual new/delete?
Comment on attachment 165169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165169&action=review Note that in my next patch I've pulled out MutationAccumulator so it's no longer an inner class. This is mostly just to make the number of colons-per-line less ridiculous in the .cpp file. I don't think there's much danger of someone trying to use this class, but I left a comment at the top saying not to anyway. >> Source/WebCore/dom/ChildListMutationScope.cpp:70 >> + return; > > I would move this check to ChildListMutationScope so that it can be inlined into ContainerNode member functions if I were you. Yeah, I guess that could be worth it. I had it that way initially but moved it to simplify the .h file, but I guess there's not much advantage to doing it that way. Fixed (and replaced with an ASSERT). >> Source/WebCore/dom/ChildListMutationScope.cpp:115 >> m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling.release(), m_nextSibling.release())); > > Nit: Wrong indentation (I know this is an existing code but...) Split into 4 statements for clarity. >> Source/WebCore/dom/ChildListMutationScope.cpp:149 >> + delete this; > > I'm not a big fun of manually calling delete like this. I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. >> Source/WebCore/dom/ChildListMutationScope.h:96 >> + typedef HashMap<Node*, MutationAccumulator*> AccumulatorMap; > > Can we use HashMap<Node*, OwnPtr<MutationAccumulator> > instead so as to avoid manual new/delete? Was using that before, but wanted to make the destructor private. See above.
Created attachment 165188 [details] Patch
Created attachment 165189 [details] Patch
Created attachment 165213 [details] Patch
(In reply to comment #4) > >> Source/WebCore/dom/ChildListMutationScope.cpp:149 > >> + delete this; > > > > I'm not a big fun of manually calling delete like this. > > I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. What's the advantage of making the destructor private?
Created attachment 165220 [details] Alternate approach: RefCount MutationAccumulator
(In reply to comment #8) > (In reply to comment #4) > > >> Source/WebCore/dom/ChildListMutationScope.cpp:149 > > >> + delete this; > > > > > > I'm not a big fun of manually calling delete like this. > > > > I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. > > What's the advantage of making the destructor private? Just trying to avoid mis-use. Take a look at the latest patch, where I ditch m_scopingLevel entirely since it's effectively a refcount, and use RefPtrs in the scopes to handle all this automatically.
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > Source/WebCore/dom/ChildListMutationScope.h:53 > +class MutationAccumulator { I would have named this ChildListMutationAccumulator now that it's not an inner class. > Source/WebCore/dom/ChildListMutationScope.h:103 > + if (m_accumulator && m_accumulator->hasObservers()) Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true.
Comment on attachment 165220 [details] Alternate approach: RefCount MutationAccumulator OMG, this code looks sooo much prettier!
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review >> Source/WebCore/dom/ChildListMutationScope.h:53 >> +class MutationAccumulator { > > I would have named this ChildListMutationAccumulator now that it's not an inner class. Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. >> Source/WebCore/dom/ChildListMutationScope.h:103 >> + if (m_accumulator && m_accumulator->hasObservers()) > > Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here?
(In reply to comment #13) > (From update of attachment 165213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > > >> Source/WebCore/dom/ChildListMutationScope.h:53 > >> +class MutationAccumulator { > > > > I would have named this ChildListMutationAccumulator now that it's not an inner class. > > Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. Autocompletion on IDEs will suffer... :( > >> Source/WebCore/dom/ChildListMutationScope.h:103 > >> + if (m_accumulator && m_accumulator->hasObservers()) > > > > Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. > > Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here? What's not clear is why we would not have observers here given that we had already checked that we had interested observers when we created m_accumulator.
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review >>>> Source/WebCore/dom/ChildListMutationScope.h:53 >>>> +class MutationAccumulator { >>> >>> I would have named this ChildListMutationAccumulator now that it's not an inner class. >> >> Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. > > Autocompletion on IDEs will suffer... :( Hmm, ok. I'll do some followups to move stuff around/rename, but I'd like to be done with this patch for now before I accidentally break it. >>>> Source/WebCore/dom/ChildListMutationScope.h:103 >>>> + if (m_accumulator && m_accumulator->hasObservers()) >>> >>> Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. >> >> Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here? > > What's not clear is why we would not have observers here given that we had already checked that we had interested observers when we created m_accumulator. So the document-level flag only tells you if there are observers _somewhere_ in the document. hasObservers() tells you whether there are actually observers in this particular subtree. That's why we create the accumulator even if there aren't any observers in the subtree: it allows us to cache the tree-walking. Not sure how naming would help here: the fact that hasMutationObserversOfType() is on Document, and that it isn't given a context node, should be a hint that it's not getting the complete story.
Created attachment 165226 [details] Patch for landing
(In reply to comment #15) > (From update of attachment 165213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > > >>>> Source/WebCore/dom/ChildListMutationScope.h:53 > >>>> +class MutationAccumulator { > >>> > >>> I would have named this ChildListMutationAccumulator now that it's not an inner class. > >> > >> Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. > > > > Autocompletion on IDEs will suffer... :( > > Hmm, ok. I'll do some followups to move stuff around/rename, but I'd like to be done with this patch for now before I accidentally break it. Did this rename for now, since I had to fix up the ChangeLog anyway.
Comment on attachment 165226 [details] Patch for landing Clearing flags on attachment: 165226 Committed r129280: <http://trac.webkit.org/changeset/129280>
All reviewed patches have been landed. Closing bug.