Summary: | text node should not be created, On setting document.title to the empty string | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shivakumar J M <shiva.jm> | ||||||||||||||||||
Component: | DOM | Assignee: | Shivakumar J M <shiva.jm> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Shivakumar J M
2014-12-01 01:07:53 PST
Created attachment 242305 [details]
Patch
text node should not be created, On setting document.title to the empty string.
Comment on attachment 242305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242305&action=review The change looks fine overall. > Source/WebCore/ChangeLog:9 > + Also This matches the behavior of both Firefox and Chrome. I have confimed this indeed matches the behavior of Firefox and Chrome. > Source/WebCore/html/HTMLTitleElement.cpp:107 > Could you fix the "const String &value" -> 'const String& value' in the definition since you are editing this code? > LayoutTests/fast/dom/Document/document-set-title-no-child.html:2 > +<title>document set title no child</title> We don't need to set a title here. > LayoutTests/fast/dom/Document/document-set-title-no-child.html:6 > + No need for this extra line > LayoutTests/fast/dom/Document/document-set-title-no-child.html:8 > + var head = document.documentElement.firstChild; document.head > LayoutTests/fast/dom/Document/document-set-title-no-child.html:9 > + head.removeChild(head.firstChild); head.firstChild looks fragile. But hopefully you don't need this code anymore if you don't set a <title> > LayoutTests/fast/dom/Document/document-set-title-no-child.html:10 > + shouldBeEqualToString("document.title", ""); shouldBeEmptyString() > LayoutTests/fast/dom/Document/document-set-title-no-child.html:12 > + shouldBeEqualToString("document.title", ""); shouldBeEmptyString() > LayoutTests/fast/dom/Document/document-set-title-no-child.html:13 > + shouldBe("head.lastChild.firstChild", 'null'); head.lastChild looks fragile. Maybe document.getElementsByTagName("title")[0] ? > LayoutTests/fast/dom/Document/document-set-title-no-child.html:14 > + No need for this extra line Looks like Darin r+'d the patch while I was commented. If you could fix the nits I mentioned before landing, that'd be great though. Created attachment 242386 [details]
Patch-Updated-Review1
updated as per review comments.
Comment on attachment 242386 [details] Patch-Updated-Review1 View in context: https://bugs.webkit.org/attachment.cgi?id=242386&action=review r=me with nits. > Source/WebCore/ChangeLog:9 > + I have confirmed this indeed matches the behavior of Firefox and Chrome. Oh, that was an informational comment. I wasn't asking you to copy my sentence. Sorry I wasn't clear. If you keep this sentence, I would drop the "indeed". > LayoutTests/fast/dom/Document/document-set-title-no-child.html:5 > + description("On setting document.title to the empty string, text node should not be created"); "On setting document.title to the empty string, no text node should be created" > LayoutTests/fast/dom/Document/document-set-title-no-child.html:7 > + document.title = ""; I would use evalAndLog("document.title = ''"); here to make the output clearer. > LayoutTests/fast/dom/Document/document-set-title-no-child.html:9 > + shouldBe("document.getElementsByTagName('title')[0].firstChild", 'null'); shouldBeNull() Created attachment 242394 [details]
Patch-Updated-Review2
Updated the review comments, cleared the review flag for earlier patch, since could not append the patch to old review id, which had r+.
Comment on attachment 242394 [details] Patch-Updated-Review2 View in context: https://bugs.webkit.org/attachment.cgi?id=242394&action=review > Source/WebCore/html/HTMLTitleElement.cpp:111 > + if (!value.isEmpty()) This is strange. Why do we use value here instead of valueCopy? Comment on attachment 242394 [details] Patch-Updated-Review2 View in context: https://bugs.webkit.org/attachment.cgi?id=242394&action=review >> Source/WebCore/html/HTMLTitleElement.cpp:111 >> + if (!value.isEmpty()) > > This is strange. Why do we use value here instead of valueCopy? And on further reading, this fix is insufficient. If the title element happens to already exist, and happens to have one child which is a text node, we will call setData on it, setting its data to the empty string, rather than removing the text node. We need test cases that cover that. Created attachment 243192 [details]
Patch-Updated-Review4
updated the patch with review comments
(In reply to comment #8) > Comment on attachment 242394 [details] > Patch-Updated-Review2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242394&action=review > > >> Source/WebCore/html/HTMLTitleElement.cpp:111 > >> + if (!value.isEmpty()) > > > > This is strange. Why do we use value here instead of valueCopy? > > And on further reading, this fix is insufficient. If the title element > happens to already exist, and happens to have one child which is a text > node, we will call setData on it, setting its data to the empty string, > rather than removing the text node. > > We need test cases that cover that. Dear Darin, Updated the patch to use valueCopy instead of value, also added test cases to cover other use case. But in the case as mentioned above, should we again remove child text nodes, in case of setting its data to the empty string, might be its more overhead in creation and deletion again. Thanks Comment on attachment 243192 [details] Patch-Updated-Review4 View in context: https://bugs.webkit.org/attachment.cgi?id=243192&action=review r- > Source/WebCore/html/HTMLTitleElement.cpp:111 > + if (!valueCopy.isEmpty()) In the "if (hasOneChild() && is<Text>(*firstChild()))" case, you likely need to call removeChildren() if value.isEmpty() instead of calling setData(). > LayoutTests/fast/dom/Document/document-set-title-no-child.html:17 > + shouldBeNonNull("document.getElementsByTagName('title')[0].firstChild"); I believe this should be null. (In reply to comment #11) > Comment on attachment 243192 [details] > Patch-Updated-Review4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243192&action=review > > r- > > > Source/WebCore/html/HTMLTitleElement.cpp:111 > > + if (!valueCopy.isEmpty()) > > In the "if (hasOneChild() && is<Text>(*firstChild()))" case, you likely need > to call removeChildren() if value.isEmpty() instead of calling setData(). > > > LayoutTests/fast/dom/Document/document-set-title-no-child.html:17 > > + shouldBeNonNull("document.getElementsByTagName('title')[0].firstChild"); > > I believe this should be null. Dear Chris, yes right, if we remove text node In the "if (hasOneChild() && is<Text>(*firstChild()))" case, if value.isEmpty(), then "document.getElementsByTagName('title')[0].firstChild" will be null. So will there be any overhead in creation and deletion of text node again for empty case?. If it has not much effect, i will modify the code and resubmit the patch. Thanks Created attachment 243278 [details]
Patch-Updated-Review4
Updated the patch for review comments to remove child node.
Created attachment 243284 [details]
Patch-Updated-Review5
Updated the patch for review comments to remove child node and added more tests.
Comment on attachment 243284 [details] Patch-Updated-Review5 View in context: https://bugs.webkit.org/attachment.cgi?id=243284&action=review > Source/WebCore/html/HTMLTitleElement.cpp:102 > + if (value.isEmpty()) > + removeChildren(); This is OK, but I think it would be cleaner to just put !value.isEmpty() into the if conditional above instead. The general case handles the empty string just fine. I would write: if (!value.isEmpty() && hasOneChild() && is<Text>(*firstChild())) { downcast<Text>(*firstChild()).setData(value, IGNORE_EXCEPTION); return; } Because of the early return we wouldn’t have to nest the main body of the function inside an if statement. > Source/WebCore/html/HTMLTitleElement.cpp:106 > + } > else { This is not correct formatting for WebKit coding style. The brace goes on the same line as the else. > Source/WebCore/html/HTMLTitleElement.cpp:116 > + appendChild(document().createTextNode(valueCopy.impl()), IGNORE_EXCEPTION); The .impl() can be removed here. Not sure why it was ever added. Created attachment 243342 [details]
Patch-Updated-Review6
updated the patch with comments, could not append the patch to earlier patch which had r+ already, due to some setup/proxy issue in my setup.
(In reply to comment #15) > Comment on attachment 243284 [details] > Patch-Updated-Review5 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243284&action=review > > > Source/WebCore/html/HTMLTitleElement.cpp:102 > > + if (value.isEmpty()) > > + removeChildren(); > > This is OK, but I think it would be cleaner to just put !value.isEmpty() > into the if conditional above instead. The general case handles the empty > string just fine. I would write: > > if (!value.isEmpty() && hasOneChild() && is<Text>(*firstChild())) { > downcast<Text>(*firstChild()).setData(value, IGNORE_EXCEPTION); > return; > } > > Because of the early return we wouldn’t have to nest the main body of the > function inside an if statement. > > > Source/WebCore/html/HTMLTitleElement.cpp:106 > > + } > > else { > > This is not correct formatting for WebKit coding style. The brace goes on > the same line as the else. > > > Source/WebCore/html/HTMLTitleElement.cpp:116 > > + appendChild(document().createTextNode(valueCopy.impl()), IGNORE_EXCEPTION); > > The .impl() can be removed here. Not sure why it was ever added. Dear Darin, Thanks for the inputs, updated the patch with below comments: 1) we cannot use return; as suggested above, it give coding style error that "An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement". 2) The brace goes on the same line as the else, it is right, "check-webkit-style" did not give any error for these, also same way it is used in other places as well. 3) The .impl() has can be removed in new patch. Thanks Comment on attachment 243284 [details] Patch-Updated-Review5 View in context: https://bugs.webkit.org/attachment.cgi?id=243284&action=review >>> Source/WebCore/html/HTMLTitleElement.cpp:102 >>> + removeChildren(); >> >> This is OK, but I think it would be cleaner to just put !value.isEmpty() into the if conditional above instead. The general case handles the empty string just fine. I would write: >> >> if (!value.isEmpty() && hasOneChild() && is<Text>(*firstChild())) { >> downcast<Text>(*firstChild()).setData(value, IGNORE_EXCEPTION); >> return; >> } >> >> Because of the early return we wouldn’t have to nest the main body of the function inside an if statement. > > Dear Darin, > > Thanks for the inputs, updated the patch with below comments: > > 1) we cannot use return; as suggested above, it give coding style error that "An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement". > 2) The brace goes on the same line as the else, it is right, "check-webkit-style" did not give any error for these, also same way it is used in other places as well. > 3) The .impl() has can be removed in new patch. > > Thanks 1) So remove the "else" that comes after the return. Created attachment 243349 [details]
Patch-Updated-Review7
updated the patch with comments.
Comment on attachment 243349 [details] Patch-Updated-Review7 Clearing flags on attachment: 243349 Committed r177435: <http://trac.webkit.org/changeset/177435> All reviewed patches have been landed. Closing bug. |