70678 - Clean up nsLocaleService::nsLocaleService string fu

Status: RESOLVED DUPLICATE of bug 195093

(Core :: Internationalization, defect)

Description

[Peter "jag" Annema]

For bug 64016 I ran into some ugly string code in
nsLocaleService::nsLocaleService. I fixed it, but didn't want to land it as part
of that bug (purity reasons), so here's a bug I can attach my fix to :-)

Comment 1

[Peter "jag" Annema]

Attachment: [patch] clean-up

Comment 2

[nhottanscp]

Thanks for the patch, let me reassign to you since you have a patch.
Cc to bstell, mkaply.
Brian, please review first half of the patch.
Mike, please reivew os/2 part of the change.

Comment 3

[Andreas Becker]

adding keyword intl and nsbeta1.

Comment 4

[kill this account]

please change the first locale fallback string (about line 237 in the source) 
from
  lang = "en-US";
to
  lang = "en_US";

which is the platform version of that locale.

Other than that it looks good.
For the unix half: r=bstell

Comment 5

[Mike Kaply [:mkaply]]

lc_temp is still allocated by the UniAPI, so it should use UniFreeMem. This 
looks like it was a bug in the original code.

     nsCRT::free(lc_temp);

should be

     UniFreeMem(lc_temp);

Also, should OS/2 be Releasing the converter in the NS_FAILED case, the same was 
as Unix?

Comment 6

[Peter "jag" Annema]

bstell:

Whoops, nice catch. So this is what the original code did:

-                    nsCAutoString langcstr("en-US");
-                    platformLocale.AssignWithConversion("en_US");

I missed the - vs _, I'll go fix my code :-)

mkaply:

Ah, okay, I understand now that different allocators were used. Hmmm, so much
for merging that code. Okay, I'll fix that. About releasing the converter in the
failed case, no, OS/2 is the only one using nsCOMPtrs currently, so it gets
automatically released when it goes out of scope.

New version coming up.

Comment 7

[Peter "jag" Annema]

mkaply: btw. in the original code nsCRT::free was used in case NS_FAILED(result)
and UniFreeMem otherwise. I guess the nsCRT::free there was incorrect.

Comment 8

[Peter "jag" Annema]

Attachment: [patch] fixed incorrect changes, use nsCOMPtr in XP_UNIX

Comment 9

[Mike Kaply [:mkaply]]

r=mkaply

Comment 10

[Peter "jag" Annema]

bstell: could you r= the new patch?

Who should I ask for r= of the Windows part?

Comment 11

[Jaime Rodriguez, Jr.]

jag - This looks like it is pretty close. can we get this by M0.9.1. If not, pls 
assign M0.9.2 milestone.

Comment 12

[Peter "jag" Annema]

Tentatively setting to 0.9.2

Comment 13

[Jaime Rodriguez, Jr.]

marking as nsbeta1-.

Comment 14

[kill this account]

It is very close to the beta release point.
Can we wait till after 0.9.1 ?

Comment 15

[Peter "jag" Annema]

This can wait till 0.9.2.

Comment 16

[Peter "jag" Annema]

Mass move to [e-mail]

Comment 17

[Peter Trudelle]

->0.9.3

Comment 18

[Jaime Rodriguez, Jr.]

Do we have a patch. Should we move this to TM0.9.4, and schedule it to be 
checked-in to the trunk?

Comment 19

[jag (Peter Annema)]

That patch has bit rotted. I will see if I get around to updating it for 0.9.3
or 0.9.4.

Comment 20

[Jaime Rodriguez, Jr.]

Thanks Jag

Comment 21

[jag (Peter Annema)]

-> 0.9.4

Comment 22

[saari (gone)]

->0.9.5

Comment 23

[Peter Trudelle]

Mass-moving lower-priority 0.9.5 bugs off to 0.9.6 to make way for remaining
0.9.4/eMojo bugs, and MachV planning, performance and feature work. If you
disagree with any of these targets, please let me know.

Comment 24

[jag (Peter Annema)]

->0.9.8

Comment 25

[jag (Peter Annema)]

If someone wants to take this patch and update it to the trunk, go for it.
-> 1.1

Comment 26

[Andreas Becker]

Changing QA contact to bobj for now. Bob, please re-assign further as you see is
appropriate.

Comment 27

[Vedran Miletic]

retargeting

Comment 28

[Jungshik Shin]

bug 195093 is a superset of this bug. I just compared my patch for bug 195093
with attachment 26694 [details] [diff] [review] and they're more or less similar (for nsLocalService.cpp.
my patch cleaned up other parts of intl/locale as well) although the patch here
is a bit more streamlined in one or two places.  I'm marking this as a dupe of
bug 195093 because I've got a patch (against the current cvs) that can go in as
soon as 1.4beta cycle begins (hopefully in a couple of days).


*** This bug has been marked as a duplicate of 195093 ***