Note: There are a few cases of duplicates in user autocompletion which are being worked on.

DeCOM nsIWin32Locale

RESOLVED FIXED in mozilla6

Status

()

Core
Internationalization
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
All
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
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.
Attachment #513822 - Flags: review?(m_kato)
(Assignee)

Comment 2

7 years ago
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?).
Attachment #513822 - Flags: review?(m_kato) → review-
(Assignee)

Comment 3

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

I think that this can define as static.

Comment 4

7 years ago
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?
(Assignee)

Comment 5

7 years ago
> 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

7 years ago
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.
Attachment #513822 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
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

7 years ago
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.
Attachment #513834 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #514104 - Flags: review?(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

7 years ago
(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 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!)
Attachment #514104 - Flags: review?(smontagu) → review-
Depends on: 637599
Filed bug 637599
(Assignee)

Comment 13

6 years ago
Created attachment 525929 [details] [diff] [review]
fix v4
Attachment #514104 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Comment on attachment 525929 [details] [diff] [review]
fix v4

rebased and a few fix per review comment.
Attachment #525929 - Flags: review?(smontagu)
Attachment #525929 - Flags: review?(smontagu) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/a45b194f7d11
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Blocks: 181515
You need to log in before you can comment on or make changes to this bug.