Inhibit title update when removing children before setting new value

Inhibit title updates when removing children before setting new value

In <https://codereview.chromium.org/107513013>; children to <title>
were removed before setting the new title child. This caused one extra
title change notification to the empty string before the new value was
set.

Avoid the extra notification by ignoring title updates when children
of HTMLTitleElement change until the final appendChild with the new
text node.

Also make sure to add a child list mutation scope for the update so
that both the removal and addition ends up in the same mutation. Add
test for this.

BUG=331351
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164789

[jochen (gone - plz use gerrit), 2014-01-09 12:17:33]

https://codereview.chromium.org/128603002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/128603002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1317:
toHTMLTitleElement(m_titleElement)->setText(title);
since setText() will also remove the children, what about using

if (m_titleElement && ...)
  toHTMLTitleElement(...)->setText()
else
  updateTitle()

and remove the removeChildren() call above?

https://codereview.chromium.org/128603002/diff/1/Source/core/html/HTMLTitleEl...
File Source/core/html/HTMLTitleElement.cpp (right):

https://codereview.chromium.org/128603002/diff/1/Source/core/html/HTMLTitleEl...
Source/core/html/HTMLTitleElement.cpp:95: IgnoreTitleUpdatesWhenChildrenChange
inhibitor(this);
here you can then set the bool directly and don't need the inhibitor

[davve, 2014-01-09 12:31:00]

https://codereview.chromium.org/128603002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/128603002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1317:
toHTMLTitleElement(m_titleElement)->setText(title);
On 2014/01/09 12:17:33, jochen wrote:
> since setText() will also remove the children, what about using
> 
> if (m_titleElement && ...)
>   toHTMLTitleElement(...)->setText()
> else
>   updateTitle()
> 
> and remove the removeChildren() call above?

setText() will only remove the children when !(numChildren == 1 &&
firstChild()->isTextNode()). This is a current bug according to current spec
(shared with Firefox. I filed
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24240 to test whether we should
change the spec or the browsers).

Now that I actually put it in writing it feels obvious that we should (try to)
fix both bugs by aligning the code according to your suggestion. Thanks for the
sanity.

[davve, 2014-01-09 13:53:01]

Setting <title>.text now aligned with setting document.title. Description
updated. PTAL.

[jochen (gone - plz use gerrit), 2014-01-09 14:23:41]

lgtm

[commit-bot: I haz the power, 2014-01-09 14:36:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/128603002/80002

[commit-bot: I haz the power, 2014-01-09 14:36:14]

Failed to apply patch for Source/core/html/HTMLTitleElement.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLTitleElement.h
  Hunk #1 FAILED at 39.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLTitleElement.h.rej

Patch:       Source/core/html/HTMLTitleElement.h
Index: Source/core/html/HTMLTitleElement.h
diff --git a/Source/core/html/HTMLTitleElement.h
b/Source/core/html/HTMLTitleElement.h
index
fcdcf6a2c81e150bf455f8a46bbc027b3738a39e..ec58f26a357b6e494cf558438e15505ce5180219
100644
--- a/Source/core/html/HTMLTitleElement.h
+++ b/Source/core/html/HTMLTitleElement.h
@@ -39,6 +39,8 @@ private:
     virtual InsertionNotificationRequest insertedInto(ContainerNode*) OVERRIDE;
     virtual void removedFrom(ContainerNode*) OVERRIDE;
     virtual void childrenChanged(bool changedByParser = false, Node*
beforeChange = 0, Node* afterChange = 0, int childCountDelta = 0);
+
+    bool m_ignoreTitleUpdatesWhenChildrenChange;
 };
 
 DEFINE_NODE_TYPE_CASTS(HTMLTitleElement, hasTagName(HTMLNames::titleTag));

[commit-bot: I haz the power, 2014-01-09 14:48:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/128603002/110002

[commit-bot: I haz the power, 2014-01-09 16:40:09]

Change committed as 164789