Bug 139121 – text node should not be created, On setting document.title to the empty string

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

[Shivakumar J M]

text node should not be created, On setting document.title to the empty string as in spec: http://www.whatwg.org/specs/web-apps/current-work/#document.title http://www.whatwg.org/specs/web-apps/current-work/#the-title-element, http://www.w3.org/html/wg/drafts/html/master/dom.html#the-title-attribute

[Shivakumar J M]

Created attachment 242305 [details]
Patch

text node should not be created, On setting document.title to the empty string.

[Chris Dumez]

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

[Chris Dumez]

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.

[Shivakumar J M]

Created attachment 242386 [details]
Patch-Updated-Review1

updated as per review comments.

[Chris Dumez]

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()

[Shivakumar J M]

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+.

[Darin Adler]

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?

[Darin Adler]

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.

[Shivakumar J M]

Created attachment 243192 [details]
Patch-Updated-Review4

updated the patch with review comments

[Shivakumar J M]

(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

[Chris Dumez]

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.

[Shivakumar J M]

(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

[Shivakumar J M]

Created attachment 243278 [details]
Patch-Updated-Review4

Updated the patch for review comments to remove child node.

[Shivakumar J M]

Created attachment 243284 [details]
Patch-Updated-Review5

Updated the patch for review comments to remove child node and added more tests.

[Darin Adler]

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.

[Shivakumar J M]

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.

[Shivakumar J M]

(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

[Chris Dumez]

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.

[Shivakumar J M]

Created attachment 243349 [details]
Patch-Updated-Review7

updated the patch with comments.

[WebKit Commit Bot]

Comment on attachment 243349 [details]
Patch-Updated-Review7

Clearing flags on attachment: 243349

Committed r177435: <http://trac.webkit.org/changeset/177435>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.