Bug 97352 – Simplify and optimize ChildListMutationScope

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Simplify and optimize ChildListMutationScope

[Adam Klein]

Created attachment 165167 [details]
Patch

[Adam Klein]

Created attachment 165169 [details]
Patch

[Ryosuke Niwa]

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?

[Adam Klein]

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.

[Adam Klein]

Created attachment 165188 [details]
Patch

[Adam Klein]

Created attachment 165189 [details]
Patch

[Adam Klein]

Created attachment 165213 [details]
Patch

[Ryosuke Niwa]

(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?

[Adam Klein]

Created attachment 165220 [details]
Alternate approach: RefCount MutationAccumulator

[Adam Klein]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 165220 [details]
Alternate approach: RefCount MutationAccumulator

OMG, this code looks sooo much prettier!

[Adam Klein]

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?

[Ryosuke Niwa]

(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.

[Adam Klein]

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.

[Adam Klein]

Created attachment 165226 [details]
Patch for landing

[Adam Klein]

(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.

[WebKit Review Bot]

Comment on attachment 165226 [details]
Patch for landing

Clearing flags on attachment: 165226

Committed r129280: <http://trac.webkit.org/changeset/129280>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.