195093 - 'modernize' nsILocale and nsIPlatformCharset

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[amardare]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

In the file intl/uconv/src/nsUNIXCharset.cpp, the metod 
ConvertLocaleToCharsetUsingDeprecatedConfig always returns 
NS_SUCCESS_USING_FALLBACK_LOCALE, even if there was an error.

It should return NS_ERROR_UCONV_NOCONV in case of error, since the return code 
is checked with NS_SUCCEEDED.

In the MOZILLA_1_2_BRANCH it used to properly return an error code. This is 
causing a problem in the mail/news client on our platform ( qnx photon ).

I attached a patch.
Here is also the cvs diff since it's very simple:
Index: intl/uconv/src/nsUNIXCharset.cpp
===================================================================
RCS file: /cvsroot/mozilla/intl/uconv/src/nsUNIXCharset.cpp,v
retrieving revision 1.37
diff -r1.37 nsUNIXCharset.cpp
127c127
<    return NS_SUCCESS_USING_FALLBACK_LOCALE;
---
>    return NS_ERROR_UCONV_NOCONV;

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[amardare]

Attachment: the patch is attached

Comment 2

[Boris Zbarsky [:bzbarsky]]

See bug 148026

Comment 3

[amardare]

I looked at the bug148026 and it seems that the original problems created a new 
problem.
From the comment on bug148026:
"In /nsUNIXCharset.cpp, if charset could not be achieved from nls_infl and 
locale name, we will default to iso-8859-1. The default value is fine, but in 
such case we should return a success code. Otherwise caller might be confused."

If you want to return a success code when we will default to iso-8859-1, then 
we should also put some value into the oResult.

The code below starts with oResult set to a null string and it stays a null 
string if you are "unable to convert locale to charset using deprecated config".

nsresult
nsPlatformCharset::ConvertLocaleToCharsetUsingDeprecatedConfig(nsAutoString& 
locale, nsAString& oResult)
{
.......

    res = gInfo_deprecated->Get(localeKey, oResult);
    if (NS_SUCCEEDED(res))  {
      return NS_OK;
    }
   }
   NS_ASSERTION(0, "unable to convert locale to charset using deprecated 
config");
   mCharset.Assign(NS_LITERAL_STRING("ISO-8859-1"));
   return NS_SUCCESS_USING_FALLBACK_LOCALE;
}

A place where this is causing problems is the file 
intl/locale/src/nsCollationUnix.cpp:

nsresult nsCollationUnix::Initialize(nsILocale* locale) 
{
<snip>
    nsCOMPtr <nsIPlatformCharset> platformCharset = do_GetService
(NS_PLATFORMCHARSET_CONTRACTID, &res);
    if (NS_SUCCEEDED(res)) {
      PRUnichar* mappedCharset = NULL;
      res = platformCharset->GetDefaultCharsetForLocale(aLocale.get(), 
&mappedCharset);
      if (NS_SUCCEEDED(res) && mappedCharset) {
        mCharset = mappedCharset;
^^^^^^^^ here mCharset is set to a null string^^^^^^^
        nsMemory::Free(mappedCharset);
      }
    }
  }
<snip>
}

The mCharset is assigned the oResult value from GetDefaultCharsetForLocale(). 
But the oResult is not set to any value, is left untouched. This was not 
happening before since an error code was returned by GetDefaultCharsetForLocale.
This is causing a crashing on our platform, in the mail/news client, when 
the "create new account wizard" finishes.
If a succes code is returned, then why not update the oResult?

Comment 4

[Jungshik Shin]

While looking into this bug, I found that implementations of
|nsIPlatformCharset| use an archaic style. It looks like they're written in the
'pre-string class era' (or in the very early stage of string class development)
 I'm gonna upload a patch that gets rid of alloc/free. 

I also have a patch that does the same with |nsILocale| : |GetCategory|, but I
want to fix |nsIPlatformCharset| first. 

Comment 5

[Jungshik Shin]

Attachment: a patch to fix GetDefaultCharsetForLocale

Comment 6

[Jungshik Shin]

