Bug 96793 – Title string should be changed when document.title is set to ''.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Byungwoo Lee]

callback function for 'title,changed' is not called for document.title='' in javascript.

[Byungwoo Lee]

Created attachment 164290 [details]
Patch

[Byungwoo Lee]

Created attachment 164291 [details]
Patch

[Byungwoo Lee]

Modified ChangeLog more clearly.

[Chris Dumez]

Comment on attachment 164291 [details]
Patch

The fix is in WebCore so you'll probably need a LayoutTest for it. A WebKit EFL test is not enough here as it affects all ports.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 164291 [details])
> The fix is in WebCore so you'll probably need a LayoutTest for it. A WebKit EFL test is not enough here as it affects all ports.

You should probably take a look at testRunner.dumpTitleChanges() and the following layout tests:
  fast/dom/title-text-property.html
  fast/dom/title-text-property-2.html

[WebKit Review Bot]

Comment on attachment 164291 [details]
Patch

Attachment 164291 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13858622

New failing tests:
fast/dom/title-text-property-2.html

[Byungwoo Lee]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 164291 [details] [details])
> > The fix is in WebCore so you'll probably need a LayoutTest for it. A WebKit EFL test is not enough here as it affects all ports.
> 
> You should probably take a look at testRunner.dumpTitleChanges() and the following layout tests:
>   fast/dom/title-text-property.html
>   fast/dom/title-text-property-2.html

Ok~ I'll create new test for this. Thanks :)

[Byungwoo Lee]

Created attachment 164300 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164300&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:366
> +#define TITLE_AFTER_CHANGED "Title after changed"

Now why is this needed?

> LayoutTests/fast/dom/title-text-property-assigning-empty-string-expected.txt:4
> +Original title is: Original Title
> +Setting new title to:
> +New title is:

Current title is: 'Original title'
Changing title to: ''
Current title is: ''

Would be more understandable

[Byungwoo Lee]

Comment on attachment 164300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164300&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:366
>> +#define TITLE_AFTER_CHANGED "Title after changed"
> 
> Now why is this needed?

This string is used at 372, 373 line for checking, so I defined it for preventing typo.
Is it better to remove this definition?

>> LayoutTests/fast/dom/title-text-property-assigning-empty-string-expected.txt:4
>> +New title is:
> 
> Current title is: 'Original title'
> Changing title to: ''
> Current title is: ''
> 
> Would be more understandable

Ok~ It's more clear. I'll apply this. Thanks :)

[Chris Dumez]

Comment on attachment 164300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164300&action=review

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:366
>>> +#define TITLE_AFTER_CHANGED "Title after changed"
>> 
>> Now why is this needed?
> 
> This string is used at 372, 373 line for checking, so I defined it for preventing typo.
> Is it better to remove this definition?

Please use static const char titleAfterChange[] = "Title after changed";
You should avoid #define in C++.

[Byungwoo Lee]

Comment on attachment 164300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164300&action=review

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:366
>>>> +#define TITLE_AFTER_CHANGED "Title after changed"
>>> 
>>> Now why is this needed?
>> 
>> This string is used at 372, 373 line for checking, so I defined it for preventing typo.
>> Is it better to remove this definition?
> 
> Please use static const char titleAfterChange[] = "Title after changed";
> You should avoid #define in C++.

