1214169 - Always use ICU in nsCollationMacUC

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

Keeping the non-ICU code paths in nsCollationMacUC makes no sense. We should remove the non-ICU code paths. (Once that's done, we should use the ICU-based code on other platforms, too.)

Basically, we should assume that mUseICU is always true and remove dead code accordingly.

Comment 1

[Gordon Su]

Attachment: nsCollationMacUC.cpp

My attempt at addressing Bug 1214169. This is my first bug patch, so if I forgot anything, please tell me. Also removed mUseICU from nsCollationMacUC.h.

Removed all instances of mUseICU, and dead code using it.

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8674048 [details] [diff] [review]
nsCollationMacUC.cpp

Looks good to me. Thank you!

However, I'm not the module owner or peer, so my review isn't sufficient to land this. Also, we generally attach patches generated with |hg export|. I'll attach the changes in a patch form and request review from a module peer in a moment.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Remove the non-ICU code path

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8676092 [details] [diff] [review]
Remove the non-ICU code path

Oh, this is still lacking the .h changes.

Gordon, could you, please, post a patch that includes the .h changes (removing mUseICU as well as mCollator, the Carbon header include and the dead-code constants kCollationValueSizeFactor and kCacheSize)?

Comment 5

[Gordon Su]

Attachment: Always use ICU in nsCollationMacUC

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8678708 [details] [diff] [review]
Always use ICU in nsCollationMacUC

Review of attachment 8678708 [details] [diff] [review]:
-----------------------------------------------------------------

Please attach a new patch with the comments addressed and request a review again.

::: intl/locale/mac/nsCollationMacUC.cpp
@@ -24,3 @@
>    , mBuffer(nullptr)
>    , mBufferLen(1)
> -  , mUseICU(true)

You can also remove mLocale, mBuffer and mBufferLen because they are used only in the non-ICU code path.

@@ -26,5 @@
> -  , mUseICU(true)
> -{
> -  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> -  if (prefs) {
> -    prefs->GetBoolPref("intl.collation.mac.use_icu", &mUseICU);

Please also remove the "intl.collation.mac.use_icu" pref from modules/libpref/init/all.js and rewrite intl/locale/tests/unit/test_collation_mac_icu.js to remove dependency on the pref.

@@ -178,5 @@
> -    NS_ENSURE_TRUE(U_SUCCESS(status), NS_ERROR_FAILURE);
> -  } else {
> -    OSStatus err;
> -    UCCollateOptions newOptions;
> -    res = StrengthToOptions(newStrength, &newOptions);

Please remove the definition of StrengthToOptions. It is not used in the ICU code path.

@@ -225,5 @@
>  
> -  if (mUseICU) {
> -    rv = ConvertLocaleICU(locale, &mLocaleICU);
> -  } else {
> -    rv = ConvertLocale(locale, &mLocale);

Likewise, remove ConvertLocale.

@@ -327,4 @@
>        case UCOL_LESS: res = -1; break;
>        case UCOL_EQUAL: res = 0; break;
>        case UCOL_GREATER: res = 1; break;
>        default: MOZ_CRASH("ucol_strcoll returned bad UCollationResult");

nit: please fix the indent.

Comment 7

[Gordon Su]

Attachment: Always use ICU in nsCollationMacUC. r= Masatoshi Kimura

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8681643 [details] [diff] [review]
Always use ICU in nsCollationMacUC. r= Masatoshi Kimura

Review of attachment 8681643 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/locale/mac/nsCollationMacUC.cpp
@@ +210,5 @@
> +                          (const UChar*)PromiseFlatString(string1).get(), string1.Length(),
> +                          (const UChar*)PromiseFlatString(string2).get(), string2.Length());
> +  int32_t res;
> +  switch (uresult) {
> +      case UCOL_LESS:

I meant to say, please change the 4 indent to 2.

Comment 9

[Makoto Kato [:m_kato]]

Attachment: for checkin

applying comment #8 by review and nit fix (fix bustage)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b592618357

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5dcc2d9ccd7

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5dcc2d9ccd7