A more extensive patch (GetCategory) is coming, but let me do this in steps.
(the second batch is over 1000 lines long)
 
Boris, can you take a look at the current patch? If you like it, can you sr it?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Not till sometime later this weekend (possibly as late as Tuesday).

Comment 8

[Darin Fisher]

Comment on attachment 127993 [details] [diff] [review]
a patch to fix GetDefaultCharsetForLocale

>Index: intl/locale/src/mac/nsCollationMac.cpp

>     if (NS_SUCCEEDED(res)) {
>+      nsCAutoString mappedCharset;
>+      res = platformCharset->GetDefaultCharsetForLocale(aLocale.get(), mappedCharset);
>+      if (NS_SUCCEEDED(res))
>+        res = mCollation->SetCharset(mappedCharset.get());
>       }
>     }

funny bracketing here.	probably won't compile on mac?


>Index: intl/uconv/src/nsMacCharset.cpp

>+nsPlatformCharset::GetDefaultCharsetForLocale(const PRUnichar* localeName, nsACString &oResult)
...
>+    nsAutoString localeAsString(localeName);

there is a way to initialize a nsAutoString that avoids a string copy (see
CBufDescriptor);
however, i'm not sure it's really worth it here.  probably would be better to
fix the locale
interface to not require a |const nsString*|.  but, neither of these changes
are obviously
required for this patch :)


>Index: intl/uconv/src/nsUNIXCharset.cpp

>+  nsAutoString charset;
>   nsresult res = ConvertLocaleToCharsetUsingDeprecatedConfig(localeStr, charset);
>   if (NS_SUCCEEDED(res)) {
>+    LossyCopyUTF16toASCII(charset, oResult); // charset name is always ASCII.

so, at some point it'd be nice to make
ConvertLocaleToCharsetUsingDeprecatedConfig
work with |nsACString&| instead ;-)


so, just make sure this doesn't break the mac, and you've got r=darin

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127993 [details] [diff] [review]
a patch to fix GetDefaultCharsetForLocale

>Index: intl/uconv/src/nsMacCharset.cpp

>+  nsresult rv = NS_SUCCESS_USING_FALLBACK_LOCALE; 

Why bother assigning to rv here?  It's always going to be clobbered by the
do_CreateInstance call....

>Index: intl/uconv/src/nsOS2Charset.cpp
> NS_IMETHODIMP
>-nsPlatformCharset::GetDefaultCharsetForLocale(const PRUnichar* localeName, PRUnichar** _retValue)
>+nsPlatformCharset::GetDefaultCharsetForLocale(const PRUnichar* localeName, nsACString &aResult)
> {
>   return NS_OK;
> }

Shouldn't this at least call Truncate() on aResult or something?  I know you
did not really change this code, but....

>Index: intl/uconv/src/nsUNIXCharset.cpp
>   nsresult res = ConvertLocaleToCharsetUsingDeprecatedConfig(localeStr, charset);
>   if (NS_SUCCEEDED(res)) {
>-    *_retValue = ToNewUnicode(charset);
>+    LossyCopyUTF16toASCII(charset, oResult); // charset name is always ASCII.

Any chance to modernize ConvertLocaleToCharsetUsingDeprecatedConfig too?

>Index: intl/uconv/src/nsWinCharset.cpp
>+  nsresult rv;
>+  winLocale = do_CreateInstance(NS_WIN32LOCALE_CONTRACTID, &rv);
>+  rv = winLocale->GetPlatformLocale(&localeAsNSString,&localeAsLCID);

Again, I know that you did not change the code, but what if the
do_CreateInstance fails?

>+  rv = MapToCharset(acp_key,oResult);
> 
>-  *_retValue = ToNewUnicode(charset);
>-  return result;
>+  return rv;

How about |return MapToCharset(acp_key, oResult);|?  And yes, there should be a
space after that comma.

>Index: intl/uconv/tests/plattest.cpp
>+	PRUnichar*						category_value;
>+    nsCAutoString                   charset;
>+	nsString						categoryAsNSString;

Indentation weirdness. Tabs/spaces?

>+	rv = platform_charset->GetDefaultCharsetForLocale(categoryAsNSString.get(),charset);

Space after comma?

Is there a reason not to make the locale arg an nsAString?  All the callers are
calling .get() on nsStrings to call this method, and all the callees are
assigning from the PRUnichar* into an nsAutoString....

Comment 10

[Jungshik Shin]

Attachment: a new patch

Thank you for review. I addressed Darin's and Boris' concerns other than
further modernizing nsILocale and nsIPlatformCharset that I'm planning to do
after landing this. 

> Is there a reason not to make the locale arg an nsAString?
...
> ConvertLocaleToCharsetUsingDeprecatedConfig
> work with |nsACString&| instead ;-)
..
> would be better to fix the locale interface to not require a |const
nsString*|

