Last Comment Bug 635170 - DeCOM nsIWin32Locale
: DeCOM nsIWin32Locale
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All Windows Vista
: -- normal (vote)
: mozilla6
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on: 637599
Blocks: deCOM 181515
  Show dependency treegraph
 
Reported: 2011-02-18 00:37 PST by Makoto Kato [:m_kato]
Modified: 2011-05-08 23:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First attempt at deCOMtaminating nsIWin32Locale (63.06 KB, patch)
2011-02-19 17:20 PST, Brandon Bohrer
m_kato: review-
Details | Diff | Splinter Review
Made functions static (62.81 KB, patch)
2011-02-19 23:32 PST, Brandon Bohrer
no flags Details | Diff | Splinter Review
Misc. changes (V3) (52.16 KB, patch)
2011-02-21 17:16 PST, Brandon Bohrer
smontagu: review-
Details | Diff | Splinter Review
fix v4 (23.61 KB, patch)
2011-04-13 21:34 PDT, Makoto Kato [:m_kato]
smontagu: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2011-02-18 00:37:19 PST
nsIWin32Locale is only called from intl/* code.  We should be deDOM.
Comment 1 Brandon Bohrer 2011-02-19 17:20:28 PST
Created attachment 513822 [details] [diff] [review]
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] 2011-02-19 20:41:25 PST
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] 2011-02-19 20:43:36 PST
> 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 2011-02-19 21:19:21 PST
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] 2011-02-19 21:49:58 PST
> 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 2011-02-19 23:32:11 PST
Created attachment 513834 [details] [diff] [review]
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] 2011-02-20 04:31:40 PST
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 2011-02-21 17:16:46 PST
Created attachment 514104 [details] [diff] [review]
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 2011-02-28 02:59:32 PST
(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 2011-02-28 19:08:30 PST
(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 2011-03-01 00:54:29 PST
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 2011-03-01 01:50:32 PST
Filed bug 637599
Comment 13 Makoto Kato [:m_kato] 2011-04-13 21:34:51 PDT
Created attachment 525929 [details] [diff] [review]
fix v4
Comment 14 Makoto Kato [:m_kato] 2011-04-13 21:36:18 PDT
Comment on attachment 525929 [details] [diff] [review]
fix v4

rebased and a few fix per review comment.
Comment 15 Makoto Kato [:m_kato] 2011-05-02 02:03:07 PDT
http://hg.mozilla.org/mozilla-central/rev/a45b194f7d11

Note You need to log in before you can comment on or make changes to this bug.