635170 - DeCOM nsIWin32Locale

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Makoto Kato [:m_kato]]

nsIWin32Locale is only called from intl/* code.  We should be deDOM.

Comment 1

[Brandon Bohrer]

Attachment: First attempt at deCOMtaminating nsIWin32Locale

I'm trying to get started with Mozilla development so I took this on as a first bug. While I was at it I cleaned up some formatting issues.

There are a couple of things I wasn't sure about, such as the class naming and whether to make the functions virtual even if they won't be used with COM. I replaced nsIWin32Locale and nsIWin32LocaleImpl with nsWin32Locale. Should there still be an Impl class?  Please be picky - you won't scare me away.

One last note: I don't know yet whether my contract with my employer permits me to contribute to open-source projects. I'm currently researching this and don't think it will be a problem, but it's probably not a good idea to commit this until I have an answer.

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 513822 [details] [diff] [review]
First attempt at deCOMtaminating nsIWin32Locale

This class has no member variable, so all method should use static (or change to C function with namespace mozilla::intl?).

Comment 3

[Makoto Kato [:m_kato]]

> nsresult GetPlatformLocale(const nsAString& locale, LCID* winLCID); 
> nsresult GetXPLocale(LCID winLCID, nsAString& locale);

I think that this can define as static.

Comment 4

[Brandon Bohrer]

Are you sure? I made them non-static because these functions rely on localeNameToLCID and lcidToLocaleName, which are initialized in the constructor. Making them non-static was a quick and easy way to make sure that the necessary variables were initialized.

I could also initialize these variables by making the constructor static, but I read in the portability guidelines that static constructors are discouraged.

The third approach would be to initialize localeNameToLCID and lcidToLocaleName when GetPlatformLocale and GetXPLocale are called (and possibly make them local variables). 

Personally I prefer my approach, but I'm open to anything. Do you prefer one approach over another?

Comment 5

[Makoto Kato [:m_kato]]

> Are you sure? I made them non-static because these functions rely on
> localeNameToLCID and lcidToLocaleName, which are initialized in the
> constructor. Making them non-static was a quick and easy way to make sure that
> the necessary variables were initialized.

You should initialize function pointers at other point such as first call or startup.  I don't make sense that you cannot do it.

Also, kernel32.dll is already loaded by startup, so you should use GetModuleHandle instead of.  (LoadLibrary increments reference count)

Comment 6

[Brandon Bohrer]

Attachment: Made functions static

I made the functions static and initialized the pointers on first call. I also replaced LoadLibrary with GetModuleHandle (thanks for the tip), and made the constructor private so the class can't be instantiated. Let me know if you find any other problems.

Comment 7

[Makoto Kato [:m_kato]]

If possible, you should attach this by MQ patches format (https://developer.mozilla.org/en/Mercurial_queues).

Looks better, but...

>+  static LocaleNameToLCIDPtr localeNameToLCID;
>+  static LCIDToLocaleNamePtr lcidToLocaleName;

Why is it into public?  you should be into private or protected.

> +    LCID win_lcid = GetSystemDefaultLCID();
> +    if (win_lcid==0) { return;}
> +        rv = nsWin32Locale::GetXPLocale(win_lcid, xpLocale);
> +    if (NS_FAILED(rv)) { return;}
> +        rv = NewLocale(xpLocale, getter_AddRefs(mSystemLocale));
> +    if (NS_FAILED(rv)) { return;}

Why are indents different?

>+void
>+nsWin32Locale::initFunctionPointers(void)
>+{

This code always calls GetProcAddress every time.  You should use static variant such as the following.

nsWin32Locale::initFunctionPointers(void)
{
  static PRBool sInitialized = PR_FALSE;
  if (!sInitialized) {
    ...
    sInitialized = PR_TRUE;
  }
}

> +nsresult
> +nsWin32Locale::GetXPLocale(LCID winLCID, nsAString& locale)

This method always return NS_OK.  You should change to nsAString& nsWin32Locale::GetXPLocale(LCID winLCID).

Comment 8

[Brandon Bohrer]

Attachment: Misc. changes (V3)

I made the changes you requested (and used MQ format this time).

About the indentation: It was like that before and I must have forgotten to change it. I also noticed some other formatting problems in the affected files (tabs, for example). Should I go ahead and fix those?

About GetXPLocale:
Using locale as a parameter also made the memory management easier, so I left it as a parameter and made the function return void. Let me know if you see a better way.

Thanks.

Comment 9

[Simon Montagu :smontagu]

(In reply to comment #8)
> About the indentation: It was like that before and I must have forgotten to
> change it. I also noticed some other formatting problems in the affected files
> (tabs, for example). Should I go ahead and fix those?

Sure, go wild. But if you wouldn't mind, put the formatting fixes and the semantic fixes in separate patches. It makes it much easier to review.

Comment 10

[Brandon Bohrer]

(In reply to comment #9)
> Sure, go wild. But if you wouldn't mind, put the formatting fixes and the
> semantic fixes in separate patches. It makes it much easier to review.

Yeah, will do. 

Also, I got a response from the legal department where I work, and they don't have a problem with open-source projects. I figured as much, but it's good news anyway.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 514104 [details] [diff] [review]
Misc. changes (V3)

>diff --git a/intl/locale/src/nsLocaleConstructors.h b/intl/locale/src/nsLocaleConstructors.h

> #ifdef XP_WIN
>-#include "nsIwin32LocaleImpl.h"
>+#include "nsWin32Locale.h"

You don't need to include nsWin32Locale.h here

The patch doesn't build with shared linking, because intl/uconv and intl/locale are built as separate libraries, so the call to GetPlatformLocale in nsWinCharset.cpp is unresolved. I'm not sure, but maybe the cleanest solution to this is to move all of nsIPlatformCharset from intl/uconv to intl/locale (and you thought this was a nice small first bug!)

Comment 12

[Simon Montagu :smontagu]

Filed bug 637599

Comment 13

[Makoto Kato [:m_kato]]

Attachment: fix v4

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 525929 [details] [diff] [review]
fix v4

rebased and a few fix per review comment.

Comment 15

[Makoto Kato [:m_kato]]

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