And there were a couple of other comments on further modernizing nsILocale and
nsIPlatformCharset. Yes, I agree that we need to do that. Actually, I've
already made that patch (in comment #6, I was talking about it although not
very clearly), but I wanted to land this first because the second batch
involves a lot more extensive changes on Mac, OS/2 and BeOS to which I don't
have access.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128239 [details] [diff] [review]
a new patch 

sr=me, but please actually fix the indentation weirdness in plattest.cpp before
checking in (the same weirdness I commented on before).

Comment 12

[Darin Fisher]

Comment on attachment 128239 [details] [diff] [review]
a new patch 

r=darin (with same plattest.cpp indentation nit)

Comment 13

[Jungshik Shin]

Attachment: modernize nsILocale and friends

I didn't expect this to become a beast. There are still some more changes I
want to make in intl/locale, but it's already too big. 

I need some help for testing on MacOS, OS2 and BeOS. If there's a problem for
those platforms, it's more likely to be in what I didn't change (because I
overlooked them) than in what I changed.

Comment 14

[Jungshik Shin]

Attachment: firebird patch

Comment 15

[Jungshik Shin]

I have yet to update intl/locale/tests/LocaleSelfTest.cpp and
intl/locale/tests/nsLocaleTest.cpp. Neither of them is built (intl/locale/tests
is not listed in intl/locale/Makesfile.in and intl/locale/tests/Makefile.in is
broken). So, I guess we can just land attachment 128569 [details] [diff] [review] (after r/sr and testing
on Mac/OS2/BeOS) without fixing two test files. Nonetheless, I'll fix them
(nobody seems to have  touched them for ages).

BTW, I was tempted to use ASCII strings for 'locale names' and 'category names'
instead of UTF16, but I just left them alone for the moment because I'm not sure
of the reasoning behind the decision to use UTF16 for them. Simon, do you know
the story behind it? 

Comment 16

[Simon Montagu :smontagu]

No I don't. In fact I filed bug 181520 to change it some time ago, and even
started working on a patch, but it quickly became huge and hard to keep up with.

Comment 17

[Jungshik Shin]

Attachment: updated patch

This was tested on both Win32 and Linux and is ready for review. It's over 3000
lines (partly because I used '-u7' instead of '-u3') long, but most of changes
are  straightforward. 

It has to be tested on MacOS, OS/2 and BeOS. Launching Mozilla under non-en-US
locales and browsing a few non-English pages, reading emails (to test collation
and timedate format)  and opening a download manager would be sufficient. I'll
ask timeless for help with BeOS. darin, can you test it on MacOS?

Comment 18

[Jungshik Shin]

Hmm. my bugzilla mail setting may have been screwed up. I haven't received
Simon's comment #16 about bug 181520. I'm not sure which is better, to just
going ahead with this patch for the now and working on converting 'locale names'
to nsACString later (in bug 181520) or doing it in a single step.  I'm slightly
inclined toward the former. 

Simon, Darin and Boris, if you think it's a good idea to do this in two steps,
can you review my patch? It's rather large and ...

Otherwise, I'll convert localenames to nsACString. How about category names? I
don't see any reason they have to be Unicode strings, either.
 

Comment 19

[Jungshik Shin]

Comment on attachment 128758 [details] [diff] [review]
updated patch

