Bug 29653 – [Qt] Qt-based browser fails to show the right language translation

• Component: Platform
• Status: RESOLVED FIXED

[Chang Shu]

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"

[Chang Shu]

Created attachment 39948 [details]
fix patch

[Eric Seidel (no email)]

navigator.language in Safari returns "en-us".  Do you really want it capitalized "en-US"?

[Chang Shu]

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

[Eric Seidel (no email)]

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!

[Chang Shu]

Created attachment 40517 [details]
patch with new test case

[Simon Hausmann]

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

[Simon Hausmann]

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

[Chang Shu]

Created attachment 40654 [details]
using QLocale

[Holger Freyther]

How often is defaultLanguage called?

[Kenneth Rohde Christiansen]

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.

[Chang Shu]

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

[Chang Shu]

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

[Kenneth Rohde Christiansen]

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

[Simon Hausmann]

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

[Chang Shu]

Created attachment 40721 [details]
hardcode LANG in run-webkit-tests

[Eric Seidel (no email)]

This seems related to bug 18994.  At least the testing bits do.

[Chang Shu]

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

[Eric Seidel (no email)]

bug 18994 makes it possible to test locale-sensitive changes like this one.

[Chang Shu]

Any suggestion on next step? Thanks

[Holger Freyther]

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

[Eric Seidel (no email)]

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?

[Chang Shu]

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

[Eric Seidel (no email)]

I thought it was landed already, but maybe not.

[Chang Shu]

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

[Evan Martin]

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.

[Chang Shu]

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.

[Evan Martin]

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

[Chang Shu]

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

[Chang Shu]

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.

[Evan Martin]

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

[Chang Shu]

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.

[[e-mail]] 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.

[Evan Martin]

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

[Chang Shu]

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

[Chang Shu]

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?

[Chang Shu]

yeah, I made the qt work now. Will address this in another bug.

[Chang Shu]

I created a new bug 30268 to address the missing implementation of setPOSIXLocale on Qt issue and put the fixing patch.

[Chang Shu]

Created attachment 41173 [details]
Use setPOSIXLocale to set system locale for the test case

[Simon Hausmann]

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.

[Chang Shu]

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

[Chang Shu]

(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".

[Chang Shu]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Chang Shu]

Created attachment 41241 [details]
new patch with test case checks "en"

[Eric Seidel (no email)]

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.

[Chang Shu]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

(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".

[Eric Seidel (no email)]

OK.  Please file additional bugs to track those changes.

[Chang Shu]

(In reply to comment #51)
> OK.  Please file additional bugs to track those changes.

I filed a bug 30439 against GTK.