Closed
Bug 1214169
Opened 8 years ago
Closed 8 years ago
Always use ICU in nsCollationMacUC
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: hsivonen, Assigned: gordonsu14)
References
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(2 files, 3 obsolete files)
18.06 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
18.01 KB,
patch
|
Details | Diff | Splinter Review |
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.
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•8 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•8 years ago
|
||
Attachment #8676092 -
Flags: review?(VYV03354)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → gordonsu14
Reporter | ||
Comment 4•8 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)
Attachment #8678708 -
Flags: review?(VYV03354)
Comment 6•8 years ago
|
||
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•8 years ago
|
Attachment #8674048 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8676092 -
Attachment is obsolete: true
Attachment #8678708 -
Attachment is obsolete: true
Attachment #8681643 -
Flags: review?(VYV03354)
Comment 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
applying comment #8 by review and nit fix (fix bustage) https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b592618357
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b5dcc2d9ccd7
Status: NEW → RESOLVED
Closed: 8 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.
Description
•