Closed Bug 1170072 Opened 4 years ago Closed 4 years ago

Remove nsCharProps1 if using ICU

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

All members of nsCharProps1 can replace with ICU API
Attached patch WIP (obsolete) — Splinter Review
WIP.  Also, I should add fix to perl script (genUnicodePropertyData.pl) for this.
Blocks: 1184116
Assignee: nobody → m_kato
Attachment #8613366 - Attachment is obsolete: true
Comment on attachment 8655876 [details] [diff] [review]
Part 1. Make GetCharProps1 as static function

nsCharProps1 can replace with ICU, so I want that GetCharProps1 isn't exported to replace with ICU.
Attachment #8655876 - Flags: review?(jfkthame)
Comment on attachment 8655878 [details] [diff] [review]
Part 2. Use ICU instead of GetCharProps1 if available

If ICU is available, we should use ICU API instead of using nsCharProps1
Attachment #8655878 - Flags: review?(jfkthame)
Just wondering, have you compared performance at all (e.g. talos tp numbers) to see if this makes any measurable change (either better or worse) compared to our existing code? I think character property lookup is pretty hot code.
OK, looks like this isn't going to move the needle perceptibly (either way), which is fine - thanks.
Attachment #8655876 - Flags: review?(jfkthame) → review+
Comment on attachment 8655878 [details] [diff] [review]
Part 2. Use ICU instead of GetCharProps1 if available

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

Looks OK to me. (Please make the commit message a bit more complete - e.g. insert "instead of GetCharProps1 and its supporting data", or something like that.)
Attachment #8655878 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/eab3103736eb
https://hg.mozilla.org/mozilla-central/rev/5bc2bf65bcf5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.