Open Bug 1719678 Opened 3 months ago Updated 4 days ago

Unify date interval formatting/ DateTime(Range) formatToParts in SpiderMonkey

Categories

(Core :: Internationalization, task, P3)

task

Tracking

()

People

(Reporter: gregtatum, Assigned: allstars.chh)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [i18n-unification], [i18n-unification-help-wanted])

Attachments

(3 files)

In https://searchfox.org/mozilla-central/source/js/src/builtin/intl/DateTimeFormat.cpp

The calls to create and use UDateIntervalFormat need to use a mozilla:intl component.

This will remove the following import:

#include "unicode/udateintervalformat.h"
Blocks: 1719680
No longer blocks: 1719680
Depends on: 1719680
Blocks: 1719693
No longer blocks: 1719693

Hi Yoshi, Greg and I think this would be a good next project for you when you have the time.

Flags: needinfo?(allstars.chh)
Whiteboard: unification-help-wanted
Whiteboard: unification-help-wanted → [i18n-unification-help-wanted]

Cool, thanks. I'll work on this, although I am on parental leave and the progress will be slower.

Assignee: nobody → allstars.chh
Flags: needinfo?(allstars.chh)
Whiteboard: [i18n-unification-help-wanted] → [i18n-unification], [i18n-unification-help-wanted]

Add a mozilla::intl::DateIntervalFormat class, with two methods:

  • TryFormatCalendar
  • TryFormatDateTime

These methods also take a callback, the callback will be called with the
UFormattedValue* argument.

However, there's a failure when running check_vanilla_allocations.py.

The callback is a std::function, and gcc will call operator new if the
lambda is too large, which is detected by check_vanilla_allocations.py
and causes failure.

https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/std_function.h#L161

Attachment #9241284 - Attachment description: Bug 1719678 - WIP: Add a mozilla::intl::DateIntervalFormat → Bug 1719678 - Part 1:(WIP) Add a mozilla::intl::DateIntervalFormat

Add DateTimeFormat::TryFormatToParts and
DateIntervalFormat::TryFormattedToParts methods.

Duplicate of this bug: 1719693
Summary: Unify date interval formatting in SpiderMonkey → Unify date interval formatting/ DateTime(Range) formatToParts in SpiderMonkey

Hi, Greg, could you check my comment in mozphab?
https://phabricator.services.mozilla.com/D125642#4086962

(copy from mozphab)

For getting the formatted string of FormatDateTimeRange, there are two interface calls:

Call DateIntervalFormat::TryFormatCalendar or TryFormatDateTime, and store the formatted value in AutoFormattedDateInterval
Use the AutoFormattedDateInterval to check if the date ranges are "practically equal" by calling DateIntervalFormat::DateFieldsPracticallyEqual

Do you think this DateIntervalFormat::DateFieldsPracticallyEqual interface makes sense?
Or should I add an extra parameter 'isEqual' in TryFormatCalendar/TryFormatDateTime?

Thanks

Flags: needinfo?(gtatum)

Do you think this DateIntervalFormat::DateFieldsPracticallyEqual interface makes sense?
Or should I add an extra parameter 'isEqual' in TryFormatCalendar/TryFormatDateTime?

It seems like the current approach adds implementation details to the unified API. My preference looking at things would be to add an extra parameter, especially if it reduces the API size. Generally we're trying to keep ICU-specific things hidden away as much as possible.

Flags: needinfo?(gtatum)
Attachment #9241284 - Attachment description: Bug 1719678 - Part 1:(WIP) Add a mozilla::intl::DateIntervalFormat → Bug 1719678 - Part 1: Add a mozilla::intl::DateIntervalFormat.
Attachment #9242445 - Attachment description: Bug 1719678 - Part 2: (WIP) Unify FormatDateTimeToParts and FormatDateTimeRangeToParts. → Bug 1719678 - Part 2: Unify FormatDateTimeToParts and FormatDateTimeRangeToParts.

Hi, Greg:
In my patches, I only handle the UTF-16 case,
(For example. DateTimeFormat::TryFormatToParts only accepts buffer with CharType is char16_t)

should I file a new bug to tackling the UTF-8 case?

Flags: needinfo?(gtatum)

I would only file UTF-8 support if we actually plan on using them in Gecko somewhere. Otherwise it's probably fine to limit scope. We can always file follow-up tracking issues if we need to switch for ICU4X. At this point, I'd prefer to keep the scope more limited here.

Flags: needinfo?(gtatum)
You need to log in before you can comment on or make changes to this bug.