Bug 139121

Summary: text node should not be created, On setting document.title to the empty string
Product: WebKit Reporter: Shivakumar J M <shiva.jm>
Component: DOMAssignee: 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 Flags
Patch
none
Patch-Updated-Review1
cdumez: review+, cdumez: commit-queue-
Patch-Updated-Review2
darin: review-, darin: commit-queue-
Patch-Updated-Review4
cdumez: review-, cdumez: commit-queue-
Patch-Updated-Review4
none
Patch-Updated-Review5
darin: review+, darin: commit-queue-
Patch-Updated-Review6
none
Patch-Updated-Review7 none

Comment 1 Shivakumar J M 2014-12-01 01:27:48 PST
Created attachment 242305 [details]
Patch

text node should not be created, On setting document.title to the empty string.
Comment 2 Chris Dumez 2014-12-01 09:27:41 PST
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
Comment 3 Chris Dumez 2014-12-01 09:33:29 PST
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.
Comment 4 Shivakumar J M 2014-12-01 22:49:55 PST
Created attachment 242386 [details]
Patch-Updated-Review1

updated as per review comments.
Comment 5 Chris Dumez 2014-12-01 23:05:00 PST
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()
Comment 6 Shivakumar J M 2014-12-02 01:08:49 PST
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 7 Darin Adler 2014-12-02 09:05:48 PST
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 8 Darin Adler 2014-12-02 09:07:26 PST
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.
Comment 9 Shivakumar J M 2014-12-12 03:59:34 PST
Created attachment 243192 [details]
Patch-Updated-Review4

updated the patch with review comments
Comment 10 Shivakumar J M 2014-12-12 04:04:19 PST
(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 11 Chris Dumez 2014-12-12 09:49:56 PST
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.
Comment 12 Shivakumar J M 2014-12-12 23:24:15 PST
(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
Comment 13 Shivakumar J M 2014-12-14 19:40:15 PST
Created attachment 243278 [details]
Patch-Updated-Review4

Updated the patch for review comments to remove child node.
Comment 14 Shivakumar J M 2014-12-14 22:56:22 PST
Created attachment 243284 [details]
Patch-Updated-Review5

Updated the patch for review comments to remove child node and added more tests.
Comment 15 Darin Adler 2014-12-15 08:53:16 PST
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.
Comment 16 Shivakumar J M 2014-12-15 20:37:33 PST
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.
Comment 17 Shivakumar J M 2014-12-15 20:43:16 PST
(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 18 Chris Dumez 2014-12-15 20:53:56 PST
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.
Comment 19 Shivakumar J M 2014-12-15 22:13:19 PST
Created attachment 243349 [details]
Patch-Updated-Review7

updated the patch with comments.
Comment 20 WebKit Commit Bot 2014-12-16 21:19:24 PST
Comment on attachment 243349 [details]
Patch-Updated-Review7

Clearing flags on attachment: 243349

Committed r177435: <http://trac.webkit.org/changeset/177435>
Comment 21 WebKit Commit Bot 2014-12-16 21:19:31 PST
All reviewed patches have been landed.  Closing bug.