[webkit-reviews] review granted: [Bug 36802] setting document.title doesn't change document.title value : [Attachment 52137] v1; fix style violation

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted MORITA Hajime <morrita at google.com>'s
request for review:
Bug 36802: setting document.title doesn't change document.title value
https://bugs.webkit.org/show_bug.cgi?id=36802

Attachment 52137: v1; fix style violation
https://bugs.webkit.org/attachment.cgi?id=52137&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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!