1. load page: http://testsuite.nokia-boston.com/content/esmp_nonESMP/navigatorObject/language1.asp actual result: language translation shows "en" expected result: language translation should show "en-US"
Created attachment 39948 [details] fix patch
navigator.language in Safari returns "en-us". Do you really want it capitalized "en-US"?
(In reply to comment #2) > navigator.language in Safari returns "en-us". Do you really want it > capitalized "en-US"? It's a surprise. I just installed Safari 4 (4.0.3 (531.9.1)) and the test case displays "en-US". I checked Firefox and Chrome and both of them show "en-US", too. This is also what our test scripts expect. I guess "en-US" is still preferred.
Comment on attachment 39948 [details] fix patch You are correct. Safari 4.0.3 on my Mac (SL) returns "en-us" but FF on the same machine returns "en-US". It seems we need to file a bug about Mac Safari, I wonder if the casing could be a compat concern. Please add a test which checks this. Ideally a simple js-only test: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree shouldBe("navigator.language", "en-US"); which I suspect we'll have to skip on Mac. I think that such a test should run fine on all localizations of OS X/Windows? since I assume we override the language settings in DumpRenderTree, but maybe my assumptions are wrong. Either way, this change needs a test of some sort or an explanation why testing is impossible/impractical. Otherwise looks good!
Created attachment 40517 [details] patch with new test case
Comment on attachment 40517 [details] patch with new test case I agree that the Qt implementation is not correct, but I don't think "en-US" is the correct value. We should be using QLocale, just like the other ports are also returning variable values instead of hard-coding "en-US".
Let me phrase this differently: Try running Safari/Firefox/Chrome in a different locale and I'm sure you'll see navigator.language return something different than "en-US" :-)
Created attachment 40654 [details] using QLocale
How often is defaultLanguage called?
uh, I dont really think that run-webkit-tests should depend on my current locale. Actually we hardcode things there (like render engine) so make it depend as little on the environment/system as possible, in order to make reliable expected test results.
(In reply to comment #9) > How often is defaultLanguage called? The bug comes from a js test case we run. I don't think the function is called every time a page is loaded or something.
(In reply to comment #10) > uh, I dont really think that run-webkit-tests should depend on my current > locale. Actually we hardcode things there (like render engine) so make it > depend as little on the environment/system as possible, in order to make > reliable expected test results. The reason I have to change run-webkit-tests is: In Qt Linux, QLocale depends on the environment variable LANG to figure out the language of the system. While I run DumpRenderTree individually, the test case passed as LANG is passed to the code. However, run-webkit-tests throws away most of the env variables including LANG before, and the test case failed because of that. As you suggested, I can hardcode the LANG here, e.g., en_US.iso88591 instead of taking the real one. What do you think? Thanks!
(In reply to comment #12) > (In reply to comment #10) > > uh, I dont really think that run-webkit-tests should depend on my current > > locale. Actually we hardcode things there (like render engine) so make it > > depend as little on the environment/system as possible, in order to make > > reliable expected test results. > > The reason I have to change run-webkit-tests is: > In Qt Linux, QLocale depends on the environment variable LANG to figure out the > language of the system. While I run DumpRenderTree individually, the test case > passed as LANG is passed to the code. However, run-webkit-tests throws away > most of the env variables including LANG before, and the test case failed > because of that. As you suggested, I can hardcode the LANG here, e.g., > en_US.iso88591 instead of taking the real one. What do you think? Thanks! Yes, I believe hardcoding it would be better. Simon?
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #10) > > > uh, I dont really think that run-webkit-tests should depend on my current > > > locale. Actually we hardcode things there (like render engine) so make it > > > depend as little on the environment/system as possible, in order to make > > > reliable expected test results. > > > > The reason I have to change run-webkit-tests is: > > In Qt Linux, QLocale depends on the environment variable LANG to figure out the > > language of the system. While I run DumpRenderTree individually, the test case > > passed as LANG is passed to the code. However, run-webkit-tests throws away > > most of the env variables including LANG before, and the test case failed > > because of that. As you suggested, I can hardcode the LANG here, e.g., > > en_US.iso88591 instead of taking the real one. What do you think? Thanks! > > Yes, I believe hardcoding it would be better. Simon? Sure. Either that or we simply make the tests expect the results from the default (C) locale?
Created attachment 40721 [details] hardcode LANG in run-webkit-tests
This seems related to bug 18994. At least the testing bits do.
(In reply to comment #16) > This seems related to bug 18994. At least the testing bits do. Thanks for pointing this out. The patch for bug18994 seems provide an API to let layoutTestController change the locale and set the default LC_ALL to "". This may have a negative impact on my patch. I should use "LC_ALL" instead of "LANG" though.
bug 18994 makes it possible to test locale-sensitive changes like this one.
Any suggestion on next step? Thanks
(In reply to comment #11) > (In reply to comment #9) > > How often is defaultLanguage called? > > The bug comes from a js test case we run. I don't think the function is called > every time a page is loaded or something. And SVGTest is calling it too. I don't think any of the top100 sites have SVG on the entry page though.
Comment on attachment 40721 [details] hardcode LANG in run-webkit-tests Why wouldnt' this use layoutTestsController.overridePOSIXLocal("en_US.iso88591"); in the one test instead of overriding it for all tests?
(In reply to comment #21) > (From update of attachment 40721 [details]) > Why wouldnt' this use > layoutTestsController.overridePOSIXLocal("en_US.iso88591"); in the one test > instead of overriding it for all tests? I thought it would be good to standardize the test environment. But sure, I can use this interface. It's still in working state, right? Shall I wait for it's checked in?
I thought it was landed already, but maybe not.
(In reply to comment #23) > I thought it was landed already, but maybe not. The code is in and bug is reopened. I will submit a new patch.
The locale-setting API change has landed. We now always call setlocale(LC_ALL, "") which means tests are run in whichever env you specify. Currently WebKit tests rely on the locale being English (or at least LC_NUMERIC not putting in commas); for example, DumpRenderTree calls snprintf directly rather than going through String::format() which is locale-independent for at least Qt. I think it'd be ok to always force an English locale when running the tests; however, WebKit needs to work from any locale! There are a bunch of WebKit bugs that forcing the locale would hide -- like what I'm working on in bug 18994 -- and until those are fixed it would be valuable to still be able to run the tests in any locale. (As I find such bugs I intend to add tests like I've added in bug 18994 that explicitly manage the locale so they will continue to test the code regardless of the locale run-webkit-tests is ran in.) Maybe a better idea is to just check LC_ALL and related variables (maybe just parse the output of the 'locale' command?) when running run-webkit-tests and warn the person running the tests accordingly that they should set their locale if they want run-webkit-tests to pass.
Thanks for the comments, Evan. That makes a lot sense. The current code does not pass LC_ALL,etc., to the executable at all. They are just Null. If this is preferred, we will keep the way it is. If we want a default value, such as en-US, I think we should set it in run-webkit-tests. Then, if any test case wants to test particular locale, use setPosixLocale in the test case. (btw, would this affect the subsequent test cases?) But I would equally take the way that each test case sets itslocale if it depends on it. My next patch will be based on the last approach. (In reply to comment #25) > The locale-setting API change has landed. We now always call setlocale(LC_ALL, > "") which means tests are run in whichever env you specify. > > Currently WebKit tests rely on the locale being English (or at least LC_NUMERIC > not putting in commas); for example, DumpRenderTree calls snprintf directly > rather than going through String::format() which is locale-independent for at > least Qt. > > I think it'd be ok to always force an English locale when running the tests; > however, WebKit needs to work from any locale! There are a bunch of WebKit > bugs that forcing the locale would hide -- like what I'm working on in bug > 18994 -- and until those are fixed it would be valuable to still be able to run > the tests in any locale. (As I find such bugs I intend to add tests like I've > added in bug 18994 that explicitly manage the locale so they will continue to > test the code regardless of the locale run-webkit-tests is ran in.) > > Maybe a better idea is to just check LC_ALL and related variables (maybe just > parse the output of the 'locale' command?) when running run-webkit-tests and > warn the person running the tests accordingly that they should set their locale > if they want run-webkit-tests to pass.
(In reply to comment #26) > Thanks for the comments, Evan. That makes a lot sense. > The current code does not pass LC_ALL,etc., to the executable at all. They are > just Null. If this is preferred, we will keep the way it is. If we want a > default value, such as en-US, I think we should set it in run-webkit-tests. Unless they're explicitly cleared somewhere, locale settings will be propagated through the environment. > Then, if any test case wants to test particular locale, use setPosixLocale in > the test case. (btw, would this affect the subsequent test cases?) No, we reset the locale (to the settings in the environment) before running each test.
> > Unless they're explicitly cleared somewhere, locale settings will be propagated > through the environment. > Actually, it is true. Look the run-webkit-tests script(sub openDumpTool()), it will take several env variables and throw away everything else. > > Then, if any test case wants to test particular locale, use setPosixLocale in > > the test case. (btw, would this affect the subsequent test cases?) > > No, we reset the locale (to the settings in the environment) before running > each test. Great.
I updated to the latest webkit on linux and made the build. However I got the following error when attempting to run either my html page or Evan's html page. Anyone has a clue? Thanks! WebKitBuild/Release/bin/DumpRenderTree --qt -o /home/cshu/LayoutTests/ LayoutTests/fast/css/opacity-float.html CONSOLE MESSAGE: line 6: TypeError: Result of expression 'layoutTestController.setPOSIXLocale' [undefined] is not a function.
(In reply to comment #29) > I updated to the latest webkit on linux and made the build. However I got the > following error when attempting to run either my html page or Evan's html page. > Anyone has a clue? Thanks! > > WebKitBuild/Release/bin/DumpRenderTree --qt -o /home/cshu/LayoutTests/ > LayoutTests/fast/css/opacity-float.html > CONSOLE MESSAGE: line 6: TypeError: Result of expression > 'layoutTestController.setPOSIXLocale' [undefined] is not a function. Check that you have these definitions: DumpRenderTree/LayoutTestController.cpp:static JSValueRef setPOSIXLocaleCallback(JSContextRef context, JSObjectRef DumpRenderTree/LayoutTestController.cpp: controller->setPOSIXLocale(locale.get()); DumpRenderTree/LayoutTestController.cpp: { "setPOSIXLocale", setPOSIXLocaleCallback, kJSPropertyAttributeRea DumpRenderTree/LayoutTestController.cpp:void LayoutTestController::setPOSIXLocale(JSStringRef locale) DumpRenderTree/LayoutTestController.h: void setPOSIXLocale(JSStringRef locale); And then check that this code is in your DRT build.
Yes. To make sure everything, I checked out a new copy, made the build from scratch but I got the same error. The grep also shows the code is there. I am not sure if running on qt-linux will make a difference. [cshu@webkit.org.trunk.rel] grep setPOSIXLocale WebKitTools/DumpRenderTree/LayoutTestController.cpp static JSValueRef setPOSIXLocaleCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) controller->setPOSIXLocale(locale.get()); { "setPOSIXLocale", setPOSIXLocaleCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, void LayoutTestController::setPOSIXLocale(JSStringRef locale) (In reply to comment #30) > (In reply to comment #29) > > I updated to the latest webkit on linux and made the build. However I got the > > following error when attempting to run either my html page or Evan's html page. > > Anyone has a clue? Thanks! > > > > WebKitBuild/Release/bin/DumpRenderTree --qt -o /home/cshu/LayoutTests/ > > LayoutTests/fast/css/opacity-float.html > > CONSOLE MESSAGE: line 6: TypeError: Result of expression > > 'layoutTestController.setPOSIXLocale' [undefined] is not a function. > > Check that you have these definitions: > > DumpRenderTree/LayoutTestController.cpp:static JSValueRef > setPOSIXLocaleCallback(JSContextRef context, JSObjectRef > DumpRenderTree/LayoutTestController.cpp: > controller->setPOSIXLocale(locale.get()); > DumpRenderTree/LayoutTestController.cpp: { "setPOSIXLocale", > setPOSIXLocaleCallback, kJSPropertyAttributeRea > DumpRenderTree/LayoutTestController.cpp:void > LayoutTestController::setPOSIXLocale(JSStringRef locale) > DumpRenderTree/LayoutTestController.h: void setPOSIXLocale(JSStringRef > locale); > > And then check that this code is in your DRT build.
(In reply to comment #31) > Yes. To make sure everything, I checked out a new copy, made the build from > scratch but I got the same error. The grep also shows the code is there. I am > not sure if running on qt-linux will make a difference. The code is there, but perhaps Qt doesn't build that code in?
It seems this LayoutTestController.cpp is NOT compiled/linked into DumpRenderTree on qt! See this DumpRenderTree.pro file: SOURCES = WorkQueue.cpp DumpRenderTree.cpp main.cpp jsobjects.cpp testplugin.cpp
I guess it is out of the scope of this bug to port LayoutTestController to qt. Shall I create a new bug and try to make it work?
yeah, I made the qt work now. Will address this in another bug.
I created a new bug 30268 to address the missing implementation of setPOSIXLocale on Qt issue and put the fixing patch.
Created attachment 41173 [details] Use setPOSIXLocale to set system locale for the test case
I think your patch looks fine! But I'm wondering: Why skip the other platforms? Now that you're using a globally supported DRT function (setPOSIXLocale), shouldn't the test pass on the other platforms? :) I suggest to try it on windows/mac or ask someone on IRC to quickly run just your layout test on mac/win, and if it passes remove them from the skip list.
(In reply to comment #38) > I think your patch looks fine! But I'm wondering: Why skip the other platforms? > Now that you're using a globally supported DRT function (setPOSIXLocale), > shouldn't the test pass on the other platforms? :) > > I suggest to try it on windows/mac or ask someone on IRC to quickly run just > your layout test on mac/win, and if it passes remove them from the skip list. The other platforms failed before. But I will run again since the new setPOSIXLocale may help.
(In reply to comment #38) > I think your patch looks fine! But I'm wondering: Why skip the other platforms? > Now that you're using a globally supported DRT function (setPOSIXLocale), > shouldn't the test pass on the other platforms? :) > > I suggest to try it on windows/mac or ask someone on IRC to quickly run just > your layout test on mac/win, and if it passes remove them from the skip list. Safari on Mac still fails: it shows 'en-us" instead of "en-US". Safari on Windows succeeded, actually as before. I am not sure about GTK. On some other browsers, I have seen "en_US". As a result, how about I undo the "Skipped" for win? I will rely on Mac guys to fix the bug and revert the "Skipped".
Created attachment 41227 [details] remove win/Skipped as it works I verified Safari on windows works but both Mac and GTK failed. Output of Mac is "en-us" and GTK is "en".
(In reply to comment #41) > Created an attachment (id=41227) [details] > remove win/Skipped as it works > > I verified Safari on windows works but both Mac and GTK failed. Output of Mac > is "en-us" and GTK is "en". What do Firefox Mac, Firefox Win, and the various IE and opera versions do? It seems we should come to some consensus here.
Safari 4 Mac: "en-us" Firefox 3 Mac: "en-US" Opera 9.64: "en" http://tools.ietf.org/html/rfc5646 seems to be the relevant spec. https://developer.mozilla.org/en/Navigator.language is mozilla's docs.
Interesting: 2.1.1. Formatting of Language Tags At all times, language tags and their subtags, including private use and extensions, are to be treated as case insensitive: there exist conventions for the capitalization of some of the subtags, but these MUST NOT be taken to carry meaning.
Created attachment 41241 [details] new patch with test case checks "en"
Comment on attachment 41241 [details] new patch with test case checks "en" Looks fine. The spec comment didn't actually have to go in the description, it could just have been a javascript comment. Thanks. Also, the spec comment is about case insensitivity and yet we don't actually deal with case insensitivity in the test. Or at least not directly. But again, it's fine.
(In reply to comment #46) > (From update of attachment 41241 [details]) > Looks fine. The spec comment didn't actually have to go in the description, it > could just have been a javascript comment. Thanks. > > Also, the spec comment is about case insensitivity and yet we don't actually > deal with case insensitivity in the test. Or at least not directly. But > again, it's fine. Thanks for the comments and review, Eric.
Comment on attachment 41241 [details] new patch with test case checks "en" Clearing flags on attachment: 41241 Committed r49675: <http://trac.webkit.org/changeset/49675>
All reviewed patches have been landed. Closing bug.
(In reply to comment #46) > (From update of attachment 41241 [details]) > Looks fine. The spec comment didn't actually have to go in the description, it > could just have been a javascript comment. Thanks. > > Also, the spec comment is about case insensitivity and yet we don't actually > deal with case insensitivity in the test. Or at least not directly. But > again, it's fine. Eric, even this bug is closed, I think we should check against "en-us" after translate to all lower case and fix those browsers that return "en".
OK. Please file additional bugs to track those changes.
(In reply to comment #51) > OK. Please file additional bugs to track those changes. I filed a bug 30439 against GTK.