[webkit-reviews] review granted: [Bug 82089] Match DOM4 spec with respect to DocumentFragment insertion : [Attachment 133570] Patch minus replaceChild ordering change

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted  review:
Bug 82089: Match DOM4 spec with respect to DocumentFragment insertion
https://bugs.webkit.org/show_bug.cgi?id=82089

Attachment 133570: Patch minus replaceChild ordering change
https://bugs.webkit.org/attachment.cgi?id=133570&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133570&action=review


>> Source/WebCore/dom/ContainerNode.cpp:287
>> +	if (next && (next->previousSibling() == newChild || next == newChild))
// nothing to do
> 
> What if next had been removed from this during the mutation event? r- because
of this.
> 
> In general, it's better to define a boolean or a helper function that
documents this than adding a comment like "nothing to do",

On my second thought, this condition was present in the old code and it'll only
match one-node case so this is probably okay.

> Source/WebCore/dom/ContainerNode.cpp:-289
> -	   // If the new child is already in the right place, we're done.
> -	   if (next && (next == child || next == child->previousSibling()))
> -	       break;

I don't think this old code was correct to begin with for the document fragment
case since there are more nodes next in the targets.
It's probably a left over from the days we still used to iterate through
nextSibling instead of collecting nodes upfront.