Always use ICU in nsCollationMacUC

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hsivonen, Assigned: gordonsu14)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

3 years ago
Created attachment 8674048 [details] [diff] [review]
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.
Attachment #8674048 - Flags: review?(hsivonen)
(Reporter)

Comment 2

3 years ago
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.
Attachment #8674048 - Flags: review?(hsivonen) → feedback+
(Reporter)

Comment 3

3 years ago
Created attachment 8676092 [details] [diff] [review]
Remove the non-ICU code path
Attachment #8676092 - Flags: review?(VYV03354)
(Reporter)

Updated

3 years ago
Assignee: nobody → gordonsu14
(Reporter)

Comment 4

3 years ago
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)?
Attachment #8676092 - Flags: review?(VYV03354)
(Assignee)

Comment 5

3 years ago
Created attachment 8678708 [details] [diff] [review]
Always use ICU in nsCollationMacUC
(Assignee)

Updated

3 years ago
Attachment #8678708 - Flags: review?(VYV03354)
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.
Attachment #8678708 - Flags: review?(VYV03354) → feedback+
(Reporter)

Updated

3 years ago
Attachment #8674048 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8676092 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8681643 [details] [diff] [review]
Always use ICU in nsCollationMacUC. r= Masatoshi Kimura
(Assignee)

Updated

3 years ago
Attachment #8678708 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8681643 - Flags: review?(VYV03354)
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.
Attachment #8681643 - Flags: review?(VYV03354) → review+

Updated

3 years ago
Blocks: 943287

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5dcc2d9ccd7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.