Asking darin and bz (the duo who r/sr'd the first patch) for r/sr. smontague,
please feel free to pick it up if you like.

BTW, I've just tried to replace nsAString (for localeName) with nsACString in a
few files, but the ripple effect is too extensive. So, I decided to do it
later.

There are a few strange indentations in the patch. intl/locale/ has  many files
with tabs... I tried to leave them alone (because I don't want to make lxr
'blame' useless). 

nsOS2Locale.cpp/h has 'nsAutoString' in the function signature, I meant to use
nsAString. That was fixed on my tree.

Another btw, I posted to BeOS, OS2 and Mac(OSX) groups to ask for help with
ports.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this before I leave town; after that it will be weeks
before I have a net connection, much less am in a position to review patches.

Comment 21

[Simon Montagu :smontagu]

Assigning bug to jshin, which should help him get bugmail from it ;-)

Comment 22

[Jungshik Shin]

Comment on attachment 128758 [details] [diff] [review]
updated patch

Boris, thanks for the note.
Simon, thansks for the reassignment. now I'm returning the favor by asking for
r :-). It's huge (larger than it would be because I used '-u7' in diff.), but
most changes are rather 'mechanical'...

Comment 23

[Darin Fisher]

Comment on attachment 128758 [details] [diff] [review]
updated patch

>Index: intl/locale/idl/nsILocale.idl

> [scriptable, uuid(21035ee0-4556-11d3-91cd-00105aa3f7dc)]
> interface nsILocale : nsISupports {
> 	
>+	AString GetCategory (in AString category);
> };

maybe change "GetCategory" to "getCategory" ... be sure this doesn't break any
JS consumers.


>Index: intl/locale/idl/nsILocaleService.idl

> [scriptable, uuid(7c094410-4558-11d3-91cd-00105aa3f7dc)]
> interface nsILocaleDefinition : nsISupports {
>-	void SetLocaleCategory(in wstring category, in wstring value);
>+	void SetLocaleCategory(in AString category, in AString value);
> };

> [scriptable, uuid(48ab1fa0-4550-11d3-91cd-00105aa3f7dc)]
> interface nsILocaleService : nsISupports {
> 
>+	nsILocale NewLocale(in AString aLocale);
> 	nsILocale NewLocaleObject(in nsILocaleDefinition localeDefinition);
> 	nsILocale GetSystemLocale();
> 	nsILocale GetApplicationLocale();
> 	nsILocale GetLocaleFromAcceptLanguage(in string acceptLanguage);
>+	AString GetLocaleComponentForUserAgent();
> };

same interCaps nit applies to all these interfaces.  feel free to punt on
this correction if you want, but i think ultimately these interfaces should
use the interCaps standard convention.


>Index: intl/locale/src/nsLocale.cpp

