Bug 36802 – setting document.title doesn't change document.title value

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

[Hajime Morrita]

When <title> element has 2 children, setting document.title fails.
See attached for a concrete example.

[Alexey Proskuryakov]

There is no attachment in this bug.

[Hajime Morrita]

Created attachment 52009 [details]
reproduce

>There is no attachment in this bug.
Oops. I've failed to attach it. Sorry for that.

[Hajime Morrita]

Created attachment 52135 [details]
patch v0

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLTitleElement.cpp:84:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files


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

[Hajime Morrita]

Created attachment 52137 [details]
v1; fix style violation

[Hajime Morrita]

This problems caused by Document.setTitle() which is called during removeChildren().
So another possible approach is defer event propagation during HTMLTitleElement::setText().
But it seems too complicated because it will need a variant of both removeChidren() and addChild() 
that support deffering event dispatch, and we don't have such one.

[Darin Adler]

Comment on attachment 52137 [details]
v1; fix style violation

> +        When HTMLTitleElement::setText() is called reentrant manner by
> +        Document::seTitle(), the argument of setText() changes implicitly.

Typo: "Document:seTitle".

> +description("This test checks to see if setting documen.title works even if the title element has multiple children.");

Typo here: "documen"

> +        // We make a copy here because value.m_impl will be changed implicitly during removeChildren(),
> +        // which causes HTMLTitleElement::childrenChanged(), which ends up Document::setTitle().
> +        String valueCopy(value);

The comment is a little confusing because strings are immutable. The real issue is that value is a reference to a string from the DOM tree.

But given the special case this patch added above for empty values and no children, I don't think this second code change actually matters. Could you test without this second change? I believe only the isEmpty change is needed.

I'm going to say review+, though. This seems slightly flawed, but OK. The test is the best part!

[Darin Adler]

(In reply to comment #7)
> But given the special case this patch added above for empty values and no
> children, I don't think this second code change actually matters. Could you
> test without this second change? I believe only the isEmpty change is needed.
> 
> I'm going to say review+, though. This seems slightly flawed, but OK. The test
> is the best part!

Further, I think the fix for bug 25567 takes care of this in another way. I suggest we make only the minimum code changes needed to fix the problem.

[Hajime Morrita]

Created attachment 52282 [details]
v2; fixed typo, did respect the change on Bug 25567

[Hajime Morrita]

Created attachment 52283 [details]
v3; fix changelog wording a litle

[Hajime Morrita]

Darin, thank you reviewing!
I updated the patch.

(In reply to comment #7)
> Typo: "Document:seTitle".
> (snip)
> Typo here: "documen"
Fixed.

> 
> > +        // We make a copy here because value.m_impl will be changed implicitly during removeChildren(),
> > +        // which causes HTMLTitleElement::childrenChanged(), which ends up Document::setTitle().
> > +        String valueCopy(value);
> 
> The comment is a little confusing because strings are immutable. The real issue
> is that value is a reference to a string from the DOM tree.
Agreed. And tried to clarify the comment.

> 
> But given the special case this patch added above for empty values and no
> children, I don't think this second code change actually matters. Could you
> test without this second change? I believe only the isEmpty change is needed.
This was required. But the fix on Bug 25567 made this special case no longer usefull.
Thank you pointing it out at #8.

[Darin Adler]

Comment on attachment 52283 [details]
v3; fix changelog wording a litle

> +        // We make a copy here because entity of "value" argument can be Document::m_title,
> +        // which goes empty during removeChildren() invocation below,
> +        // which causes HTMLTitleElement::childrenChanged(), which ends up Document::setTitle().

"ends up Document::setTitle" should be "ends up calling Document::setTitle".

r=me

[WebKit Commit Bot]

Comment on attachment 52283 [details]
v3; fix changelog wording a litle

Clearing flags on attachment: 52283

Committed r56974: <http://trac.webkit.org/changeset/56974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.