Bug 167065 – Document title changed twice when setting document.title

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Consider the following code:

<script>
    document.title = 'one';
    document.title = 'two';
    document.title = 'three';
</script>

In trunk, this changes the title six times: first to null, then to 'one', then to null again, then to 'two', then to null yet again. I presume that the changes to null are undesired. See bug #165073 comment 15 for a detailed description of why this happens.

[Michael Catanzaro]

(Note: this grew out of an attempt to add a GTK+ API test for bug #165073. It would be good to add a cross-platform test as well. That's harder for me.)

[Michael Catanzaro]

Created attachment 298900 [details]
Patch

[Build Bot]

Comment on attachment 298900 [details]
Patch

Attachment 298900 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2895203

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html

[Build Bot]

Created attachment 298910 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298900 [details]
Patch

Attachment 298900 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2895215

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html

[Build Bot]

Created attachment 298914 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298900 [details]
Patch

Attachment 298900 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2895220

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html

[Build Bot]

Created attachment 298915 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 298900 [details]
Patch

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

Great catch, thanks for tackling this.

I don’t think this is the right fix. I think we want to add the special behavior to setTextContent rather than to setTitle.

Do you know why this bug is not detectable in cross-platform regression tests? I’m really glad you did the testing at the GTK API level, that’s how we learned about this in the first place, but I am surprised that we can’t see the same problem in tests like fast/dom/title-text-property-2.html. Or maybe we can see it, and the patch just doesn’t have updated expected results for that test yet. Now that I look closer there do seem to be things saying the document title has changed to the empty string. Those may in fact reflect this behavior.

If that kind of test does show the symptoms of this bug, then I would like to see the patch include more of the same test cases that we are adding here in TestWebKitWebView.cpp also covered by a test done in JavaScript that is run on all platforms. Might not cover the "loading new documents" cases well, but should be able to cover the many types of DOM mutation ones well.

> Source/WebCore/dom/Document.cpp:1498
> +    if (!m_upcomingTitle.isEmpty() && m_upcomingTitle != title.string)

Using an empty string as the "no upcoming title" special value means this might not work properly for changing the title from a non-empty string to the empty string. We could use a null string or even an optional string to make this less ambiguous. But there are other problems with this approach so I won’t go deeper into it.

> Source/WebCore/dom/Document.cpp:1557
> +    // updateTitleFromTitleElement will be called twice below inside
> +    // Node::setTextContent, once with an empty title and then once with the new
> +    // title. m_upcomingTitle helps us ensure that we only inform the loader of
> +    // one title change instead of two.
> +    m_upcomingTitle = title;

I think this technique will get things wrong if there is a mutation event handler that changes the title in response to the mutation that changes the title the first time and we re-enter. In particular if the mutation changes the title by mutating the DOM and *not* with a call to set document.title. We could end up not changing the title to "x" while m_upcomingTitle was set to "a" and never communicating the new title "x" at all. So the trick where we say "deaden updateTitle to all titles except for the title I am about to set" is not the best approach.

If we want setTitle to be the special function that has the property of not making DOM changes visible as empty titles, then instead of the "upcoming title" trick, we could add a m_ignoreChangesToTitleElement boolean, and updateTitleFromTitleElement would respect it and, and then in setTitle we would write something along these lines (we would want to factor carefully to keep the code looking good and share between the HTML and SVG cases, I think):

    {
        SetForScope<bool> changingTitleElement(m_ignoreChangesToTitleElement, true);
        m_titleElement->setTextContent(title);
    }
    updateTitleFromTitleElement();

But the problem with my suggested solution is that it gives the special "atomic" property to changes to the document.title property, but not changes to the titleElement.textContent property and the other analogous composite DOM functions, and I think we want this property for the others too.

Somehow we want the document to know that a DOM mutation is in progress, and then the code that calls updateTitleFromTitleElement would instead queue up a later call to the function, which occur only after the possibly-multi-part DOM mutation is done. This is a similar concept to CustomElementReactionStack; it’s no accident that document.title is has the [CEReactions] attribute in the IDL file. It’s also a similar concept to ChildListMutationScope. But I’m not sure that either CustomElementReactionStack or ChildListMutationScope is the perfect mechanism to hook into for this.

We could also do a cruder form of merging multiple title changes into a single one. We often use timers to coalesce multiple calls to a function to a single one to avoid seeing multiple intermediate cases. When we do that, it’s returning to the "run loop" that triggers the work. So instead of updateTitleFromTitleElement, we would call updateTitleFromTitleElementSoon, and then the actual updating would take place when the timer fires. I’d hate to add a new timer just for this. HTML specifications seem to use "microtask" to formalize this. We can think of this as queuing a microtask to communicate the title’s state.

I’d love to give the one simple answer to which is the best approach, but I’m not completely sure.

If we’re mostly focused on restoring the old behavior of setting document.title, then I’d lean toward my m_ignoreChangesToTitleElement proposal; I think it gets us back no worse than where we were before without adding any new strange cases. But I think it’s strange that setting textContent of the title element will communicate an extra empty title.

Or we could decide to live with these additional title changes to empty titles.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:988
> +    test->loadHtml("<script>document.title = 'one'; document.title = 'two'; document.title = 'three';</script>", nullptr);

To be thorough we probably need a few more test cases:

We can change the title by:

- changing the title property on the document
- changing the children of the title element, either with low-level DOM function that make a single change at a time or functions that make multiple changes all at once by setting textContent, innerHTML, innerText, outerText
- changing title element itself, adding new title elements, removing the old title element, moving the title element within the document

For each of these changes to know what correct behavior is, we need a view of whether the change is atomic or not. For example, if we move the title element in the DOM using insertBefore, replaceChild, or appendChild, do we ever see a side effect of the temporary state without the title element that leads to a different title, or do we make them atomic from the point of view of title change notifications?

One thing to note is that these functions that make multiple changes that should be seen as atomic, like textContent, innerHTML, and innerText all use ChildListMutationScope to make their entire mutation atomic from the point of DOM mutation events. Perhaps our title updating should respond to that same policy.

Chris: Do you know why Node::setTextContent does not use replaceChildrenWithText?

Then there is the timing of the changes to the title. Besides the normal cases, there are these more exotic ones:

- inside a mutation event handler of a DOM mutation that itself affects the title
- inside the title changed callback

Then there are special values for the title:

- empty string
- the same value the title already has, especially when the DOM won’t be the same (multiple text nodes, for example, or collapsing of spaces)

[Michael Catanzaro]

So this turned out to be unexpectedly complicated! I don't expect to have time soon to investigate further, and seems I need to learn much more about the DOM in general to handle this properly. But hopefully my investigation above is helpful.

(In reply to comment #9)
> Do you know why this bug is not detectable in cross-platform regression
> tests? I’m really glad you did the testing at the GTK API level, that’s how
> we learned about this in the first place, but I am surprised that we can’t
> see the same problem in tests like fast/dom/title-text-property-2.html.

I was thinking it's not simple to test in a layout test because the change occurs twice before the call to document.set_title returns, so there would need to be some special test infrastructure... which it seems we already have. We just need to update the expected results.

(By the way, what tests belong under fast/? It seems it's just a collection of a bunch of random tests? Is it historical? Or were the tests really supposed to execute quicker than other tests at one point?)

> Or
> maybe we can see it, and the patch just doesn’t have updated expected
> results for that test yet. Now that I look closer there do seem to be things
> saying the document title has changed to the empty string. Those may in fact
> reflect this behavior.

Yes.

> > Source/WebCore/dom/Document.cpp:1498
> > +    if (!m_upcomingTitle.isEmpty() && m_upcomingTitle != title.string)
> 
> Using an empty string as the "no upcoming title" special value means this
> might not work properly for changing the title from a non-empty string to
> the empty string. We could use a null string or even an optional string to
> make this less ambiguous. But there are other problems with this approach so
> I won’t go deeper into it.

When changing the title from a non-empty string to an empty string, m_upcomingTitle.isEmpty() will always be true here and this early return will never occur.

But you've found other problems indeed. Thanks for the detailed review, as usual, and for insisting on correctness, as usual!

[Darin Adler]

(In reply to comment #10)
> (By the way, what tests belong under fast/? It seems it's just a collection
> of a bunch of random tests? Is it historical? Or were the tests really
> supposed to execute quicker than other tests at one point?)

It’s historical.

A long time ago, the tests under fast were a majority of the total tests, but the longer running suites were outside fast, and so "run-webkit-tests fast" was an expedient way to run tests quickly.

That didn’t last all that long, and the distinction is long obsolete, and the name is no longer apropos. Even the name of the entire tests directory, LayoutTests, seems obsolete to me.

There are some confusing things, for example the directory named "dom" has some old DOM test suites in it that originally came from the DOM standardization world--nowadays we would probably put that somewhere within the directory "imported". The directory named "fast/dom" has a lot of other useful assorted DOM tests we wrote while developing WebKIt. Nothing about the word "fast" makes that distinction clear.

I think the tests are long overdue for some reorganization, but that is a tedious project that no one has tackled in a serious way; we mostly leave the tests where they are. I think we should still take the time to search and put new tests with other similar tests even if the organization is a bit chaotic. I would welcome improvements to the organization of the tests even if they were done incrementally rather than all at once.

[Darin Adler]

Chris, I’d love to hear what you think we should do.

[Chris Dumez]

(In reply to comment #12)
> Chris, I’d love to hear what you think we should do.

Ok, I'll look into this soon.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Chris, I’d love to hear what you think we should do.
> 
> Ok, I'll look into this soon.

I am experimenting with this right now but one way to deal with this would be to implement DOM's "replace all" algorithm [1] in a way that calls ContainerNode::childrenChanged() only once. Besides avoiding the double title update issue, this would likely be a performance win as we would do less work when setting Node.textContent.

We just need to be careful about calling ContainerNode::childrenChanged() *before* DOM mutation events are fired (in particular before DOMNodeInserted & DOMSubtreeModified) so our tree is in a sane state before executing any JS.

We should also make sure DOMSubtreeModified is fired only once when setting Node.textContent instead of twice currently. Both Firefox and Chrome fire DOMSubtreeModified only once in this case.

[1] https://dom.spec.whatwg.org/#concept-node-replace-all

[Chris Dumez]

Created attachment 299002 [details]
Alternative proposal

[WebKit Commit Bot]

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


ERROR: Source/WebCore/dom/ContainerNode.h:56:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 13 files


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

[Chris Dumez]

Created attachment 299003 [details]
Alternative proposal

[Chris Dumez]

Created attachment 299005 [details]
Alternative proposal

[Chris Dumez]

Created attachment 299006 [details]
alternative proposal

[Chris Dumez]

Comment on attachment 299006 [details]
alternative proposal

Here is my proposal to fix this.

[Darin Adler]

Comment on attachment 299006 [details]
alternative proposal

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

Looks great.

Are there any other places that should be using replaceAllChildren? I think there might be in HTML editing code.

> Source/WebCore/dom/ContainerNode.cpp:348
>      ChildChange change;
> -    change.type = child.isElementNode() ? ElementInserted : child.isTextNode() ? TextInserted : NonContentsChildChanged;
> -    change.previousSiblingElement = ElementTraversal::previousSibling(child);
> -    change.nextSiblingElement = ElementTraversal::nextSibling(child);
> -    change.source = source;
> +    if (replacedAllChildren == ReplacedAllChildren::Yes)
> +        change = { AllChildrenReplaced, nullptr, nullptr, source };
> +    else {
> +        change.type = child.isElementNode() ? ElementInserted : child.isTextNode() ? TextInserted : NonContentsChildChanged;
> +        change.previousSiblingElement = ElementTraversal::previousSibling(child);
> +        change.nextSiblingElement = ElementTraversal::nextSibling(child);
> +        change.source = source;
> +    }

I think this code should be in a helper inline function; would be cleaner to return ChildChange objects instead of constructing an empty one and then assigning to it.

> Source/WebCore/dom/ContainerNode.cpp:615
> +    RELEASE_ASSERT(!is<DocumentFragment>(*node));
> +    RELEASE_ASSERT(!node->parentNode());

I’m OK with these being RELEASE_ASSERT instead of ASSERT but a little surprised; there seems little danger of either of these firing.

> Source/WebCore/dom/ContainerNode.cpp:623
> +    // Remove all parentâs children, in tree order.

We normally avoid using UTF-8 in comments, maybe we don’t need to, though.

> Source/WebCore/dom/ContainerNode.cpp:657
> +    if (!document().svgExtensions())
> +        return;
> +
> +    Element* shadowHost = this->shadowHost();
> +    if (!shadowHost || !shadowHost->hasTagName(SVGNames::useTag))
> +        document().accessSVGExtensions().rebuildElements();

I would have used auto* here. But why not use is<>? Something like this:

    if (document().svgExtensions() && is<SVGUseElement>(shadowHost()))
        document().accessSVGExtensions().rebuildElements();

> Source/WebCore/dom/ContainerNode.h:56
> +    ExceptionOr<void> replaceAllChildrenWith(RefPtr<Node>&&);

I think this should be two different functions. The removeAllChildren function doesn’t need to return an exception, and the replaceAllChildren function can take Ref<Node>&&. If you look at the implementation, the removeAllChildren case is entirely separate, and the call sites would be clearer with separate functions. I also don’t think we need the word “with” in the function’s name.

> Source/WebCore/dom/Node.cpp:1512
> +        return downcast<ContainerNode>(*this).replaceAllChildrenWith(text.isEmpty() ? RefPtr<Node>(nullptr) : document().createTextNode(text));

I think this reads better like this:

    auto& container = downcast<ContainerNode>(*this);
    if (text.isEmpty())
        container.removeAllChildren();
    else
        container.replaceAllChildren(document().createTextNode(text));

> Source/WebCore/dom/Range.cpp:1114
> +    if (newParent.hasChildNodes()) {
> +        auto result = downcast<ContainerNode>(newParent).replaceAllChildrenWith(nullptr);
>          if (result.hasException())
>              return result.releaseException();
>      }

This exception can never happen, it’s dead code.

    if (newParent.hasChildNodes())
        downcast<ContainerNode>(newParent).removeAllChildren();

But also, should steps 4, 5, and 6 be combined in some way? Maybe not in this patch, but in a future one.

[Chris Dumez]

(In reply to comment #21)
> Comment on attachment 299006 [details]
> alternative proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299006&action=review
> 
> Looks great.
> 
> Are there any other places that should be using replaceAllChildren? I think
> there might be in HTML editing code.

I can take a look in a follow up. I took care of all usages in the DOM specification. I can check other specs.

[Chris Dumez]

Comment on attachment 299006 [details]
alternative proposal

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

>> Source/WebCore/dom/ContainerNode.cpp:348
>> +    }
> 
> I think this code should be in a helper inline function; would be cleaner to return ChildChange objects instead of constructing an empty one and then assigning to it.

Will do.

>> Source/WebCore/dom/ContainerNode.cpp:615
>> +    RELEASE_ASSERT(!node->parentNode());
> 
> I’m OK with these being RELEASE_ASSERT instead of ASSERT but a little surprised; there seems little danger of either of these firing.

My worry is that someone starts using it in new code and expects the method to work with any input Node. A debug ASSERT may suffice. I was trying to be extra careful.

>> Source/WebCore/dom/ContainerNode.cpp:623
>> +    // Remove all parentâs children, in tree order.
> 
> We normally avoid using UTF-8 in comments, maybe we don’t need to, though.

Oops, not even sure how I typed this character.

>> Source/WebCore/dom/ContainerNode.cpp:657
>> +        document().accessSVGExtensions().rebuildElements();
> 
> I would have used auto* here. But why not use is<>? Something like this:
> 
>     if (document().svgExtensions() && is<SVGUseElement>(shadowHost()))
>         document().accessSVGExtensions().rebuildElements();

Will do.

>> Source/WebCore/dom/ContainerNode.h:56
>> +    ExceptionOr<void> replaceAllChildrenWith(RefPtr<Node>&&);
> 
> I think this should be two different functions. The removeAllChildren function doesn’t need to return an exception, and the replaceAllChildren function can take Ref<Node>&&. If you look at the implementation, the removeAllChildren case is entirely separate, and the call sites would be clearer with separate functions. I also don’t think we need the word “with” in the function’s name.

I was trying to match the text of the specification to make it clear when this is the right function to call.

Having a removeAllChildren() is a bit confusing because it is not clear from the name what the difference is with removeChildren(). (The difference is the number of mutations when there are mutation observers).

How about I keep the same function name and keep taking a RefPtr<> but inline the null check? Then out code looks like the spec and we do not pay the cost of the branch.

[Chris Dumez]

Comment on attachment 299006 [details]
alternative proposal

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

>>> Source/WebCore/dom/ContainerNode.h:56
>>> +    ExceptionOr<void> replaceAllChildrenWith(RefPtr<Node>&&);
>> 
>> I think this should be two different functions. The removeAllChildren function doesn’t need to return an exception, and the replaceAllChildren function can take Ref<Node>&&. If you look at the implementation, the removeAllChildren case is entirely separate, and the call sites would be clearer with separate functions. I also don’t think we need the word “with” in the function’s name.
> 
> I was trying to match the text of the specification to make it clear when this is the right function to call.
> 
> Having a removeAllChildren() is a bit confusing because it is not clear from the name what the difference is with removeChildren(). (The difference is the number of mutations when there are mutation observers).
> 
> How about I keep the same function name and keep taking a RefPtr<> but inline the null check? Then out code looks like the spec and we do not pay the cost of the branch.

I don't know yet if the compiler will like it but I'd love to be able to use overloading:
void replaceAllChildren(std::nullptr_t);
void replaceAllChildren(Ref<Node>&& newChild);

[Darin Adler]

Your plans sound fine to me as is, but I do have a few thoughts:

The distinction between removeChildren vs. replaceAllChildren is still confusing. We should consider a name change to removeChildren to make it clear when it’s OK to call it. It would not be OK to call it in the code we are modifying here and that is not obvious from the name! That has little to do with the name of the new function; the problem is in the name of the old function. I think it should have a slightly longer name that would make it clear that it’s incorrect to call directly in a case like this. Perhaps a name including "without change notification" or something like that.

As far as replaceAllChildren(nullptr) vs. removeAllChildren(), I think it’s slightly more elegant to not have an argument and also slightly nicer to not have to understand that "null" is  way to say "no children". Once we have fixed the problem with the name of removeChildren, I hope that we will decide to consider the name removeAllChildren again, or maybe we should think further on how we can match the specification’s terminology. Maybe it is replaceAllChildrenWithNoChildren? What would this be called in the specification? I don’t think it would be called "replacing all children with null".

When it comes to replaceAllChildren not accepting a fragment, this seems like an important and common pattern in many internal functions. It’s too bad there is not a type that says "node that is not a fragment or document" because there are lots of cases like this one where a fragment or document would be illegal. In functions that are part of the public DOM, of course, the fragment is used as the way to pass an entire list of children as a node. This makes me realize that we are now offering "replace all children with nothing", and "replace all children with a single node" functions, but not a "replace all children with multiple nodes" function. It’s interesting that replaceAllChildren doesn't something in its name to make it clear it only works with a single node. Maybe it should.

When it comes to whether replaceAllChildren(nullptr) and replaceAllChildren(children) are the same function, since one function can return an exception and the other can’t, I think it’s valuable to have them be separate. The overloading trick you are trying will make them be different at compile time. Using different names would make them clearly different for programmers. As an aside, I am kind of surprised that we are ignoring the possible exception in some call sites. Sam and I have been talking about following up my ExceptionOr project by making ignoring exceptions explicit once more, with a distinction between asserting that there is no exception and silently ignoring an exception that might occur that is harmless.

[Chris Dumez]

(In reply to comment #25)
> Your plans sound fine to me as is, but I do have a few thoughts:
> 
> The distinction between removeChildren vs. replaceAllChildren is still
> confusing. We should consider a name change to removeChildren to make it
> clear when it’s OK to call it. It would not be OK to call it in the code we
> are modifying here and that is not obvious from the name! That has little to
> do with the name of the new function; the problem is in the name of the old
> function. I think it should have a slightly longer name that would make it
> clear that it’s incorrect to call directly in a case like this. Perhaps a
> name including "without change notification" or something like that.
> 
> As far as replaceAllChildren(nullptr) vs. removeAllChildren(), I think it’s
> slightly more elegant to not have an argument and also slightly nicer to not
> have to understand that "null" is  way to say "no children". Once we have
> fixed the problem with the name of removeChildren, I hope that we will
> decide to consider the name removeAllChildren again, or maybe we should
> think further on how we can match the specification’s terminology. Maybe it
> is replaceAllChildrenWithNoChildren? What would this be called in the
> specification? I don’t think it would be called "replacing all children with
> null".

Actually, this is exactly what the specification is currently using:
- https://dom.spec.whatwg.org/#dom-range-surroundcontents

"If newParent has children, replace all with null within newParent."

This is why I was arguing for keeping the replaceAllChildren() name for null or a Node.

[Chris Dumez]

> When it comes to whether replaceAllChildren(nullptr) and
> replaceAllChildren(children) are the same function, since one function can
> return an exception and the other can’t, I think it’s valuable to have them
> be separate. The overloading trick you are trying will make them be
> different at compile time. Using different names would make them clearly
> different for programmers. As an aside, I am kind of surprised that we are
> ignoring the possible exception in some call sites. Sam and I have been
> talking about following up my ExceptionOr project by making ignoring
> exceptions explicit once more, with a distinction between asserting that
> there is no exception and silently ignoring an exception that might occur
> that is harmless.

Actually, none of them can return an exception. I have fixing this in my next iteration. appendChildWithoutPreInsertionValidityCheck() can only throw an exception if the newChild has a parent. We already assert that newChild does not have a parent in replaceAllChildren().

[Chris Dumez]

Created attachment 299051 [details]
Patch

[Chris Dumez]

(In reply to comment #22)
> (In reply to comment #21)
> > Comment on attachment 299006 [details]
> > alternative proposal
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=299006&action=review
> > 
> > Looks great.
> > 
> > Are there any other places that should be using replaceAllChildren? I think
> > there might be in HTML editing code.
> 
> I can take a look in a follow up. I took care of all usages in the DOM
> specification. I can check other specs.

One promising one is Element.innerHTML. The specification says we should use replace all algorithm and I know that innerHTML setter is hot so using the new method would likely improve perf. The only issue is that the innerHTML setter calls "update all" with a fragment, so we would have to add support for that.

[Chris Dumez]

Comment on attachment 299051 [details]
Patch

Darin, I am letting you take a look before I land as some of the changes were non-trivial and I may not have understood your comments correctly.

[Darin Adler]

Comment on attachment 299051 [details]
Patch

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

Since I have one tiny style comment, I decided to not set commit-queue+ for you.

> Source/WebCore/dom/Range.cpp:1112
> +    if (newParent.hasChildNodes()) {
> +        downcast<ContainerNode>(newParent).replaceAllChildren(nullptr);
>      }

Should remove these braces to fit WebKit coding style.

[Chris Dumez]

Committed r210833: <http://trac.webkit.org/changeset/210833>

[Darin Adler]

Comment on attachment 299051 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:630
> +    // If node is not null, adopt node into parentâs node document.

Found another stray UTF-8 smart quote mark.

> Source/WebCore/dom/ContainerNode.h:131
> +    void updateTreeAfterInsertion(Node& child, ReplacedAllChildren = ReplacedAllChildren::No);
> +    static ChildChange changeForChildInsertion(Node&, ChildChangeSource, ReplacedAllChildren = ReplacedAllChildren::No);

The "child" argument is named here in one call and not in the other.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 299083 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299083 [details]
Patch

Clearing flags on attachment: 299083

Committed r210839: <http://trac.webkit.org/changeset/210839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.