> NS_IMETHODIMP
>-nsLocale::GetCategory(const nsString* category,nsString* result)
>+nsLocale::GetCategory(const nsAString& category, nsAString& result)
> {
>+	const nsString key(category);

maybe |const nsAutoString key(category);| instead.


>+	const nsString *value = (const nsString*) PL_HashTableLookup(fHashtable, &key);

would be nice to fix this hash table to not require |nsString*| as the key.

>+nsLocale::AddCategory(const nsAString &category, const nsAString &value)
> {
> 	nsString* new_key = new nsString(category);
>+	NS_ENSURE_TRUE(new_key, NS_ERROR_OUT_OF_MEMORY);
> 	
> 	nsString* new_value = new nsString(value);
>+	NS_ENSURE_TRUE(new_value, NS_ERROR_OUT_OF_MEMORY);

hmm... notice that if you hit this second early return you will leak |new_key|?
looks like a bug in the existing code, but would be good to fix it while you're
here.


> nsLocale::Hash_HashFunction(const void* key)
> {
>+	const PRUnichar*	ptr = (*((nsString *) key)).get();

nit:
	const PRUnichar*	ptr = ((const nsString *) key)->get();


> PRIntn
> nsLocale::Hash_CompareNSString(const void* s1, const void* s2)
> {
>+	return (*((nsString *) s1)).Equals((*(nsString *) s2));

nit:
	return ((const nsString *) s1)->Equals(*(const nsString *) s2);


>+nsLocale::Hash_EnumerateDelete(PLHashEntry *he, PRIntn hashIndex, void *arg)
> {
>+	delete ((nsString*)he->key);
>+	delete ((nsString*)he->value);

nit: avoid gratuitous parentheses...

	delete (nsString *) he->key;
	delete (nsString *) he->value;

notice that it would have been simpler to just make the key and value be
straight |PRUnichar*|.	bonus points if you eliminate the |nsString*|'s here!
;-)



> PRIntn
> nsLocale::Hash_EnumerateCopy(PLHashEntry *he, PRIntn hashIndex, void* arg)
> {
>+	nsString* new_key = new nsString(*((nsString*)he->key));
> 	if (!new_key)
> 		return HT_ENUMERATE_STOP;
> 
>+	nsString* new_value = new nsString(*((nsString*)he->value));
> 	if (!new_value)
> 		return HT_ENUMERATE_STOP;

this early return leaks |new_key|.


>Index: intl/locale/src/nsLocaleService.cpp

>                 if ( lang == nsnull ) {
>+                    result = os2Converter->GetXPLocale("en-US", xpLocale);
>+		}
>                 else
>+                    result = os2Converter->GetXPLocale(lang, xpLocale); 

fix brack indentation.	also, seems like this block of code mixed
2 space indenting with 4 space indenting.  maybe you can unify it?


>+nsLocaleService::GetLocaleComponentForUserAgent(nsAString& retval)
> {
...
> 	if (NS_SUCCEEDED(result))
> 	{
>-		nsString	lc_messages; lc_messages.AssignWithConversion(NSILOCALE_MESSAGE);
>-		result = system_locale->GetCategory(lc_messages.get(),_retval);
>+		result = system_locale->
>+            GetCategory(NS_LITERAL_STRING(NSILOCALE_MESSAGE), retval);

nit: wacked indentation.


>Index: intl/locale/src/mac/nsMacLocale.cpp

>+		for(i=0;strlen(country_list[i].iso_code)!=0;i++) {

no need to call strlen here.  just check if first byte is not '\0'


>+	for(i=0;strlen(lang_list[i].iso_code)!=0;i++) {

same deal here.


> 	for(i=0;strlen(lang_list[i].iso_code)!=0;i++) {

pattern repeats.... fix all.

> 	for(i=0;strlen(country_list[i].iso_code)!=0;i++) {


>+			// XXX : perhaps, we need AppendASCIItoUTF16
>+			AppendUTF8toUTF16(nsDependentCString(country_list[i].iso_code), locale);

yeah, that might be a nice optimization.


>Index: intl/locale/src/os2/nsOS2Locale.cpp

>+nsOS2Locale::GetXPLocale(const char* os2Locale, nsAutoString& locale)

why is the argument |nsAutoString&|?  maybe |nsString&| would be better.


>+      locale.AssignWithConversion(os2Locale);  // use os2 if parse failed

use CopyASCIItoUTF16 instead?


>+    locale.AssignWithConversion(os2_locale);

same here.


>Index: intl/locale/src/unix/nsCollationUnix.cpp

>+  nsAutoString aCategory(NS_LITERAL_STRING("NSILOCALE_COLLATE##PLATFORM"));

does this need to be a string copy?  can you use NS_NAMED_LITERAL_STRING
instead?

    NS_NAMED_LITERAL_STRING(aCategory, "NSILOCALE_COLLATE##PLATFORM");


>+//  if (localeStr.Equals("en_US", nsCaseInsensitiveStringComparator())) { // note: locale is in platform format

nit: kill commented code??



>Index: intl/locale/src/unix/nsDateTimeFormatUnix.cpp

>+  nsAutoString aCategory(NS_LITERAL_STRING("NSILOCALE_TIME##PLATFORM"));

same thing here.


>Index: intl/locale/src/windows/nsCollationWin.cpp

>   mCollation = new nsCollation;
>-  if (mCollation == NULL) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>+  NS_ENSURE_TRUE(mCollation != nsnull, NS_ERROR_OUT_OF_MEMORY);

hmm... if allocation fails, then probably we won't be able to
printf either.	i don't usually print warnings for out of memory
errors for this reason.


>Index: intl/locale/src/windows/nsIWin32LocaleImpl.cpp

> 			for(j=0;strlen(iso_list[i].sublang_list[j].iso_code)!=0;j++) {

nit: more fun pointless strlen calls...

Comment 24

[Jungshik Shin]

Thanks, Darin, for your thorough review.  
 
I must have been too mechanical to catch that :-) 
 
 >  for(j=0;strlen(iso_list[i].sublang_list[j].iso_code)!=0;j++) { 
 
> no need to call strlen here.  just check if first byte is not '\0' 
 
 How about chaging 'sentinel' of the array to 'NULL' (from "") and checking if the pointer 
is null?   
 
const iso_lang_map lang_list[] = { 
............ 
        { "", 0, 0} 
}; 
 
I don't see any reason that doesn't work. However, I'm wondering if there's anything I 
don't know about  MacOS compiler (that makes it break). The portability guide doesn't 
mention it, but...   
 
BTW, some files I'm touching have a lot of tabs which is why I have some funny 
indentations.  Do you mind if I clean them up in functions I'm modifying (fixing them all 
file-wide  would  make 'cvs blame' useless). Hmm, in that case,  my patch would be 
hard to read even with diff '-w'....  

Comment 25

[Darin Fisher]

>How about chaging 'sentinel' of the array to 'NULL' (from "") and checking if the 
>pointer is null?

sounds even better!  i don't think you will have a portability problem, but i
could be wrong.

sucks about the whitespace being all screwed up in that file... maybe just
cleanup the individual functions, or bite-the-bullet and take the cvs blame! ;-)
  .... your call.

Comment 26

[Paul]

I will test the latest patch for BeOS.  Is the last patch from 7/28/03 the only
one I need to apply?

Comment 27

[Paul]

Comment on attachment 128758 [details] [diff] [review]
updated patch

Tested under BeOS.  Compiled and did not run into any immediate problems.

Comment 28

[Jungshik Shin]

Thanks a lot for testing. Did you test 'date/time format' (in download
maanger/mailnews/bookmark) or sorting in addressbook? If it gets compiled fine
and run fine, it should be all right but just in case...

Anyway, I made another patch based on Darin's review comment, but I guess the
changes made don't affect BeOS port. I'm a bit tied up so that it'll take a
while to upload after testing on Linux and Windows. Another test (when I have a
new patch) would be nice.

Comment 29

[Jungshik Shin]

Attachment: a new patch addressing darin's concerns

I fixed idl files to get them compliant to our intercap convention. xml/js
files were modified accordingly. The locale hash (in nsILocale) now uses
PRUnichar* as both keys and values instead of nsString*.  Embedded tabs are
removed 'blockwise'. For review, perhaps I have to upload a patch identical to
this except for spacing (diff -w).

This patch also includes the patch for firebird, but I'm gonna upload it
separately because I can't commit to the firebird partition.

Comment 30

[Jungshik Shin]

Attachment: patch for review (diff -w)

This is the same as attachment 129852 [details] [diff] [review], but was made with 'diff -w'. I'm not
sure this is easier for reviwers' than attachment 129852 [details] [diff] [review].

Comment 31

[Jungshik Shin]

Comment on attachment 129854 [details] [diff] [review]
patch for review (diff -w)

asking for r/sr.
This patch(diff -w) may be harder to read than attachment 129852 [details] [diff] [review]. In that case,
pls review attachment 129852 [details] [diff] [review], instead. They're identical other than 'white
spaces'.

Comment 32

[Darin Fisher]

Comment on attachment 129854 [details] [diff] [review]
patch for review (diff -w)

>Index: intl/locale/src/nsLocale.cpp


>+  (void) PL_HashTableAdd(fHashtable, newKey, newValue);

presumably this never fails??  hmm... seems like it could fail if
there is not enough memory.  in which case it seems like we'd leak
newKey and newValue :(


> PRIntn
> nsLocale::Hash_CompareNSString(const void* s1, const void* s2)
> {
>+  return nsDependentString((const PRUnichar *) s1).
>+                           Equals(nsDependentString((const PRUnichar *) s2));

nit: use nsCRT::strcmp instead.  the result will be much smaller
code size (no virtual destructor calls) and probably a bit faster
because it won't have to compute the string length for each string.


>Index: intl/locale/src/nsScriptableDateFormat.cpp

>+  nsCOMPtr<nsILocale> aLocale = 0; 

nit: no need to initialize a nsCOMPtr to null.	it starts that way by default.


>Index: intl/locale/src/mac/nsMacLocale.cpp

>-			for(i=0;strlen(country_list[i].iso_code)!=0;i++) {
>+    for(i=0;!country_list[i].iso_code;i++) {

hmm... this looks wrong to me.	your checking if the character pointer
is NULL, and you are only entering the loop if it is NULL.  and then
you are passing that pointer to |strcmp|... that is going to crash.
i think you want this instead:

      for (i=0; country_list[i].iso_code; i++) {

same thing repeats elsewhere.


>Index: intl/locale/src/windows/nsIWin32LocaleImpl.cpp

>+      for(j=0;!iso_list[i].sublang_list[j].win_code;j++) {

same problem here too...


sr=darin with these things fixed.

Comment 33

[Jungshik Shin]

Attachment: patch with darin's concerned taken care of (diff -w)

Thanks, darin for review. I don't know what I was thinking using
'!...iso_code[j]' instead of just '....iso_code[j]'. I fixed up all your
concerns. Otherwise, this patch is identical to attachment 129854 [details] [diff] [review]

smontagu, can you review this patch? darin, can you put sr on it?

Comment 34

[Darin Fisher]

Comment on attachment 130214 [details] [diff] [review]
patch with darin's concerned taken care of (diff -w) 

sr=darin

Comment 35

[Jungshik Shin]

ping smontagu :-). can you review? let's move this forward.

Comment 36

[Simon Montagu :smontagu]

Comment on attachment 130214 [details] [diff] [review]
patch with darin's concerned taken care of (diff -w) 

r=smontagu. Every time I look at this code I see tons of other things that I
would like to see changed, but I would rather move forward incrementally than
bog you down at this point, especially after taking so long to review, for
which I apologise.

Comment 37

[Jungshik Shin]

thanks for r, smontagu. I'm gonna land it once 1.5beta cycle begins because I 
don't have a strong case to get this into 1.5alpha. 

I, too, see more things to change (e.g. using nsACString for localename which 
is always ASCII and avoid LossyConvertUTF16toASCII. see comment #19), but this 
patch is already rather large.  

Comment 38

[Jungshik Shin]

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

Comment 39

[Jungshik Shin]

Attachment: firebird patch

brian (or anyone with the previl. to commit to firebird), can you land this?
I've already committed attachment 130214 [details] [diff] [review] so that firebird would not build/pass
smoketest without this in. 
Thanks.

Comment 40

[Jan Varga [:janv]]

Comment on attachment 134458 [details] [diff] [review]
firebird patch

checked in

Comment 41

[Jungshik Shin]

thanks all, patches (plus a few more lines that need to be changed to fix
bustage on Mac and OS2) landed in the trunk

Comment 42

[Steffen Wilberg]

This regressed bug 224222: OK and Cancel buttons gone.

Comment 43

[Scott MacGregor]

do we need a thunderbird patch too? I think we might.

Comment 44

[Jungshik Shin]

toolkit/ is shared by firebird and thunderbird, isn't it? I couldn't find any
place in mail/ where I need to change (nsILocale or GetApplicationLocale). What
are the thunderbird-equivalent of contentAreaUtils.js/dialog.xml? 

Comment 45

[Scott MacGregor]

yeah thunderbird uses mozilla/toolkit. I did a search and came up empty so it
looks like we are in the clear. Thanks for double checking for me.