Bug 96793

Created attachment 164290 [details] Patch

Created attachment 164291 [details] Patch

Modified ChangeLog more clearly.

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.

(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

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

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

Created attachment 164300 [details] Patch

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

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

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

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?

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.

Created attachment 164322 [details] Patch

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"

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

Created attachment 164405 [details] Patch

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

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

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.

Created attachment 164551 [details] Patch

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?

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.

(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

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? :)

Comment on attachment 164551 [details] Patch Clearing flags on attachment: 164551 Committed r128943: <http://trac.webkit.org/changeset/128943>

All reviewed patches have been landed. Closing bug.

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

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