568459 - read system locale from gconf

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

Extend the mechanism of reading the locale (for matchOS pref) to be able to read from GConf.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 451639 [details] [diff] [review]
patch

This feature is MAEMO6/MeeGo specific. The application framework there defines that gconf setting as authoritative.
It's not on other Linux systems.

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 451639 [details] [diff] [review]
patch

>+                if ( rv && !gconfLocaleString.IsEmpty() ) {

This should be |if ((NS_SUCCEEDED(rv)....|, no?

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 451639 [details] [diff] [review]
patch

oops, this was an older version. Will attach the correct one asap.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Attachment: correct patch

This is the current patch.
It's changing the order a bit compared to the previous (and already has the right if statement :-() The gconf setting is meant to override any other LC_ environment variables and is finally authoritative for the UI locale

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 451878 [details] [diff] [review]
correct patch

>+            if (lang == nsnull) {
>+                nsresult rv;
>+                nsCOMPtr<nsIGConfService> gconf =
>+                    do_GetService(NS_GCONFSERVICE_CONTRACTID, &rv);
>+                if (NS_SUCCEEDED(rv)) {
>+                    nsCAutoString gconfLocaleString;
>+                    rv = gconf->GetString(NS_LITERAL_CSTRING("/meegotouch/i18n/language"),
>+                                          gconfLocaleString);
>+                    if (NS_SUCCEEDED(rv) && !gconfLocaleString.IsEmpty()) {
>+                        lang = static_cast<const char*>PromiseFlatCString(gconfLocaleString).get();
>+                    }
>+                }
>+            }

...

>+                CopyASCIItoUTF16(lang, platformLocale);
>+                result = posixConverter->GetXPLocale(lang, xpLocale); 


I think it would be safer to use ToNewCString rather than get here, but I'd like a second opinion

Comment 7

[Benjamin Smedberg]

Comment on attachment 451878 [details] [diff] [review]
correct patch

>diff --git a/intl/locale/src/nsLocaleService.cpp b/intl/locale/src/nsLocaleService.cpp

>+#if (MOZ_PLATFORM_MAEMO == 6)
>+#  include "nsIGConfService.h"
>+#endif

==6, not >= 6?


>+            const char* lang = getenv("LANG");
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+            if (lang == nsnull) {
>+                nsresult rv;
>+                nsCOMPtr<nsIGConfService> gconf =
>+                    do_GetService(NS_GCONFSERVICE_CONTRACTID, &rv);
>+                if (NS_SUCCEEDED(rv)) {
>+                    nsCAutoString gconfLocaleString;
>+                    rv = gconf->GetString(NS_LITERAL_CSTRING("/meegotouch/i18n/language"),
>+                                          gconfLocaleString);
>+                    if (NS_SUCCEEDED(rv) && !gconfLocaleString.IsEmpty()) {
>+                        lang = static_cast<const char*>PromiseFlatCString(gconfLocaleString).get();
>+                    }
>+                }
>+            }

This is definitely not safe the PromiseFlatCString goes out of scope immediately, so you can't use the char* beyond its lifetime. Besides which, you don't actually need PromiseFlatCString because nsCAutoString is always flat to begin with. I suggest you hoist gconfLocaleString up to the same block as "lang", and then you can just set lang = gconfLocaleString.get().

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #2

> ==6, not >= 6?

Who knows but for now I'm fine (and even in favour) of using >=6.


From reading I see it going out of scope but then I'm wondering why it succeeded in every test we've made.
Did what you suggested in this one.

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

Bad timing. I need to make a tiny change which changes the priorities of settings. The platform also defines LANG now to a default value for other reasons. The GConf key needs to be used as primary source now and LANG as fallback only. Slightly changed patch coming up soon.

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #3

Slightly changed patch to use the gconf setting if available instead of preferring LANG.
That's needed as the platform in question changed a bit and LANG is now "misused" a bit.

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

Need that for Fennec2 -> asking blocking

Comment 12

[Jeremias Bosch (:jbos)]

Request push. Its +.

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

During another review I found that the patch does the right thing on the platform but doesn't do it correctly or "standard conform" enough (and not efficient asking gconf too many times).
I'm currently rewriting it a bit.

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #4

This moves the GConf access out of the for loop (also the getenv LANG which was there originally also).
For standard compliance I don't override LC_* settings anymore.

(Two not related whitespace fixes)

Comment 15

[Oleg Romashin (:romaxa)]

+                lang = gconfLocaleString.get();
this line cause 
intl/locale/src/nsLocaleService.cpp: In constructor 'nsLocaleService::nsLocalService()':
intl/locale/src/nsLocaleService.cpp:192: error: invalid conversion from 'const char*' to 'char*'

g++  4.3.3

Comment 16

[Wolfgang Rosenauer [:wolfiR]]

Yes, I removed the const by accident before submission while I tested with it :-(
Need to be considered when pushed.

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Fixed, cast problem, patch to push

-+                lang = gconfLocaleString.get();
++                lang = static_cast<const char*>(gconfLocaleString.get());
approval? autoapproval?

Comment 18

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/projects/electrolysis/rev/4084caac398f