Bug 30440 - [GTK] Fails new test fast/js/navigator-language.html
Summary: [GTK] Fails new test fast/js/navigator-language.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
: 30439 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-10-16 09:04 PDT by Gustavo Noronha (kov)
Modified: 2009-10-29 12:11 PDT (History)
6 users (show)

See Also:


Attachments
reimplementation of defaultLanguage (2.48 KB, patch)
2009-10-16 09:04 PDT, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
update test case to check against "en-us" (3.13 KB, patch)
2009-10-19 06:41 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
update description (3.16 KB, patch)
2009-10-19 14:46 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
reimplementation of defaultLanguage (3.34 KB, patch)
2009-10-28 07:14 PDT, Gustavo Noronha (kov)
xan.lopez: review+
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-10-16 09:04:31 PDT
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.
Comment 1 Chang Shu 2009-10-16 09:12:20 PDT
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.
Comment 2 Gustavo Noronha (kov) 2009-10-16 14:17:20 PDT
*** Bug 30439 has been marked as a duplicate of this bug. ***
Comment 3 Gustavo Noronha (kov) 2009-10-16 14:21:13 PDT
(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"
Comment 4 Chang Shu 2009-10-16 14:25:32 PDT
(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?
Comment 5 Gustavo Noronha (kov) 2009-10-16 15:06:10 PDT
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.
Comment 6 Gustavo Noronha (kov) 2009-10-16 15:06:38 PDT
(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!
Comment 7 Chang Shu 2009-10-16 18:20:12 PDT
(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.
Comment 8 Jan Alonzo 2009-10-17 19:18:20 PDT
(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.
Comment 9 Chang Shu 2009-10-19 06:41:42 PDT
Created attachment 41415 [details]
update test case to check against "en-us"
Comment 10 Chang Shu 2009-10-19 06:56:24 PDT
(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.
Comment 11 Eric Seidel (no email) 2009-10-19 14:16:47 PDT
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.
Comment 12 Eric Seidel (no email) 2009-10-19 14:17:17 PDT
By "new bug" I meant "new patch".
Comment 13 Chang Shu 2009-10-19 14:32:35 PDT
(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?
Comment 14 Chang Shu 2009-10-19 14:46:24 PDT
Created attachment 41455 [details]
update description
Comment 15 Eric Seidel (no email) 2009-10-19 15:23:56 PDT
Comment on attachment 41455 [details]
update description

LGTM.  I'm assuming you want this cq+ as well since you're not a committer yet.
Comment 16 WebKit Commit Bot 2009-10-19 15:36:39 PDT
Comment on attachment 41455 [details]
update description

Clearing flags on attachment: 41455

Committed r49818: <http://trac.webkit.org/changeset/49818>
Comment 17 WebKit Commit Bot 2009-10-19 15:36:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Gustavo Noronha (kov) 2009-10-21 02:33:21 PDT
There's still my patch, so reopenning.
Comment 19 Gustavo Noronha (kov) 2009-10-21 02:34:04 PDT
Comment on attachment 41415 [details]
update test case to check against "en-us"

Clearing review flag from committed patch.
Comment 20 Eric Seidel (no email) 2009-10-27 02:49:03 PDT
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.
Comment 21 Gustavo Noronha (kov) 2009-10-28 07:14:43 PDT
Created attachment 42026 [details]
reimplementation of defaultLanguage
Comment 22 Xan Lopez 2009-10-28 08:17:19 PDT
Comment on attachment 42026 [details]
reimplementation of defaultLanguage

Looks good to me. r=me
Comment 23 Evan Martin 2009-10-28 08:20:04 PDT
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.
Comment 24 Gustavo Noronha (kov) 2009-10-28 15:44:26 PDT
Landed as r50244.
Comment 25 Gustavo Noronha (kov) 2009-10-29 05:45:48 PDT
(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.
Comment 26 Eric Seidel (no email) 2009-10-29 12:11:56 PDT
Comment on attachment 41288 [details]
reimplementation of defaultLanguage

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