Bug 160728 – Optimization in Node::appendChild() is not spec-compliant

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

We have an optimization in Node::appendChild() that avoid doing any work if the node to be appended is already the last child. This optimization is not spec-compliant:
- https://dom.spec.whatwg.org/#dom-node-appendchild

The issue is that this is observable via DOM Mutation observers / listeners or DOM ranges.

[Chris Dumez]

Created attachment 285709 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 285709 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        common to call parent.appendChild(parent.lastChild). It turns out to
> +        regress the performance of things we care about though, we could fall
> +        back to do the optimization only when it is not observable.

You mean if it turns out to regress?  If we know for sure that this regresses the perf, we need to change the spec instead.

[Chris Dumez]

Created attachment 285710 [details]
Patch

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 285709 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285709&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        common to call parent.appendChild(parent.lastChild). It turns out to
> > +        regress the performance of things we care about though, we could fall
> > +        back to do the optimization only when it is not observable.
> 
> You mean if it turns out to regress?  If we know for sure that this
> regresses the perf, we need to change the spec instead.

*If* it turns out. I indeed forgot a word :) This is fixed in the latest iteration.

[Ryosuke Niwa]

Comment on attachment 285710 [details]
Patch

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

> LayoutTests/fast/dom/Node/appendChild-no-op-mutationobserver-expected.txt:1
> +Tests that calls of appendChild() lead to mutation events even if they are no-ops.

In the future, we should probably write these tests using testharness.js so that we can upstream to W3C test suite.

[WebKit Commit Bot]

Comment on attachment 285710 [details]
Patch

Clearing flags on attachment: 285710

Committed r204322: <http://trac.webkit.org/changeset/204322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.