Bug 29653

Summary: [Qt] Qt-based browser fails to show the right language translation
Product: WebKit Reporter: Chang Shu <cshu>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, evan, hausmann, kenneth, zecke
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Bug Depends on: 30268    
Bug Blocks:    
Attachments:
Description Flags
fix patch
eric: review-
patch with new test case
hausmann: review-
using QLocale
none
hardcode LANG in run-webkit-tests
none
Use setPOSIXLocale to set system locale for the test case
none
remove win/Skipped as it works
none
new patch with test case checks "en" none

Description Chang Shu 2009-09-22 14:40:51 PDT
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"
Comment 1 Chang Shu 2009-09-22 14:55:11 PDT
Created attachment 39948 [details]
fix patch
Comment 2 Eric Seidel (no email) 2009-09-22 16:17:41 PDT
navigator.language in Safari returns "en-us".  Do you really want it capitalized "en-US"?
Comment 3 Chang Shu 2009-09-23 06:11:09 PDT
(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 4 Eric Seidel (no email) 2009-09-23 11:13:12 PDT
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!
Comment 5 Chang Shu 2009-10-02 08:29:39 PDT
Created attachment 40517 [details]
patch with new test case
Comment 6 Simon Hausmann 2009-10-02 09:02:13 PDT
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".
Comment 7 Simon Hausmann 2009-10-02 09:05:20 PDT
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" :-)
Comment 8 Chang Shu 2009-10-05 13:00:48 PDT
Created attachment 40654 [details]
using QLocale
Comment 9 Holger Freyther 2009-10-05 20:54:27 PDT
How often is defaultLanguage called?
Comment 10 Kenneth Rohde Christiansen 2009-10-05 20:58:03 PDT
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.
Comment 11 Chang Shu 2009-10-06 07:28:56 PDT
(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.
Comment 12 Chang Shu 2009-10-06 07:40:35 PDT
(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!
Comment 13 Kenneth Rohde Christiansen 2009-10-06 07:44:17 PDT
(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?
Comment 14 Simon Hausmann 2009-10-06 08:14:36 PDT
(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?
Comment 15 Chang Shu 2009-10-06 08:29:53 PDT
Created attachment 40721 [details]
hardcode LANG in run-webkit-tests
Comment 16 Eric Seidel (no email) 2009-10-06 08:31:34 PDT
This seems related to bug 18994.  At least the testing bits do.
Comment 17 Chang Shu 2009-10-06 08:49:12 PDT
(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.
Comment 18 Eric Seidel (no email) 2009-10-06 08:55:11 PDT
bug 18994 makes it possible to test locale-sensitive changes like this one.
Comment 19 Chang Shu 2009-10-06 09:08:14 PDT
Any suggestion on next step? Thanks
Comment 20 Holger Freyther 2009-10-06 18:32:56 PDT
(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 21 Eric Seidel (no email) 2009-10-08 12:17:57 PDT
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?
Comment 22 Chang Shu 2009-10-08 12:26:22 PDT
(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?
Comment 23 Eric Seidel (no email) 2009-10-08 12:27:44 PDT
I thought it was landed already, but maybe not.
Comment 24 Chang Shu 2009-10-08 12:38:35 PDT
(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.
Comment 25 Evan Martin 2009-10-08 12:39:45 PDT
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.
Comment 26 Chang Shu 2009-10-08 12:53:19 PDT
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.
Comment 27 Evan Martin 2009-10-08 13:02:39 PDT
(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.
Comment 28 Chang Shu 2009-10-08 15:14:02 PDT
> 
> 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.
Comment 29 Chang Shu 2009-10-09 11:12:12 PDT
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.
Comment 30 Evan Martin 2009-10-09 11:14:03 PDT
(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.
Comment 31 Chang Shu 2009-10-09 14:38:46 PDT
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.
Comment 32 Evan Martin 2009-10-09 14:46:50 PDT
(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?
Comment 33 Chang Shu 2009-10-09 14:48:00 PDT
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
Comment 34 Chang Shu 2009-10-09 14:55:38 PDT
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?
Comment 35 Chang Shu 2009-10-09 15:13:21 PDT
yeah, I made the qt work now. Will address this in another bug.
Comment 36 Chang Shu 2009-10-12 07:20:11 PDT
I created a new bug 30268 to address the missing implementation of setPOSIXLocale on Qt issue and put the fixing patch.
Comment 37 Chang Shu 2009-10-14 11:23:50 PDT
Created attachment 41173 [details]
Use setPOSIXLocale to set system locale for the test case
Comment 38 Simon Hausmann 2009-10-14 11:35:00 PDT
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.
Comment 39 Chang Shu 2009-10-14 11:38:14 PDT
(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.
Comment 40 Chang Shu 2009-10-14 18:52:34 PDT
(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".
Comment 41 Chang Shu 2009-10-15 08:37:06 PDT
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".
Comment 42 Eric Seidel (no email) 2009-10-15 12:10:42 PDT
(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.
Comment 43 Eric Seidel (no email) 2009-10-15 12:15:03 PDT
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.
Comment 44 Eric Seidel (no email) 2009-10-15 12:16:36 PDT
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.
Comment 45 Chang Shu 2009-10-15 13:12:19 PDT
Created attachment 41241 [details]
new patch with test case checks "en"
Comment 46 Eric Seidel (no email) 2009-10-15 13:21:11 PDT
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.
Comment 47 Chang Shu 2009-10-15 14:45:04 PDT
(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 48 WebKit Commit Bot 2009-10-16 05:47:26 PDT
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>
Comment 49 WebKit Commit Bot 2009-10-16 05:47:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Chang Shu 2009-10-16 08:17:30 PDT
(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".
Comment 51 Eric Seidel (no email) 2009-10-16 08:32:33 PDT
OK.  Please file additional bugs to track those changes.
Comment 52 Chang Shu 2009-10-16 09:03:07 PDT
(In reply to comment #51)
> OK.  Please file additional bugs to track those changes.

I filed a bug 30439 against GTK.