Closed Bug 1719550 Opened 2 months ago Closed 1 month ago

Unify Intl APIs in intl/locale/nsCollation.h

Categories

(Core :: Internationalization, task, P3)

task

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox93 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert, Whiteboard: [i18n-unification])

Attachments

(12 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Work: Medium
What it is: An abstraction over the collator

https://searchfox.org/mozilla-central/source/intl/locale/nsCollation.h

Summary: intl/locale/nsCollation.h → Unify Intl APIs in intl/locale/nsCollation.h
Blocks: 1719664
Assignee: nobody → gtatum

This collator has all the features needed for backing nsCollation.h. The tests
focus on making sure the component is wired correctly, but does not worry over
much about correctness of the backing implementation. I felt that this was covered
well by existing tests such as intl/locale/tests/gtest/TestCollation.cpp

As part of the mozilla::intl unification project, this removes the direct calls
to ICU4C, and instead uses mozilla::intl::Collator.

I included a little bit of simplification by removing the mHasCollator, and
relied on the mCollator to be nullptr instead.

I also removed nsCollation::ConvertStrength so that I could only forward
declare the mozilla::intl::Collator and simplify the header imports. The
function where it was being used ended up being simplified anyway through
the new API.

Depends on D120494

Attachment #9232423 - Attachment description: Bug 1719550 - Use the mozilla::intl API for nsCollation; r?nordzilla → Bug 1719550 - Use the mozilla::intl API for nsCollation; r?nordzilla,#platform-i18n-reviewers
Attachment #9232423 - Attachment description: Bug 1719550 - Use the mozilla::intl API for nsCollation; r?nordzilla,#platform-i18n-reviewers → Bug 1719550 - Use the mozilla::intl API for nsCollation; r?nordzilla
Attachment #9232422 - Attachment description: Bug 1719550 - Build an initial unified mozilla::intl::Collator; r?nordzilla → Bug 1719550 - Build an initial unified mozilla::intl::Collator; r?#platform-i18n-reviewers
Attachment #9232423 - Attachment description: Bug 1719550 - Use the mozilla::intl API for nsCollation; r?nordzilla → Bug 1719550 - Use the mozilla::intl API for nsCollation; r?#platform-i18n-reviewers

This is an experiment to completely remove nsCollation.

Depends on D120495

Attachment #9232423 - Attachment is obsolete: true

SpiderMonkey requires the BCP 47 locale extensions, which involves iterating
over the keywords in ICU, and mapping them to the BCP 47 version. Specifically,
this will change the "phonebook" keyword to "phonebk". This should hopefully
expose a simpler API to SpiderMonkey, the only consumer.

Depends on D120494

Blocks: 1722541
Attachment #9232422 - Attachment description: Bug 1719550 - Build an initial unified mozilla::intl::Collator; r?#platform-i18n-reviewers → Bug 1719550 - Build an initial unified mozilla::intl::Collator; r?#platform-i18n-reviewers!
Attachment #9233200 - Attachment description: Bug 1719550 - Add support for BCP 47 extensions to mozilla::intl::Collator; r?#platform-i18n-reviewers → Bug 1719550 - Add support for BCP 47 extensions to mozilla::intl::Collator; r?#platform-i18n-reviewers!
Attachment #9233201 - Attachment description: Bug 1719550 - Use mozilla::intl::Collator in the SpiderMonkey; r?#platform-i18n-reviewers,anba,tcampbell → Bug 1719550 - Use mozilla::intl::Collator in the SpiderMonkey; r?#platform-i18n-reviewers!,anba,tcampbell

This method will allow mozilla::intl components to be initialized in the current
Gecko app's locale.

Depends on D120904

Attachment #9232471 - Attachment description: WIP: Bug 1719550 - Remove nsCollation from nsNavHistory.cpp → Bug 1719550 - Unify collation in nsNavHistory.cpp; r?#platform-i18n-reviewers!

This now uses JavaScript's standardized Intl.Collator.

Depends on D121433

All the call sites now use mozilla::intl::Collator

Depends on D121434

Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b04162618a1
Build an initial unified mozilla::intl::Collator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/7b46b5cc7424
Add support for BCP 47 extensions to mozilla::intl::Collator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/b14702168fb9
Use mozilla::intl::Collator in the SpiderMonkey; r=anba,platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/a8b778ca2375
Add a TryCreateComponent to LocaleService; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/bd125cc5b9b4
Unify collation in nsNavHistory.cpp; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/48f39b5ef77f
Unify collator in nsXULSortService and nsXULContentUtils; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/389a77da6ddc
Unify collator in txXPathResultComparator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/3ef5792020bb
Unify collator in nsDirectoryIndexStream.cpp; r=platform-i18n-reviewers,necko-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/b4b0d58778e9
Unify collator in mozStorageService and SQLCollations; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/105676dd2d6f
Remove unused nsLocaleConstructors.h; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/5e2019335244
Use Intl.Collator in test_locale_collation.js; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/3b2735a29138
Remove nsCollation; r=platform-i18n-reviewers,nordzilla
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f05d70fc350c
Build an initial unified mozilla::intl::Collator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/4c972c58e835
Add support for BCP 47 extensions to mozilla::intl::Collator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/45228aa42bc7
Use mozilla::intl::Collator in the SpiderMonkey; r=anba,platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/6aad2834ea2f
Add a TryCreateComponent to LocaleService; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/ed5309e3101a
Unify collation in nsNavHistory.cpp; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/07628cc8c768
Unify collator in nsXULSortService and nsXULContentUtils; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/bfa3c9c9e5d3
Unify collator in txXPathResultComparator; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/5e7ff35e0560
Unify collator in nsDirectoryIndexStream.cpp; r=platform-i18n-reviewers,necko-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/9f822c3f4f6a
Unify collator in mozStorageService and SQLCollations; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/184d7eadd3a1
Remove unused nsLocaleConstructors.h; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/fae99cc97f31
Use Intl.Collator in test_locale_collation.js; r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/1a74809df6b9
Remove nsCollation; r=platform-i18n-reviewers,nordzilla
Flags: needinfo?(gtatum)

== Change summary for alert #30871 (as of Tue, 10 Aug 2021 16:35:26 GMT) ==

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
19% decision gecko-decision 144.49 -> 116.75

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30871

Keywords: perf-alert
Duplicate of this bug: 1719728
Whiteboard: [i18n-unification]
Duplicate of this bug: 1722541
You need to log in before you can comment on or make changes to this bug.