Bug 30440 – [GTK] Fails new test fast/js/navigator-language.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

Created attachment 41288 [details]
reimplementation of defaultLanguage

This test is failing because GTK+/Pango don't support changing locale after the first gtk_init() call, thus our defaultLanguage() implementation always returns whatever you had when you first started. I am not really sure about the patch. WebKit's notion of locale will diverge from GTK+'s in case the application changes setlocale(). If the application doing that expects WebKit to behave like GTK+ (ignore setlocale changes), this may be bad. The other way of making this test pass is to run an instance of DRT for it exclusively, which sounds like complex and FAIL to me.

[Chang Shu]

This bug relates to bug 30439. Please check if we should set 30439 as a duplicate if necessary. Btw, I thought GTK will pass the current navigator-language test if running in English environment as it only checks language code not country code.

[Gustavo Noronha (kov)]

*** Bug 30439 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

(In reply to comment #1)
> This bug relates to bug 30439. Please check if we should set 30439 as a
> duplicate if necessary. Btw, I thought GTK will pass the current
> navigator-language test if running in English environment as it only checks
> language code not country code.

Yeah, I market that one as duplicate, thanks! Yeah, the problem is that DRT runs under C locale, and GTK+ doesn't want to notice the fact that the locale changed. I'm starting to think this patch I posted is indeed a good idea, since it should fix the problem you described in the other bug.

Let me quote the important part here:

Open a GTK-based browser and 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" or "en-us"

[Chang Shu]

(In reply to comment #3)
> (In reply to comment #1)
> > This bug relates to bug 30439. Please check if we should set 30439 as a
> > duplicate if necessary. Btw, I thought GTK will pass the current
> > navigator-language test if running in English environment as it only checks
> > language code not country code.
> 
> Yeah, I market that one as duplicate, thanks! Yeah, the problem is that DRT
> runs under C locale, and GTK+ doesn't want to notice the fact that the locale
> changed. I'm starting to think this patch I posted is indeed a good idea, since
> it should fix the problem you described in the other bug.
> 
> Let me quote the important part here:
> 
> Open a GTK-based browser and 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" or "en-us"

Is it ok I attach the patch that will update the test case to this bug? Is it possible we commit the patch but keeps this bug open until your patches are done?

[Gustavo Noronha (kov)]

Also, it looks like these tests are failing when this test is run (if I skip it, the failures go away):

+fast/workers/worker-context-multi-port.html
+fast/workers/worker-multi-port.html
+fast/xmlhttprequest/xmlhttprequest-invalid-values.html
+fast/xpath/attr-namespace.html
+fast/xpath/detached-subtree-invalidate-iterator.html
+fast/xpath/py-dom-xpath/nodetests.html
+http/tests/misc/canvas-pattern-from-incremental-image.html
+http/tests/security/canvas-remote-read-remote-image.html
+http/tests/security/postMessage/invalid-origin-throws-exception.html
+http/tests/security/postMessage/target-origin.html
+http/tests/workers/shared-worker-redirect.html
+http/tests/workers/worker-redirect.html
+http/tests/xmlhttprequest/inject-header.html
+fast/forms/textarea-setvalue-submit.html

I don't know why, yet.

[Gustavo Noronha (kov)]

(In reply to comment #4)
> Is it ok I attach the patch that will update the test case to this bug? Is it
> possible we commit the patch but keeps this bug open until your patches are
> done?

Sure, go ahead!

[Chang Shu]

(In reply to comment #5)
> Also, it looks like these tests are failing when this test is run (if I skip
> it, the failures go away):
> 
> +fast/workers/worker-context-multi-port.html
> +fast/workers/worker-multi-port.html
> +fast/xmlhttprequest/xmlhttprequest-invalid-values.html
> +fast/xpath/attr-namespace.html
> +fast/xpath/detached-subtree-invalidate-iterator.html
> +fast/xpath/py-dom-xpath/nodetests.html
> +http/tests/misc/canvas-pattern-from-incremental-image.html
> +http/tests/security/canvas-remote-read-remote-image.html
> +http/tests/security/postMessage/invalid-origin-throws-exception.html
> +http/tests/security/postMessage/target-origin.html
> +http/tests/workers/shared-worker-redirect.html
> +http/tests/workers/worker-redirect.html
> +http/tests/xmlhttprequest/inject-header.html
> +fast/forms/textarea-setvalue-submit.html
> 
> I don't know why, yet.

Only thing I can think of is the Locale setting affects the following tests but it was supposed be reset... What will happen if you run them individually? And on what platform did you run? GTK? I ran a couple of above individually on qt-linux too and they all failed. I haven't tried if I reverted my change.

[Jan Alonzo]

(In reply to comment #3)
> 
> Let me quote the important part here:
> 
> Open a GTK-based browser and 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" or "en-us"

When I try to go to the site above, I get the expected "en-us" (LANG=en_US.UTF-8). When I switch it to LANG=pl_PL.UTF-8, I get "pl-pl", which is expected. 

(In reply to comment #0)
> Created an attachment (id=41288) [details]
> reimplementation of defaultLanguage
> 
> This test is failing because GTK+/Pango don't support changing locale after the
> first gtk_init() call, thus our defaultLanguage() implementation always returns
> whatever you had when you first started. I am not really sure about the patch.

I think the test case needs to be expanded to include multiple calls to setPOSIXLocale (which calls setlocale()) to be sure that defaultLanguage() is doing the right thing.

[Chang Shu]

Created attachment 41415 [details]
update test case to check against "en-us"

[Chang Shu]

(In reply to comment #8)
> When I try to go to the site above, I get the expected "en-us"
> (LANG=en_US.UTF-8). When I switch it to LANG=pl_PL.UTF-8, I get "pl-pl", which
> is expected. 
> 

Good that it worked as expected. Maybe the browser or the version I was running was out of date. Running WebKitTools/Scripts/run-webkit-tests could still cause trouble to GTK as the script does not pass LC_* or LANG envs to DumpRenderTree and GTK may set to default "C" and as you guys said, it cannot be updated later.

[Eric Seidel (no email)]

Comment on attachment 41415 [details]
update test case to check against "en-us"

I am confused by there being two unrelated patches on this bug.

This line is confusing:
 11 Language tags should follow the specification below:

What you're quoting is a piece of a spec, so you either need to provide a link to the original spec, or at least some description of where you got that snippet.  In this case it's from the RFC I mentioned.

Otherwise looks fine.  Won't this cause failures on qt or gtk?  OK to land w/ modifications.  If you need the commit-queue to land this, please post a new bug.

[Eric Seidel (no email)]

By "new bug" I meant "new patch".

[Chang Shu]

(In reply to comment #11)
> (From update of attachment 41415 [details])
> I am confused by there being two unrelated patches on this bug.
> 
> This line is confusing:
>  11 Language tags should follow the specification below:
> 
> What you're quoting is a piece of a spec, so you either need to provide a link
> to the original spec, or at least some description of where you got that
> snippet.  In this case it's from the RFC I mentioned.

I will load a new patch with link to RFC.
> 
> Otherwise looks fine.  Won't this cause failures on qt or gtk?  OK to land w/
> modifications.  If you need the commit-queue to land this, please post a new
> bug.

Qt should work as it returns "en-US" and then converted to "en-us" in the test case. GTK fails even before this change because of other issues and Gustavo's patch should fix this, right?

[Chang Shu]

Created attachment 41455 [details]
update description

[Eric Seidel (no email)]

Comment on attachment 41455 [details]
update description

LGTM.  I'm assuming you want this cq+ as well since you're not a committer yet.

[WebKit Commit Bot]

Comment on attachment 41455 [details]
update description

Clearing flags on attachment: 41455

Committed r49818: <http://trac.webkit.org/changeset/49818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

There's still my patch, so reopenning.

[Gustavo Noronha (kov)]

Comment on attachment 41415 [details]
update test case to check against "en-us"

Clearing review flag from committed patch.

[Eric Seidel (no email)]

Tiger is failing this test too:
http://build.webkit.org/results/Tiger%20Intel%20Release/r50133%20(5583)/results.html
I'll file a separate bug.

[Gustavo Noronha (kov)]

Created attachment 42026 [details]
reimplementation of defaultLanguage

[Xan Lopez]

Comment on attachment 42026 [details]
reimplementation of defaultLanguage

Looks good to me. r=me

[Evan Martin]

Comment on attachment 42026 [details]
reimplementation of defaultLanguage

> +    if (!localeDefault)
> +        return String("c");
> +
> +    GOwnPtr<gchar> normalizedDefault(g_ascii_strdown(localeDefault, -1));
> +    char* ptr = strchr(normalizedDefault.get(), '_');
> +
> +    if(ptr)
> +        *ptr = '-';

The spacing after "if" here doesn't match the spacing after the one above.

> +
> +    ptr = strchr(normalizedDefault.get(), '.');
> +
> +    if(ptr)
> +        *ptr = '\0';

This one too.

[Gustavo Noronha (kov)]

Landed as r50244.

[Gustavo Noronha (kov)]

(In reply to comment #23)
> (From update of attachment 42026 [details])
> > +    if (!localeDefault)
> > +        return String("c");
> > +
> > +    GOwnPtr<gchar> normalizedDefault(g_ascii_strdown(localeDefault, -1));
> > +    char* ptr = strchr(normalizedDefault.get(), '_');
> > +
> > +    if(ptr)
> > +        *ptr = '-';
> 
> The spacing after "if" here doesn't match the spacing after the one above.
> 
> > +
> > +    ptr = strchr(normalizedDefault.get(), '.');
> > +
> > +    if(ptr)
> > +        *ptr = '\0';
> 
> This one too.

Sorry, had not seen your comments! I'll make a new commit fixing these.

[Eric Seidel (no email)]

Comment on attachment 41288 [details]
reimplementation of defaultLanguage

Clearing r? on this patch since the bug is closed.