592777 - nsLocaleService tries to use nsIGConfService before it is registered

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Pirkka Kärenlampi]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.17.8 (KHTML, like Gecko) Version/5.0.1 Safari/533.17.8
Build Identifier: 2.0b5pre, Maemo 6/Meego

nsLocaleService uses nsIGConfService to determine the system language in Meego, but at least with 2.0b5pre this fails because at the time the locale service is initialized, the gconf service has not yet been registered.

Reproducible: Always




It's possible to fix this dependency problem by manually moving components.manifest to come before interfaces.manifest in the top-level chrome.manifest. I don't know how to fix this in the code, and even if we could make that chrome.manifest change happen during the build, it seems like hacking a fix for some random symptom of a more serious problem. I filed this under intl because that's where the concrete problem is, but fixing this right might involve xpcom also. At least it's possible that this sort of problem exists also with other components or services.

Comment 1

[Pirkka Kärenlampi]

Some further information about my investigation into this problem: I tested a change to nsLocaleService.cpp so that it would try to get the gconf setting (just once) only when getSystemLocale or getApplicationLocale is called, but that change did not have the desired effect, i.e. something probably wants to call the locale getters during startup and before the gconf service is up.

Comment 2

[Pirkka Kärenlampi]

A possible solution could be to move the Meego-specific LANG environment variable handling from intl/locale/src/nsLocaleService.cpp into e.g. toolkit/xre/nsNativeAppSupportUnix.cpp and use gconf directly instead of through XPCOM. I will test that idea and provide a patch for feedback if it works.

Comment 3

[Pirkka Kärenlampi]

Attachment: Moves gconf language setting to be read in nsNativeAppSupportQt startup

This patch takes the /meegotouch/i18n/language gconf access out of nsLocaleService initialization and instead sets the LANG environment variable to that value in nsNativeAppSupportQt startup. Because we need to know the language before the XPCOM gconf service has been started, the access code has been in effect duplicated from toolkit/system/gnome/nsGConfService.cpp.

Comment 4

[Oleg Romashin (:romaxa)]

Comment on attachment 472971 [details] [diff] [review]
Moves gconf language setting to be read in nsNativeAppSupportQt startup

Do we have any Qt/Meegotouch API way to extract gconf values?

Comment 5

[Pirkka Kärenlampi]

(In reply to comment #4)
> Comment on attachment 472971 [details] [diff] [review]
> Moves gconf language setting to be read in nsNativeAppSupportQt startup
> 
> Do we have any Qt/Meegotouch API way to extract gconf values?

Good point. There's QLocale in Qt (a Meegotouch equivalent exists also) which could be used for various locale things, and we have a Meegotouch wrapper for gconf which could be used to read the locale (and I just realized we actually do have gconf settings also for collation, date/time, numeric and monetary formatting).

In addition to that I don't remember anymore why I thought it was necessary to move that code out of nsLocaleService.cpp.

I'll create a new patch for this using the gconf wrappers from where the code was originally.

Comment 6

[Pirkka Kärenlampi]

Attachment: New version of patch using GConf wrappers

I haven't been able to test this extensively yet (basic functionality works, i.e. system language is now detected) but here's an improved patch for feedback. This one uses the meegotouch gconf wrappers to read the locale settings.

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 473504 [details] [diff] [review]
New version of patch using GConf wrappers


>+ifeq ($(MOZ_WIDGET_TOOLKIT), qt)
>+CXXFLAGS	+= $(MOZ_QT_CFLAGS)
>+ifdef MOZ_ENABLE_MEEGOTOUCH
>+CXXFLAGS	+= $(MOZ_MEEGOTOUCH_CFLAGS)
>+endif
>+endif

we are including only mgconfitem.. do we relaly need $(MOZ_QT_CFLAGS) here?

> 
>+#ifdef MOZ_ENABLE_MEEGOTOUCH
>+static void copyGConfToEnv(const char* gconf, const char* env)
>+{
>+  MGConfItem item(gconf);
>+  QVariant value = item.value();
>+  if (QVariant::String == value.type()) {
>+    const std::string & s = value.toString().toStdString();
>+    setenv(env, s.c_str(), 1);

what is the point to create here std::string? can you just call "value.toString().data()" ?

Comment 8

[Pirkka Kärenlampi]

(In reply to comment #7)
> we are including only mgconfitem.. do we relaly need $(MOZ_QT_CFLAGS) here?

mgconfitem.h includes some Qt core headers, that's why we need it. I didn't find anything else than MOZ_WIDGET_TOOLKIT to compare against, although that isn't really clear from what I wrote. Actually the MOZ_MEEGOTOUCH_CFLAGS are apparently already in MOZ_QT_FLAGS, but that seems so counterintuitive that I wanted to do it some other way.

I'm not really satisfied with how that looks at the moment. So maybe just ifdef MOZ_ENABLE_MEEGOTOUCH and add a comment plus both to CXXFLAGS for clarity's sake?

> what is the point to create here std::string? can you just call
> "value.toString().data()" ?

QVariant::toString returns a QString, which I think still needs converting. I used the std::string ASCII conversion out of habit, another way would be e.g. "const char* s = value.toString().toAscii().constData();" which will then use a QByteArray as an intermediate step. I suppose that would be more pure Qt style, although I'd still need to check that some relevant object stays on the stack while using a pointer to it's internal structure in setenv. Or am I missing something?

Comment 9

[Pirkka Kärenlampi]

Attachment: Improved patch proposal after feedback

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 473994 [details] [diff] [review]
Improved patch proposal after feedback

>+#ifdef MOZ_ENABLE_MEEGOTOUCH
>+// It's necessary to include this before realloc gets macroed.
>+#  include <mgconfitem.h>
   ^^ - no spaces here.
>+#endif
>+

>+#ifdef MOZ_ENABLE_MEEGOTOUCH
>+static void copyGConfToEnv(const char* gconf, const char* env)

rename "copyGConfToEnv" to "CopyGConfToEnv"


>+{
>+  MGConfItem item(gconf);
>+  QVariant value = item.value();
>+  if (QVariant::String == value.type()) {
>+    const QByteArray & array = value.toString().toAscii();
                      ^ - no space here
>+    setenv(env, array.constData(), 1);
>"const char* s = value.toString().toAscii().constData();" which will then use a
>QByteArray as an intermediate step. I suppose that would be more pure Qt style,

Yep you right, I was looking into wrong place.
Please fix style to be 4 spaces


>+  } // else it's an incompatible type or QVariant::Invalid (not set)
>+}
>+#endif
>+

Fix this style issues and I think we are fine...

Comment 11

[Pirkka Kärenlampi]

Attachment: Style changes after review

nsLocaleService.cpp has a mix of styles to start with but I don't disagree with any of the proposed style changes, so here's a new patch.

Comment 12

[Oleg Romashin (:romaxa)]

Comment on attachment 475446 [details] [diff] [review]
Style changes after review

Looks pretty safe to me

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 475446 [details] [diff] [review]
Style changes after review

this is a=npodb..

Comment 14

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/c204e0000a90