Bug 82089 – Match DOM4 spec with respect to DocumentFragment insertion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Match DOM4 spec with respect to DocumentFragment insertion

[Adam Klein]

Created attachment 133557 [details]
Patch

[Adam Klein]

Created attachment 133558 [details]
Patch

[Philippe Normand]

Comment on attachment 133558 [details]
Patch

Attachment 133558 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12122801

[Early Warning System Bot]

Comment on attachment 133558 [details]
Patch

Attachment 133558 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12123773

[Adam Klein]

Created attachment 133564 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 133558 [details]
Patch

Talked with aklein in person. There appears to be some errors in the spec with respect to the order of mutations. I've asked aklein to separate the  atomic-removal from a document fragment for now since that's a pretty safe change to make.

[Adam Klein]

Created attachment 133570 [details]
Patch minus replaceChild ordering change

[Ryosuke Niwa]

Comment on attachment 133570 [details]
Patch minus replaceChild ordering change

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",

[Ryosuke Niwa]

Comment on attachment 133570 [details]
Patch minus replaceChild ordering change

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.

[WebKit Review Bot]

Comment on attachment 133570 [details]
Patch minus replaceChild ordering change

Clearing flags on attachment: 133570

Committed r111925: <http://trac.webkit.org/changeset/111925>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Note a regression fix was landed in http://trac.webkit.org/changeset/112323.