Bug 68955 – [MutationObservers] Implement WebKitMutationObserver.observe for childList changes

• Component: DOM
• Status: RESOLVED FIXED

[Adam Klein]

[MutationObservers] Implement WebKitMutationObserver.observe for childList changes

[Rafael Weinstein]

Created attachment 111531 [details]
Patch

[Adam Klein]

Comment on attachment 111531 [details]
Patch

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

> LayoutTests/fast/mutation/observe-childList-expected.txt:1
> +Test the constructor of WebKitMutationObserver

Need to fix this boilerplate

> LayoutTests/fast/mutation/observe-childList.html:251
> +description('Test the constructor of WebKitMutationObserver');

This is the corresponding line.

[WebKit Review Bot]

Attachment 111531 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
Source/WebCore/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Weinstein]

Created attachment 111535 [details]
Patch

[WebKit Review Bot]

Attachment 111535 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 111535 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: fast/mutation/observe-childList.html

Nit: This line should appear after the description.

> Source/WebCore/dom/MutationScope.cpp:46
> +namespace {

We don't normally use anonymous namespace in WebCore.

> Source/WebCore/dom/MutationScope.cpp:48
> +class ChildListAccumulation {

I'd call this class MutatedChildList.

> Source/WebCore/dom/MutationScope.cpp:73
> +class ChildListAccumulator {

I'd call this class ChildListMutationList.

> Source/WebCore/dom/MutationScope.cpp:112
> +        dispatch();

I don't think scoping level is guaranteed to be 0 at this point. childAdded could be called inside a mutation, load, unload, beforeunload, etc... event listeners could have modified DOM. event listener. r- because of this.

> Source/WebCore/dom/MutationScope.cpp:118
> +        ASSERT(m_nextSibling == child->nextSibling()); // Additions must be in-order.

Why is this condition guaranteed?

> Source/WebCore/dom/MutationScope.cpp:131
> +    ASSERT(isEmpty() || m_nextSibling.get() == child); // Removals must be continuous and in-order.

Again, I don't quite understand what guarantees this condition.

> Source/WebCore/dom/MutationScope.cpp:153
> +    RefPtr<MutationRecord> mutation = MutationRecord::createChildList(m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling, m_nextSibling);

Maybe split it into two lines?

> Source/WebCore/dom/MutationScope.cpp:155
> +    for (Vector<WebKitMutationObserver*>::iterator iter = m_observers.begin(); iter != m_observers.end(); ++iter)
> +        (*iter)->enqueueMutationRecord(mutation);

Please use size_t i instead.

> Source/WebCore/dom/MutationScope.cpp:182
> +// Can this get called

I don't think this comment adds any information. Remove?

> Source/WebCore/dom/MutationScope.cpp:193
> +    OwnPtr<ChildListAccumulator> instance = adoptPtr(new ChildListAccumulator);
> +    s_instance = instance.leakPtr();

This can be done in one line:
s_instance = adoptPtr(new ChildListAccumulator).leakPtr().

> Source/WebCore/dom/MutationScope.cpp:207
> +    ASSERT(m_accumulations.contains(target));

Why can we assert that m_accumulations always contain the target?

> Source/WebCore/dom/MutationScope.cpp:215
> +    ASSERT(m_accumulations.contains(target));

Ditto.

> Source/WebCore/dom/MutationScope.cpp:251
> +    if (record)
> +        record->dispatch();

I'm getting confused about this. Why do we need this scoping object all if we're implemented the end-of-micro-task timing?

[Rafael Weinstein]

Created attachment 111559 [details]
Patch

[Rafael Weinstein]

Comment on attachment 111535 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Test: fast/mutation/observe-childList.html
> 
> Nit: This line should appear after the description.

done

>> Source/WebCore/dom/MutationScope.cpp:46
>> +namespace {
> 
> We don't normally use anonymous namespace in WebCore.

Removed.

>> Source/WebCore/dom/MutationScope.cpp:48
>> +class ChildListAccumulation {
> 
> I'd call this class MutatedChildList.

done

>> Source/WebCore/dom/MutationScope.cpp:73
>> +class ChildListAccumulator {
> 
> I'd call this class ChildListMutationList.

done

>> Source/WebCore/dom/MutationScope.cpp:112
>> +        dispatch();
> 
> I don't think scoping level is guaranteed to be 0 at this point. childAdded could be called inside a mutation, load, unload, beforeunload, etc... event listeners could have modified DOM. event listener. r- because of this.

Scoping level doesn't need to be zero. The idea here is that this class is making an "attempt" to roll up all adds and removes into a single chidList mutation, but it is tolerant of failing -- at which point it will dispatch() all accumulated adds/removes as a single mutation, clear it's state, and then restart the accumulation.

>> Source/WebCore/dom/MutationScope.cpp:118
>> +        ASSERT(m_nextSibling == child->nextSibling()); // Additions must be in-order.
> 
> Why is this condition guaranteed?

It's not, but as with above, I'll move this and dispatch if this occurs.

>> Source/WebCore/dom/MutationScope.cpp:131
>> +    ASSERT(isEmpty() || m_nextSibling.get() == child); // Removals must be continuous and in-order.
> 
> Again, I don't quite understand what guarantees this condition.

This shouldn't occur based on the current code. If it does, it's still correct to dispatch() and start another mutation record, but the ASSERT will hopefully alert whomever is making the change that the implementation of the accumulator needs to be tolerant of the new pattern.

>> Source/WebCore/dom/MutationScope.cpp:153
>> +    RefPtr<MutationRecord> mutation = MutationRecord::createChildList(m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling, m_nextSibling);
> 
> Maybe split it into two lines?

done. not sure if this is right. couldn't find discussion in style guide.

>> Source/WebCore/dom/MutationScope.cpp:155
>> +        (*iter)->enqueueMutationRecord(mutation);
> 
> Please use size_t i instead.

done

>> Source/WebCore/dom/MutationScope.cpp:182
>> +// Can this get called
> 
> I don't think this comment adds any information. Remove?

woops. good catch.

>> Source/WebCore/dom/MutationScope.cpp:193
>> +    s_instance = instance.leakPtr();
> 
> This can be done in one line:
> s_instance = adoptPtr(new ChildListAccumulator).leakPtr().

done

>> Source/WebCore/dom/MutationScope.cpp:207
>> +    ASSERT(m_accumulations.contains(target));
> 
> Why can we assert that m_accumulations always contain the target?

Both here and below, the target will have been entered into the map in the ctor of the MutationScope. This assert is guarding against future misuse of this class

[WebKit Review Bot]

Attachment 111559 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Weinstein]

Backing up a bit: the design of mutation observers is such that a goal is to minimize the *number* of mutation records which are delivered.

For instance, if an innerHTML assignment destroys 1000 nodes, and create 1000 nodes, we'd like to represent that as 1 childList mutation record -- as opposed to 2000 records.

The need for ChildListMutationScope isn't related to the timing of the delivery of records, it's related to our goal to represent DOM operations in as few mutation records as we can.

The current webkit code implements many DOM operations in terms of others. For instance: removeChild and appendChild are directly callable from script. However, both are used by the implementation of innerHTML.

In order to avoid having to pass down the intent of any given call to lower methods (like removeChild) -- i..e should the removeChild dispatch a MutationRecord, or is this call conceptually a part of a larger operation, we need the MutationScope stack-based approach.

As to the questions of how MutationScope (and it's classes) are designed. The idea is that the general pattern of higher level DOM operations in webkit is:

1) remove any neccessary children - going in childList order
2) add any neccesary children -- going in childList order

The assertions attempt to capture the assumptions in the code right now, and document that if the assertions start failing, it means that we're likely dispatching more mutation records than necessary.

However, if the assertions fail, the code can still fall back to just dispatching more than one record for each root-level mutation scope.

I realize this is a bit complex. If you care do discuss further, let's connect online. If you have ideas how to simplify this approach, I'm all ears.

[Ryosuke Niwa]

(In reply to comment #8)
> View in context: https://bugs.webkit.org/attachment.cgi?id=111535&action=review
> >> Source/WebCore/dom/MutationScope.cpp:112
> >> +        dispatch();
> > 
> > I don't think scoping level is guaranteed to be 0 at this point. childAdded could be called inside a mutation, load, unload, beforeunload, etc... event listeners could have modified DOM. event listener. r- because of this.
> 
> Scoping level doesn't need to be zero. The idea here is that this class is making an "attempt" to roll up all adds and removes into a single chidList mutation, but it is tolerant of failing -- at which point it will dispatch() all accumulated adds/removes as a single mutation, clear it's state, and then restart the accumulation.

I see. That makes me think that we might want to rename this class to reflect this idea.  How about ChildListMutationAggregator or ChildListMutationAccumulator? Or maybe HeuristicMutationAggregator? Given the term "scope" has been used in WebCore with a connotation of the RAII idiom, the fact this class can invoke observers even when the scoping-level is non-zero might quite misleading.

> >> Source/WebCore/dom/MutationScope.cpp:118
> >> +        ASSERT(m_nextSibling == child->nextSibling()); // Additions must be in-order.
> > 
> > Why is this condition guaranteed?
> 
> It's not, but as with above, I'll move this and dispatch if this occurs.

Okay that makes sense. On somewhat related note, we might want to reconsider the function name "dispatch" since dispatch/fire mean very specific things in DOM event spec. Maybe invokeObservers or notifyObservers?

> >> Source/WebCore/dom/MutationScope.cpp:131
> >> +    ASSERT(isEmpty() || m_nextSibling.get() == child); // Removals must be continuous and in-order.
> > 
> > Again, I don't quite understand what guarantees this condition.
> 
> This shouldn't occur based on the current code. If it does, it's still correct to dispatch() and start another mutation record, but the ASSERT will hopefully alert whomever is making the change that the implementation of the accumulator needs to be tolerant of the new pattern.

I see. Might be nice to add a comment saying why this should never happen referring to call sites of this function.

> >> Source/WebCore/dom/MutationScope.cpp:207
> >> +    ASSERT(m_accumulations.contains(target));
> > 
> > Why can we assert that m_accumulations always contain the target?
> 
> Both here and below, the target will have been entered into the map in the ctor of the MutationScope. This assert is guarding against future misuse of this class

Ah, ok.

[Ryosuke Niwa]

(In reply to comment #10)
> Backing up a bit: the design of mutation observers is such that a goal is to minimize the *number* of mutation records which are delivered.
> 
> For instance, if an innerHTML assignment destroys 1000 nodes, and create 1000 nodes, we'd like to represent that as 1 childList mutation record -- as opposed to 2000 records.

I agree that this is a very desirable behavior.

> The need for ChildListMutationScope isn't related to the timing of the delivery of records, it's related to our goal to represent DOM operations in as few mutation records as we can.

Okay. I think we want to reconsider the name "Scope" as I suggested in my previous comment to make this semantics clear.

> As to the questions of how MutationScope (and it's classes) are designed. The idea is that the general pattern of higher level DOM operations in webkit is:
> 
> 1) remove any neccessary children - going in childList order
> 2) add any neccesary children -- going in childList order
> 
> The assertions attempt to capture the assumptions in the code right now, and document that if the assertions start failing, it means that we're likely dispatching more mutation records than necessary.

That's good but I think we need some comments indicating why those assertions are true.

> However, if the assertions fail, the code can still fall back to just dispatching more than one record for each root-level mutation scope.

That's a very nice fallback. Given how much bugs we've had around mutation events I wouldn't be surprised even if we overlooked something. So having a good fallback behavior is very important.

> I realize this is a bit complex. If you care do discuss further, let's connect online. If you have ideas how to simplify this approach, I'm all ears.

Thanks for the clarification. I think I understand the patch much better now.

I have one question. Is there a way to avoid adding nodes to m_accumulation when they are not observed by anyone?

[Rafael Weinstein]

Created attachment 111625 [details]
Patch

[Rafael Weinstein]

(In reply to comment #11)
> (In reply to comment #8)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111535&action=review
> > >> Source/WebCore/dom/MutationScope.cpp:112
> > >> +        dispatch();
> > > 
> > > I don't think scoping level is guaranteed to be 0 at this point. childAdded could be called inside a mutation, load, unload, beforeunload, etc... event listeners could have modified DOM. event listener. r- because of this.
> > 
> > Scoping level doesn't need to be zero. The idea here is that this class is making an "attempt" to roll up all adds and removes into a single chidList mutation, but it is tolerant of failing -- at which point it will dispatch() all accumulated adds/removes as a single mutation, clear it's state, and then restart the accumulation.
> 
> I see. That makes me think that we might want to rename this class to reflect this idea.  How about ChildListMutationAggregator or ChildListMutationAccumulator? Or maybe HeuristicMutationAggregator? Given the term "scope" has been used in WebCore with a connotation of the RAII idiom, the fact this class can invoke observers even when the scoping-level is non-zero might quite misleading.

I think "scope" is still appropriate. Producing a mutation record prior to the outer-most scope exiting is considering unexpected behavior here. The semantics of "doing something when the outer scope exits" is consistent, at least with the existing scope mechanism for mutation events.

Even if a record is generated prior, a record will always be generated when the outer scope exits, and it is a conceptual boundary point.

> 
> > >> Source/WebCore/dom/MutationScope.cpp:118
> > >> +        ASSERT(m_nextSibling == child->nextSibling()); // Additions must be in-order.
> > > 
> > > Why is this condition guaranteed?
> > 
> > It's not, but as with above, I'll move this and dispatch if this occurs.
> 
> Okay that makes sense. On somewhat related note, we might want to reconsider the function name "dispatch" since dispatch/fire mean very specific things in DOM event spec. Maybe invokeObservers or notifyObservers?

Agreed. I've renamed it to enqueueMutationRecord. To your point, no script is invoked here and all that is happening is the production of a record and it being enqueued to observers delivery queues.

> 
> > >> Source/WebCore/dom/MutationScope.cpp:131
> > >> +    ASSERT(isEmpty() || m_nextSibling.get() == child); // Removals must be continuous and in-order.
> > > 
> > > Again, I don't quite understand what guarantees this condition.
> > 
> > This shouldn't occur based on the current code. If it does, it's still correct to dispatch() and start another mutation record, but the ASSERT will hopefully alert whomever is making the change that the implementation of the accumulator needs to be tolerant of the new pattern.
> 
> I see. Might be nice to add a comment saying why this should never happen referring to call sites of this function.

I think there are going to be too many call sites for this (more will be coming in another patch). I've added comments to the class that hopefully clarify the assumptions.

> 
> > >> Source/WebCore/dom/MutationScope.cpp:207
> > >> +    ASSERT(m_accumulations.contains(target));
> > > 
> > > Why can we assert that m_accumulations always contain the target?
> > 
> > Both here and below, the target will have been entered into the map in the ctor of the MutationScope. This assert is guarding against future misuse of this class
> 
> Ah, ok.

[Rafael Weinstein]

(In reply to comment #12)
> (In reply to comment #10)
> > Backing up a bit: the design of mutation observers is such that a goal is to minimize the *number* of mutation records which are delivered.
> > 
> > For instance, if an innerHTML assignment destroys 1000 nodes, and create 1000 nodes, we'd like to represent that as 1 childList mutation record -- as opposed to 2000 records.
> 
> I agree that this is a very desirable behavior.
> 
> > The need for ChildListMutationScope isn't related to the timing of the delivery of records, it's related to our goal to represent DOM operations in as few mutation records as we can.
> 
> Okay. I think we want to reconsider the name "Scope" as I suggested in my previous comment to make this semantics clear.
> 
> > As to the questions of how MutationScope (and it's classes) are designed. The idea is that the general pattern of higher level DOM operations in webkit is:
> > 
> > 1) remove any neccessary children - going in childList order
> > 2) add any neccesary children -- going in childList order
> > 
> > The assertions attempt to capture the assumptions in the code right now, and document that if the assertions start failing, it means that we're likely dispatching more mutation records than necessary.
> 
> That's good but I think we need some comments indicating why those assertions are true.
> 
> > However, if the assertions fail, the code can still fall back to just dispatching more than one record for each root-level mutation scope.
> 
> That's a very nice fallback. Given how much bugs we've had around mutation events I wouldn't be surprised even if we overlooked something. So having a good fallback behavior is very important.
> 
> > I realize this is a bit complex. If you care do discuss further, let's connect online. If you have ideas how to simplify this approach, I'm all ears.
> 
> Thanks for the clarification. I think I understand the patch much better now.
> 
> I have one question. Is there a way to avoid adding nodes to m_accumulation when they are not observed by anyone?

Yes. This is already happening. When the outer-most scope begins, a check is made to see if there are any observers at that node of type childList, if not m_accumulations has null added as its accumulation, and thus all inner scopes with the same target and child change calls simple exit after a single constant-time lookup for the accumulator.

See line 238.

[Rafael Weinstein]

Created attachment 111650 [details]
Patch

[Rafael Weinstein]

Last patch adds some new tests and catches one place in ContainerNode where MutationScope needed to be used to coalesce mutations.

[Rafael Weinstein]

Created attachment 111656 [details]
Patch

[Adam Klein]

Comment on attachment 111535 [details]
Patch

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

>>> Source/WebCore/dom/MutationScope.cpp:46
>>> +namespace {
>> 
>> We don't normally use anonymous namespace in WebCore.
> 
> Removed.

I asked Rafael to put this back in, it's required to ensure that we don't violate the One Definition Rule.  By my count, there are already 86 anonymous namespaces in WebCore, so this is far from uncommon.  And it's the only way to limit a class to file scope.

[Rafael Weinstein]

Last patch restores namespace and reduces hash lookups via iterators

[Ryosuke Niwa]

(In reply to comment #19)
> >>> Source/WebCore/dom/MutationScope.cpp:46
> >>> +namespace {
> >> 
> >> We don't normally use anonymous namespace in WebCore.
> > 
> > Removed.
> 
> I asked Rafael to put this back in, it's required to ensure that we don't violate the One Definition Rule.  By my count, there are already 86 anonymous namespaces in WebCore, so this is far from uncommon.  And it's the only way to limit a class to file scope.

Those 86 anonymous namespaces should probably all go away. The same thing with index / iterator argument :( Maybe we should add that to the style guideline.

[Ryosuke Niwa]

Comment on attachment 111656 [details]
Patch

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

> Source/WebCore/dom/MutationScope.cpp:52
> +class RecordAccumulator {

I'd prefer a fully qualified name such as MutationRecordAccumulator. I still think it should be called aggregator because it's not accumulating records. It's accumulating a list of mutated nodes in order to aggregate mutation records.

> Source/WebCore/dom/MutationScope.cpp:77
> +class AccumulationManager {

I think the name AccumulationManager is too generic. Talking with eseidel to come up with a better name on IRC now.

[Rafael Weinstein]

Comment on attachment 111656 [details]
Patch

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

>> Source/WebCore/dom/MutationScope.cpp:52
>> +class RecordAccumulator {
> 
> I'd prefer a fully qualified name such as MutationRecordAccumulator. I still think it should be called aggregator because it's not accumulating records. It's accumulating a list of mutated nodes in order to aggregate mutation records.

done

>> Source/WebCore/dom/MutationScope.cpp:77
>> +class AccumulationManager {
> 
> I think the name AccumulationManager is too generic. Talking with eseidel to come up with a better name on IRC now.

done.

[Rafael Weinstein]

Created attachment 111864 [details]
Patch

[WebKit Review Bot]

Attachment 111864 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/ContainerNode.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Weinstein]

Created attachment 111869 [details]
Patch

[WebKit Review Bot]

Attachment 111869 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/ContainerNode.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 111864 [details]
Patch

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

> Source/WebCore/dom/ChildListMutationScope.cpp:114
> +bool ChildListMutationAccumulator::isAddedNodeInOrder(Node* child)

I'd add inline keyword here although I'm certain most optimizing compilers will do it automatically.

> Source/WebCore/dom/ChildListMutationScope.cpp:132
> +    m_addedNodes.append(child);
> +    m_lastAdded = child;

If you reverse the order, you don't need to define a local child and you can do:
m_lastAdded = child;
m_addedNodes.append(m_lastAdded);

> Source/WebCore/dom/ChildListMutationScope.cpp:135
> +bool ChildListMutationAccumulator::isRemovedNodeInOrder(Node* child)

Ditto about the inline keyword.

> Source/WebCore/dom/ChildListMutationScope.cpp:142
> +    RefPtr<Node> child = prpChild;

You don't need a local child here at all.

> Source/WebCore/dom/ChildListMutationScope.cpp:252
> +        m_accumulations.set(target, new ChildListMutationAccumulator(target, observers));

Oh oops, I didn't catch this in my previous review but we need to be using HashMap<Node*, OwnPtr<ChildListMutationAccumulator*> > instead for m_accumulations.set. "Raw new" is strictly disallowed in WebCore. Sorry for not noticing this earlier :( r- due to this.

I'm not even sure if HashMap supports having OwnPtr though. I guess we'd have to try it.

> Source/WebCore/dom/ContainerNode.cpp:27
>  #include "MemoryCache.h"

Could you move this line right above where MutationEvent.h is?

[Ryosuke Niwa]

(In reply to comment #28)
> > Source/WebCore/dom/ChildListMutationScope.cpp:252
> > +        m_accumulations.set(target, new ChildListMutationAccumulator(target, observers));
> 
> Oh oops, I didn't catch this in my previous review but we need to be using HashMap<Node*, OwnPtr<ChildListMutationAccumulator*> > instead for m_accumulations.set. "Raw new" is strictly disallowed in WebCore. Sorry for not noticing this earlier :( r- due to this.
> 
> I'm not even sure if HashMap supports having OwnPtr though. I guess we'd have to try it.

Okay, it apparently doesn't :( So let's either make ChildListMutationAccumulator ref-counted or add a comment that m_accumulations should use OwnPtr but we can't because HashMap doesn't support OwnPtr.

[Rafael Weinstein]

Created attachment 111991 [details]
Patch

[Rafael Weinstein]

Comment on attachment 111864 [details]
Patch

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

>> Source/WebCore/dom/ChildListMutationScope.cpp:114
>> +bool ChildListMutationAccumulator::isAddedNodeInOrder(Node* child)
> 
> I'd add inline keyword here although I'm certain most optimizing compilers will do it automatically.

done

>> Source/WebCore/dom/ChildListMutationScope.cpp:132
>> +    m_lastAdded = child;
> 
> If you reverse the order, you don't need to define a local child and you can do:
> m_lastAdded = child;
> m_addedNodes.append(m_lastAdded);

I feel like doing this makes the code less clear. Also it means that the PRP is accessed a bunch via get() which I know is safe, but seems really brittle to me.

From Darin's discussion: "Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases."

>> Source/WebCore/dom/ChildListMutationScope.cpp:135
>> +bool ChildListMutationAccumulator::isRemovedNodeInOrder(Node* child)
> 
> Ditto about the inline keyword.

done

>> Source/WebCore/dom/ChildListMutationScope.cpp:142
>> +    RefPtr<Node> child = prpChild;
> 
> You don't need a local child here at all.

ditto response from above

>> Source/WebCore/dom/ChildListMutationScope.cpp:252
>> +        m_accumulations.set(target, new ChildListMutationAccumulator(target, observers));
> 
> Oh oops, I didn't catch this in my previous review but we need to be using HashMap<Node*, OwnPtr<ChildListMutationAccumulator*> > instead for m_accumulations.set. "Raw new" is strictly disallowed in WebCore. Sorry for not noticing this earlier :( r- due to this.
> 
> I'm not even sure if HashMap supports having OwnPtr though. I guess we'd have to try it.

done. using RefPtr

>> Source/WebCore/dom/ContainerNode.cpp:28
>> +#include "ChildListMutationScope.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

done

[Ryosuke Niwa]

Comment on attachment 111991 [details]
Patch

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

> Source/WebCore/dom/ChildListMutationScope.cpp:53
> +class ChildListMutationAccumulator : public RefCounted<WebKitMutationObserver> {

I'm totally confused. Why is RefCounted templated with WebKitMutationObserver?

[Ryosuke Niwa]

Comment on attachment 111991 [details]
Patch

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

> Source/WebCore/dom/ChildListMutationScope.cpp:124
> +    if (!isAddedNodeInOrder(child.get()))

Should we add an assertion here?

> Source/WebCore/dom/ChildListMutationScope.cpp:145
> +    ASSERT(m_addedNodes.isEmpty() && isRemovedNodeInOrder(child.get()));

you should be calling isRemovedNodeInOrder here.

[Ryosuke Niwa]

Comment on attachment 111991 [details]
Patch

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

> Source/WebCore/dom/ChildListMutationScope.cpp:169
> +            m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling, m_nextSibling);

Indentation here seems wrong. should be indented by exactly 8 spaces in the total (4 spaces from where RefPtr is)

[Rafael Weinstein]

Comment on attachment 111991 [details]
Patch

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

>> Source/WebCore/dom/ChildListMutationScope.cpp:124
>> +    if (!isAddedNodeInOrder(child.get()))
> 
> Should we add an assertion here?

I think there is currently one case where this legitimately happens and it's fine to dispatch the record.

>> Source/WebCore/dom/ChildListMutationScope.cpp:145
>> +    ASSERT(m_addedNodes.isEmpty() && isRemovedNodeInOrder(child.get()));
> 
> you should be calling isRemovedNodeInOrder here.

it is called. there are two things being asserted here: that no additions have taken place yet, and that the removal is taking place in order.

[Rafael Weinstein]

Created attachment 111995 [details]
Patch

[Rafael Weinstein]

Comment on attachment 111991 [details]
Patch

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

>> Source/WebCore/dom/ChildListMutationScope.cpp:53
>> +class ChildListMutationAccumulator : public RefCounted<WebKitMutationObserver> {
> 
> I'm totally confused. Why is RefCounted templated with WebKitMutationObserver?

Ack. Copy paste error. Why didn't I get compile errors on this? Fixed.

>> Source/WebCore/dom/ChildListMutationScope.cpp:169
>> +            m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling, m_nextSibling);
> 
> Indentation here seems wrong. should be indented by exactly 8 spaces in the total (4 spaces from where RefPtr is)

Now four spaces -- but that seems wrong to me. 4 spaces suggests a new block level. I can't find anything in the style guide about breaking long lines. The only thing I see is that statements should all be on one line.

[Rafael Weinstein]

Created attachment 112000 [details]
Patch

[Adam Klein]

Committed r98121: <http://trac.webkit.org/changeset/98121>

[Adam Klein]

(In reply to comment #39)
> Committed r98121: <http://trac.webkit.org/changeset/98121>

Landed for rafaelw with final changes approved by rniwa.

[Rafael Weinstein]

Comment on attachment 111991 [details]
Patch

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

>>> Source/WebCore/dom/ChildListMutationScope.cpp:124
>>> +    if (!isAddedNodeInOrder(child.get()))
>> 
>> Should we add an assertion here?
> 
> I think there is currently one case where this legitimately happens and it's fine to dispatch the record.

I couldn't find the case. I added the ASSERT back. I'm sure we'll find the case again.