Summary: | setting document.title doesn't change document.title value | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, darin, webkit.review.bot | ||||||||||||
Priority: | P3 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-03-29 20:41:41 PDT
There is no attachment in this bug. Created attachment 52009 [details] reproduce >There is no attachment in this bug. Oops. I've failed to attach it. Sorry for that. Created attachment 52135 [details]
patch v0
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.
Created attachment 52137 [details]
v1; fix style violation
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. 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! (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. Created attachment 52282 [details] v2; fixed typo, did respect the change on Bug 25567 Created attachment 52283 [details]
v3; fix changelog wording a litle
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. 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 Comment on attachment 52283 [details] v3; fix changelog wording a litle Clearing flags on attachment: 52283 Committed r56974: <http://trac.webkit.org/changeset/56974> All reviewed patches have been landed. Closing bug. |