To use static const char variable for the string, some additional string formatting code will be needed,
and I think that the way to make string is not an important in this test case.
Just removing the definition and using the string directly might be enough.
(Some other test cases also doesn't have any string formatting code for test html and expected result)
How about your opinion?

[Chris Dumez]

Comment on attachment 164300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164300&action=review

>>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:366
>>>>> +#define TITLE_AFTER_CHANGED "Title after changed"
>>>> 
>>>> Now why is this needed?
>>> 
>>> This string is used at 372, 373 line for checking, so I defined it for preventing typo.
>>> Is it better to remove this definition?
>> 
>> Please use static const char titleAfterChange[] = "Title after changed";
>> You should avoid #define in C++.
> 
> To use static const char variable for the string, some additional string formatting code will be needed,
> and I think that the way to make string is not an important in this test case.
> Just removing the definition and using the string directly might be enough.
> (Some other test cases also doesn't have any string formatting code for test html and expected result)
> How about your opinion?

Sure, I believe it is fine to duplicate the string here.

[Byungwoo Lee]

Created attachment 164322 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164322&action=review

> LayoutTests/fast/dom/title-text-property-2-expected.txt:3
>  TITLE CHANGED: 1. setting document.title
>  TITLE CHANGED: 2. setting title.text
> +TITLE CHANGED: 

So this is how it is already done.

> LayoutTests/fast/dom/title-text-property-assigning-empty-string-expected.txt:4
> +TITLE CHANGED: 
> +Original title is: 'Original Title'
> +Changing title to: ''
> +Current title is: ''

Maybe it makes sense doing it similarily? but adding the ''s.

So why am I getting the "TITLE CHANGED:" here and then later a "Changing title to:"?

Why not just do

TITLE CHANGED: 'New non-empty title'
TITLE CHANGED: ''

That is similar and pretty understanable. Then you also don't need to care about the original title, just set it twice.

> LayoutTests/fast/dom/title-text-property-assigning-empty-string.html:11
> +function debugOutput(str) {
> +    text = document.createTextNode(str);
> +    debugDiv = document.getElementById('debugDiv');
> +    div = document.createElement ('div');
> +    div.appendChild(text);
> +    debugDiv.appendChild(div);
> +}

why space before (? That is not the style. Many other tests do similar things. Why not reuse that code (include some js like other tests) or copy it instead of doing your own thing.

> LayoutTests/fast/dom/title-text-property-assigning-empty-string.html:38
> +<div id='debugDiv'>

debugDiv? That seems like a bad name. Most other tests use something like "console"

[Byungwoo Lee]

(In reply to comment #15)
> (From update of attachment 164322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164322&action=review
> 
> > LayoutTests/fast/dom/title-text-property-2-expected.txt:3
> >  TITLE CHANGED: 1. setting document.title
> >  TITLE CHANGED: 2. setting title.text
> > +TITLE CHANGED: 
> 
> So this is how it is already done.
> 
> > LayoutTests/fast/dom/title-text-property-assigning-empty-string-expected.txt:4
> > +TITLE CHANGED: 
> > +Original title is: 'Original Title'
> > +Changing title to: ''
> > +Current title is: ''
> 
> Maybe it makes sense doing it similarily? but adding the ''s.
Ok~ I have not enough confidence to fix the other test case which have been working fine.
I'll apply it also.

> 
> So why am I getting the "TITLE CHANGED:" here and then later a "Changing title to:"?
> 
> Why not just do
> 
> TITLE CHANGED: 'New non-empty title'
> TITLE CHANGED: ''
> 
> That is similar and pretty understanable. Then you also don't need to care about the original title, just set it twice.
> 
> > LayoutTests/fast/dom/title-text-property-assigning-empty-string.html:11
> > +function debugOutput(str) {
> > +    text = document.createTextNode(str);
> > +    debugDiv = document.getElementById('debugDiv');
> > +    div = document.createElement ('div');
> > +    div.appendChild(text);
> > +    debugDiv.appendChild(div);
> > +}
> 
> why space before (? That is not the style. Many other tests do similar things. Why not reuse that code (include some js like other tests) or copy it instead of doing your own thing.

Then maybe I copied the bad reference :)
I just copied the LayoutTests/fast/dom/title-text-property.html and fixed runTests() function.
If this has a problem, I'll change it.
And should I fix the title-text-property.html too?

> 
> > LayoutTests/fast/dom/title-text-property-assigning-empty-string.html:38
> > +<div id='debugDiv'>
> 
> debugDiv? That seems like a bad name. Most other tests use something like "console"
This also. I'll fix it to console. Then how about title-text-property.html?

[Byungwoo Lee]

Created attachment 164405 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164405&action=review

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1011
> +    InjectedBundle::shared().stringBuilder()->append("'\n");

appendLiteral!

> LayoutTests/fast/dom/title-text-property.html:8
> -    debugDiv = document.getElementById('debugDiv');
> -    div = document.createElement ('div');
> +    console = document.getElementById('console');
> +    div = document.createElement('div');

Something is wrong here... didn't you add this test in this patch? Is this a diff between your new and old patch? That is wrong if so

[Kenneth Rohde Christiansen]

Comment on attachment 164405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164405&action=review

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1011
> +    InjectedBundle::shared().stringBuilder()->append("'\n");

appendLiteral!

> LayoutTests/fast/dom/title-text-property.html:8
> -    debugDiv = document.getElementById('debugDiv');
> -    div = document.createElement ('div');
> +    console = document.getElementById('console');
> +    div = document.createElement('div');

Something is wrong here... didn't you add this test in this patch? Is this a diff between your new and old patch? That is wrong if so

[Byungwoo Lee]

Comment on attachment 164405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164405&action=review

>>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1011
>>> +    InjectedBundle::shared().stringBuilder()->append("'\n");
>> 
>> appendLiteral!
> 
> appendLiteral!

Oops, Ok!

>>> LayoutTests/fast/dom/title-text-property.html:8
>>> +    div = document.createElement('div');
>> 
>> Something is wrong here... didn't you add this test in this patch? Is this a diff between your new and old patch? That is wrong if so
> 
> Something is wrong here... didn't you add this test in this patch? Is this a diff between your new and old patch? That is wrong if so

I didn't add this test in this patch. (It has been there.)
I fixed something that you commented with the below fixes about single quotation.

[Byungwoo Lee]

Created attachment 164551 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164551&action=review

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:516
> +        printf("TITLE CHANGED: '%s'\n", (titleText && titleText->string) ? titleText->string : "");

I wonder if "titleText->string : "None" makes most sense?

[Byungwoo Lee]

Comment on attachment 164551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164551&action=review

>> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:516
>> +        printf("TITLE CHANGED: '%s'\n", (titleText && titleText->string) ? titleText->string : "");
> 
> I wonder if "titleText->string : "None" makes most sense?

I'm not sure about that. Title string can be a "None" and this can make some misunderstanding.

[Kenneth Rohde Christiansen]

(In reply to comment #23)
> (From update of attachment 164551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164551&action=review
> 
> >> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:516
> >> +        printf("TITLE CHANGED: '%s'\n", (titleText && titleText->string) ? titleText->string : "");
> > 
> > I wonder if "titleText->string : "None" makes most sense?
> 
> I'm not sure about that. Title string can be a "None" and this can make some misunderstanding.

It could write 

TITLE CHANGED: None
instead of
TITLE CHANGED: 'None'

Just an idea

[Byungwoo Lee]

Comment on attachment 164551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164551&action=review

>>>> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:516
>>>> +        printf("TITLE CHANGED: '%s'\n", (titleText && titleText->string) ? titleText->string : "");
>>> 
>>> I wonder if "titleText->string : "None" makes most sense?
>> 
>> I'm not sure about that. Title string can be a "None" and this can make some misunderstanding.
> 
> It could write 
> 
> TITLE CHANGED: None
> instead of
> TITLE CHANGED: 'None'
> 
> Just an idea

It can be more clear for checking as you thought.
But for implementing, if/else statement might be needed instead of ?/:.
If just '' can be acceptable, how about reducing the changes? :)

[WebKit Review Bot]

Comment on attachment 164551 [details]
Patch

Clearing flags on attachment: 164551

Committed r128943: <http://trac.webkit.org/changeset/128943>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

What behavior did testing show in other browsers? Does this make us match those?

[Byungwoo Lee]

(In reply to comment #28)
> What behavior did testing show in other browsers? Does this make us match those?

Yes, I tested about this with IE, firefox, chrome browser before making this patch.

When I set the document.title as empty string and check the value of document.title, the result of all three browser is empty string.
But when I set the document.title as empty string and check the title string that browser displays, IE and firefox was changed but Chrome browser was not changed. (displayed old title)