Use case mapping APIs from ICU instead of building and maintaining our own mapping table

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
When ICU is available in the build, there's no need for us to have our own tables of upper/lower/titlecase mappings to support the case-conversion methods in intl/unicharutil/util. We can simply call ICU's case mapping functions. This should save around 14K of data, AFAICS.
(Assignee)

Comment 1

3 years ago
We've done this for a bunch of other properties already, but never got around to case mappings. This updates the generation tool so that the case mappings will be excluded from builds where ICU is available, saving about 14K of data size.
Attachment #8795290 - Flags: review?(m_kato)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
And this patch (to be merged with pt 1 for landing) makes the mozilla::unicode::Get*case functions into simple wrappers around the ICU functions.
Attachment #8795292 - Flags: review?(m_kato)
(Assignee)

Comment 3

3 years ago
While we're here, I think it will make the code a lot easier to read if we collect all the ICU-based wrappers together in one #if-block, instead of scattering #if-#else-#endif through each individual accessor. This will help us to see how much code is really left here, and how much can be dropped entirely once we no longer need to support an ICU-free build.
Attachment #8795293 - Flags: review?(m_kato)
(Assignee)

Comment 4

3 years ago
Looks like we also need to do this, to avoid build bustage from duplicate macro definitions; now that we're pulling in ICU headers more widely, we don't need these copies in nsBidi.h. (This will also need to be folded together with the previous patch to avoid the risk of broken bisections.)
Attachment #8795303 - Flags: review?(m_kato)
Attachment #8795290 - Flags: review?(m_kato) → review+
Attachment #8795303 - Flags: review?(m_kato) → review+
Attachment #8795292 - Flags: review?(m_kato) → review+
Comment on attachment 8795293 [details] [diff] [review]
pt 3 - Clean up/simplify use of ENABLE_INTL_API conditionals in nsUnicodeProperties (code rearrangement, no change in behavior)

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

::: intl/unicharutil/util/nsUnicodeProperties.h
@@ +95,5 @@
> +{
> +  UNumericType type =
> +    UNumericType(u_getIntPropertyValue(aCh, UCHAR_NUMERIC_TYPE));
> +  return type == U_NT_DECIMAL || type == U_NT_DIGIT
> +         ? int8_t(u_getNumericValue(aCh)) : -1;  

nits: Trim space after -1;.
Attachment #8795293 - Flags: review?(m_kato) → review+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/200c4694551f4ff7cfb6d1045c257d3023e5b8da
Bug 1305700 - pt 1 & 2 - Exclude case mappings from nsUnicodePropertyData.cpp, and use ICU case mappings instead of our own table when building with ENABLE_INTL_API. r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf57f95f81406754835dc7005a29c2d18eec8f0d
Bug 1305700 - pt 3 & 4 - Clean up/simplify use of ENABLE_INTL_API conditionals in nsUnicodeProperties (code rearrangement, no change in behavior). r=m_kato

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/200c4694551f
https://hg.mozilla.org/mozilla-central/rev/bf57f95f8140
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.