1701695 - Use intl::NumberFormat to format numbers in SpiderMonkey

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P1)

Description

[Dan Minor [:dminor]]

Once the "unified" intl::NumberFormat implementation lands in Bug 1695937, the next step will be to support the full Intl.NumberFormat API, and switch SpiderMonkey over to using it.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Add support for format to parts to intl::NumberFormat

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Use intl::NumberFormatterSkeleton in PluralRules

This temporarily adds intl::NumberFormatterSkeleton to the public API so that
we can use it in PluralRules. This allows us to remove the SpiderMonkey
NumberFormatterSkeleton implementation when we convert SpiderMonkey to use
intl::NumberFormat.

We can remove intl::NumberFormatterSkeleton from the public API when we
implement intl::PluralRules in Bug 1704509.

Depends on D111886

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Use intl::NumberFormat in SpiderMonkey

Depends on D111887

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Return std::u16string_view instead of char16_t* in intl::NumberFormat

The SpiderMonkey call sites need both the pointer and the length when
copying/deflating the string, so this changes the API to expose the underlying
string_view.

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Remove unused unitStyle parameter from intl_FormatNumber

Depends on D111888

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Add documentation to NumberFormat.h; r=gregtatum!,nordzilla!

Depends on D112802

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Rename NumberFormatOptions::CurrencyDisplayStyle; r=gregtatum!

This should be CurrencyDisplay to be consistent with ecma-402.

Depends on D112801

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Restore [[nodiscard]] annotations to NumberFormatterSkeleton; r=gregtatum!

I foolishly dropped these when bringing the code over from SpiderMonkey.

Depends on D113557

Comment 9

[Dan Minor [:dminor]]

Recent try job: https://treeherder.mozilla.org/jobs?repo=try&revision=66c72019d70f89e9cdb1da963021e7ba04d5f306

Comment 10

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Add NumberFormat::TryCreate; r=gregtatum!,nordzilla!,tcampbell!

This adds a fallible factory method to create new NumberFormat instances. This
allows us to report initialization errors at time of initialization, rather than
when format is called, and remove internal checks in the implementation for
successful initialization.

To support using the SpiderMonkey allocators, the TryCreate method optionally
takes an allocator as an argument and a DeletePolicy as template argument.
Default values are provided so that these do not need to be specified in Gecko
code.

The existing fluent code assumes that creating a NumberFormat instance always
succeeds. This patch updates that code to handle failures.

Depends on D113145

Comment 11

[Dan Minor [:dminor]]

Attachment: Bug 1701695 - Add NumberFormat::TryCreate; r=tcampbell!

This adds a fallible factory method to create new NumberFormat instances. This
allows us to report initialization errors at time of initialization, rather than
when format is called, and remove internal checks in the implementation for
successful initialization.

The existing fluent code assumes that creating a NumberFormat instance always
succeeds. This patch updates that code to handle failures.

Depends on D113145

Comment 12

[Dan Minor [:dminor]]

Updated try job: https://treeherder.mozilla.org/jobs?repo=try&revision=c7ada96e3efacc37e7f3b47e6feaf68382513bde

Comment 13

[Dan Minor [:dminor]]

All the SpiderMonkey jobs, just in case: https://treeherder.mozilla.org/jobs?repo=try&revision=58f5f40a0ac272e4e07cdadf0eec96325caa9e0e

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c0dbc73dad2
Return std::u16string_view instead of char16_t* in intl::NumberFormat r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/89ff5f91d8e9
Rename NumberFormatOptions::CurrencyDisplayStyle; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/216472b634b9
Restore [[nodiscard]] annotations to NumberFormatterSkeleton; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/4239fe67d69f
Add support for format to parts to intl::NumberFormat r=gregtatum,tcampbell,anba
https://hg.mozilla.org/integration/autoland/rev/abcf774d3b58
Use intl::NumberFormatterSkeleton in PluralRules r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/fdf12ac55593
Use intl::NumberFormat in SpiderMonkey r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/11f97bc74622
Remove unused unitStyle parameter from intl_FormatNumber r=tcampbell,anba
https://hg.mozilla.org/integration/autoland/rev/823ed70ef5fe
Add documentation to NumberFormat.h; r=gregtatum,nordzilla
https://hg.mozilla.org/integration/autoland/rev/e88dd41c0a90
Add NumberFormat::TryCreate; r=tcampbell

Comment 15

[Cristian Brindusan [:cbrindusan]]

Backed out 9 changesets (bug 1701695) for causing xpcshell failures in test_messagecontext.js.
https://hg.mozilla.org/integration/autoland/rev/a5ce805152fe2d4a4603eb39121b03c3a1886afd

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=e88dd41c0a9000090b88b332be4af27c94fae9e2&selectedTaskRun=MLSmHXT1QqGN2a1DdBH1JQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=339378415&repo=autoland&lineNumber=3439

Comment 16

[Dan Minor [:dminor]]

The problem is with these changes: https://phabricator.services.mozilla.com/D112801. I've only seen it on Android so far. The ICU calls seem to all succeed, an Ok() is returned from formatResult(), but on the FluentBundle side it looks we get an Err, and return a nullptr which results in an empty string in Fluent.

Comment 17

[Dan Minor [:dminor]]

Moving the Buffer definition out of the FluentBuiltInNumberFormatterFormat fixes things for me locally and on try [1]. I would not have expected that to make a difference, but since this was a change we had planned to make anyway to support DateTimeFormat, I'll take it.

[1] https://treeherder.mozilla.org/jobs?repo=try&revision=86fa769f5651d4d9b7163532dc8e32af9e5e3ac1

Comment 18

[Dan Minor [:dminor]]

Another try auto job: https://treeherder.mozilla.org/jobs?repo=try&revision=75246ca540aae4439d68eb9f6e0921e0acaa4a9e

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56618c50daff
Return std::u16string_view instead of char16_t* in intl::NumberFormat r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e787260f77b9
Rename NumberFormatOptions::CurrencyDisplayStyle; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/f3a30633d181
Restore [[nodiscard]] annotations to NumberFormatterSkeleton; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/1c26ef25db84
Add support for format to parts to intl::NumberFormat r=gregtatum,tcampbell,anba
https://hg.mozilla.org/integration/autoland/rev/382a076bc5a0
Use intl::NumberFormatterSkeleton in PluralRules r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/da5fdc32ae9b
Use intl::NumberFormat in SpiderMonkey r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/666e43ab82df
Remove unused unitStyle parameter from intl_FormatNumber r=tcampbell,anba
https://hg.mozilla.org/integration/autoland/rev/ecd5abbb0ffc
Add documentation to NumberFormat.h; r=gregtatum,nordzilla
https://hg.mozilla.org/integration/autoland/rev/67f15404db7c
Add NumberFormat::TryCreate; r=tcampbell

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/56618c50daff
https://hg.mozilla.org/mozilla-central/rev/e787260f77b9
https://hg.mozilla.org/mozilla-central/rev/f3a30633d181
https://hg.mozilla.org/mozilla-central/rev/1c26ef25db84
https://hg.mozilla.org/mozilla-central/rev/382a076bc5a0
https://hg.mozilla.org/mozilla-central/rev/da5fdc32ae9b
https://hg.mozilla.org/mozilla-central/rev/666e43ab82df
https://hg.mozilla.org/mozilla-central/rev/ecd5abbb0ffc
https://hg.mozilla.org/mozilla-central/rev/67f15404db7c