Add pattern/timeStyle/dateStyle for Intl.DateTimeFormat

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We're currently migrating from custom Intl API's to ICU backed ones, primarily focusing on ECMA402 style APIs.

That means that we're in an interesting position of both pushing the spec forward, and trying to replace existing APIs with it.

In case of date and time formatting there are two major areas that old APIs provide and in order to not regress while replacing, we need to cover with the new APIs.

1) common styles ( https://github.com/tc39/ecma402/issues/108 )
2) OS preferences ( https://github.com/tc39/ecma402/issues/109 )

While conversation in the ECMA402 is going on, I believe it's worth designing out own solution for the chrome-only code in order to be able to progress with the technical cleanups.
It will also allow us to test the proposed solutions and help design the future standard.

I'd like to propose implementing a new API that wraps around Intl.DateTimeFormat and adds two features:

1) `style` option that can take one of the values 12 values:

`short`/`medium`/`long`/`full`,
time-[`short`/`medium`/`long`/`full`]
date-[`short`/`medium`/`long`/`full`]

If the style is provided, it picks the skeleton from CLDR field Date & Time/Gregorian/Formats - Standard - */[short/medium/long/full] and only then adds fields explicitly listed by the options.

This would allow the user to do the most common:

mozIntl.DateTimeFormat(locale, {
  style: 'long',
});

but also:

mozIntl.DateTimeFormat(locale, {
  style: 'long',
  hour12: true,
});

or

mozIntl.DateTimeFormat(locale, {
  style: 'long',
  month: '2-digit',
});

2) Merge bug 1308329 API into this and pick style skeletons and hour12 setting from OS when possible, falling back on the CLDR when not.

I'm not a huge fan of this approach, and I'd prefer to use CLDR and only take the manually altered bits from the OS, it seems that it's not easy to separate OS defaults from user-edited bits at the moment and I believe that it's less important for us to do that in-chrome.

I believe that having an API like this available would allow us to migrate the whole chrome to use it without regressing features.
(Assignee)

Comment 1

2 years ago
:jfkthame, :m_kato - how does it sound to you?
Flags: needinfo?(m_kato)
Flags: needinfo?(jfkthame)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> I'd like to propose implementing a new API that wraps around
> Intl.DateTimeFormat and adds two features:
> 
> 1) `style` option that can take one of the values 12 values:
> 
> `short`/`medium`/`long`/`full`,
> time-[`short`/`medium`/`long`/`full`]
> date-[`short`/`medium`/`long`/`full`]

Another approach: two options `date-style` and `time-style', each of which can take one of 4 values `short`..`full`. I guess I like this mostly because it maps more directly to udat_open() parameters, and doesn't require as many individual values to be defined, but I don't suppose it matters much.

> If the style is provided, it picks the skeleton from CLDR field Date &
> Time/Gregorian/Formats - Standard - */[short/medium/long/full] and only then
> adds fields explicitly listed by the options.
> 
> This would allow the user to do the most common:
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
> });
> 

If a style is provided, and no other options are specified, we should be able to implement that using udat_open directly with timeStyle and dateStyle parameters, and not need to go through the extra steps of picking a skeleton and resolving it to a pattern.

> but also:
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
>   hour12: true,
> });
> 
> or
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
>   month: '2-digit',
> });

Yes, I think we can do this via udatpg_* functions, extracting the skeleton from the standard pattern for a given style, tweaking it, and then using the modified skeleton to create a new pattern.

> 
> 2) Merge bug 1308329 API into this and pick style skeletons and hour12
> setting from OS when possible, falling back on the CLDR when not.

In most cases, at least, I think the OS doesn't give us direct access to style skeletons; we'd have to infer the skeleton from the behavior of the available formatting APIs. I guess we could do that, but it might be simpler to only check a few basic properties such as 12/24h to use as modifiers on top of the default (ICU/CLDR) styles.

To support more extensive customization (if we feel it's worthwhile) I think it might make more sense to have a `pattern` option, which (if present) would cause all the other style options to be ignored, and the pattern would be used directly, as in

  mozIntl.DateTimeFormat(locale, { pattern: '"at" hh:mm:ss "on the" dd mmm, yyyy' });

or something like that. (Note that for platforms where the user can fully customize the format, that customization can only really be represented and passed to the formatting function as a pattern; if we distill it to a skeleton, the customization of separator characters/strings, etc, is lost.)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 3

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Another approach: two options `date-style` and `time-style', each of which
> can take one of 4 values `short`..`full`. I guess I like this mostly because
> it maps more directly to udat_open() parameters, and doesn't require as many
> individual values to be defined, but I don't suppose it matters much.

Yeah, I'm totally fine with this. It shouldn't be hard to switch between those two and we can bikeshed more in ECMA402.
 
> In most cases, at least, I think the OS doesn't give us direct access to
> style skeletons; we'd have to infer the skeleton from the behavior of the
> available formatting APIs. I guess we could do that, but it might be simpler
> to only check a few basic properties such as 12/24h to use as modifiers on
> top of the default (ICU/CLDR) styles.

Yeah, we can even start with only taking:
 - h12/24
 - styles for full/long/medium/short
 
> To support more extensive customization (if we feel it's worthwhile) I think
> it might make more sense to have a `pattern` option, which (if present)
> would cause all the other style options to be ignored, and the pattern would
> be used directly, as in
> 
>   mozIntl.DateTimeFormat(locale, { pattern: '"at" hh:mm:ss "on the" dd mmm,
> yyyy' });

this has been raised in ECMA402 as an option, I believe worth exploring although standardizing the DSL for the pattern will be nasty ;)

:jfkthame, any chance you'd be interested in taking this? :)
Flags: needinfo?(jfkthame)
This would primarily mean implementing stuff within the JS engine, right? I'm totally unfamiliar with that kind of coding... presumably I could dig in and learn enough to make progress, but it'd be far more efficient if we could find a JS-engine hacker to help out here!
Flags: needinfo?(jfkthame)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> We're currently migrating from custom Intl API's to ICU backed ones,
> primarily focusing on ECMA402 style APIs.
> 
> That means that we're in an interesting position of both pushing the spec
> forward, and trying to replace existing APIs with it.
> 
> In case of date and time formatting there are two major areas that old APIs
> provide and in order to not regress while replacing, we need to cover with
> the new APIs.
> 
> 1) common styles ( https://github.com/tc39/ecma402/issues/108 )
> 2) OS preferences ( https://github.com/tc39/ecma402/issues/109 )
> 
> While conversation in the ECMA402 is going on, I believe it's worth
> designing out own solution for the chrome-only code in order to be able to
> progress with the technical cleanups.
> It will also allow us to test the proposed solutions and help design the
> future standard.
> 
> I'd like to propose implementing a new API that wraps around
> Intl.DateTimeFormat and adds two features:
> 
> 1) `style` option that can take one of the values 12 values:
> 
> `short`/`medium`/`long`/`full`,
> time-[`short`/`medium`/`long`/`full`]
> date-[`short`/`medium`/`long`/`full`]
> 
> If the style is provided, it picks the skeleton from CLDR field Date &
> Time/Gregorian/Formats - Standard - */[short/medium/long/full] and only then
> adds fields explicitly listed by the options.
> 
> This would allow the user to do the most common:
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
> });
> 
> but also:
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
>   hour12: true,
> });
> 
> or
> 
> mozIntl.DateTimeFormat(locale, {
>   style: 'long',
>   month: '2-digit',
> });

Yes, no objection.  
 
> 2) Merge bug 1308329 API into this and pick style skeletons and hour12
> setting from OS when possible, falling back on the CLDR when not.
> 
> I'm not a huge fan of this approach, and I'd prefer to use CLDR and only
> take the manually altered bits from the OS, it seems that it's not easy to
> separate OS defaults from user-edited bits at the moment and I believe that
> it's less important for us to do that in-chrome.
> 
> I believe that having an API like this available would allow us to migrate
> the whole chrome to use it without regressing features.

Actually, we only use OS setting on OSX and Windows via API and OS setting.  UNIX only uses LC_TIME (we don't use GNOME setting) and Android doesn't use it (because Bionic doesn't support locale correctly).  And until Firefox OS 2.0, we didn't have this option to control it.

I think that we might have to have a option via preference for 12 hour vs 24 hour.  Since OS doesn't export API for it, we need investigate system settings such as registry.  So it might cause another bugs.
Flags: needinfo?(m_kato)
(Assignee)

Comment 6

2 years ago
> I think that we might have to have a option via preference for 12 hour vs 24 hour.  Since OS doesn't export API for it, we need investigate system settings such as registry.

:jfkthame in bug 1308329 wrote code for the three main platforms to retrieve 12/24h bit from the OS settings. I'm worried that adding date/time formatting settings to the browser won't scale and scenario in which someone wants their OS to use h12 and their browser to use h24 is virtually non-existing.

My suggestion would be rather to attempt to retrieve the bit from the OS, and stick to CLDR defaults if we cannot.
Does it sound reasonable?
Flags: needinfo?(m_kato)
Flags: needinfo?(jfkthame)
Yes, the browser doesn't need its own date/time formatting settings, IMO. It should simply use the CLDR-specified formatting for the chosen locale, modified by any bits of information (e.g. 12/24hr preference) we can retrieve from the OS.

We can potentially take that further and honor complete patterns from the OS, if they're available; but I don't see any reason to start creating a new place for such settings, distinct from either CLDR or the host OS.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 8

2 years ago
> Yes, the browser doesn't need its own date/time formatting settings, IMO.

I got a potential use case from our hebrew localizer. Users may actually want to fine-tune that. I think it's fine to keep it in mind and think about it for the future to consider adding a "Regional Settings" preferences panel that would allow users to override OS regional settings, but I believe it can be done later.

Now, my plan is to start with:

1) Add 'style' option which can take values [date|time|]-[full|long|medium|short]

I'm leaning toward this against :jfkthame alternative proposal because not only date and time can follow the style, but also the date+time pattern combination can follow the style, and I'd like to avoid building an API where you have to define style for date, for time and for how to combine them.

So 'long' will give you date+time long, 'time-medium' will give you just time in medium etc.

We'll take the values from CLDR - http://www.unicode.org/cldr/charts/30/summary/en.html#1816

2) Add ability to load from OS:
 - hourCycle setting
 - date-time pattern for style long|medium|short|narrow (with fallback on CLDR)

I believe that this is feasiable, and will make the mozIntl variant a good replacement for our current chrome code without regressing.

:waldo - for the (1) I need to access ICU and DateTimeFormat. I can do one of the three:
 - I can add code to our current Intl.cpp that is for chrome only that allows for 'style' option and uses EStyle [0]
 - I can add separate code to retrieve the pattern for styles using ICU and then only add 'pattern' option (that works only when called from mozIntl) to Intl.cpp DateTimeFormat and pass the pattern
 - duplicate the whole js::intl_FormatDateTime to js::mozIntl_FormatDateTime that accepts style.

Notice: there is a proposal to add the style param to ECMA402, so I see it as something we want to land for Firefox UI first, but eventually expose to the web content once we standardize it.

How should I write the code?


[0] http://www.icu-project.org/apiref/icu4c/classicu_1_1DateFormat.html
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 9

2 years ago
Ok, got a plan. Pending bug 1328386, I'll introduce `mozExtensions` bool that will be passed to DateTime constructor and be true for mozIntl and false for regular Intl.
Depends on: 1328386
Flags: needinfo?(jwalden+bmo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> > I think that we might have to have a option via preference for 12 hour vs 24 hour.  Since OS doesn't export API for it, we need investigate system settings such as registry.
> 
> :jfkthame in bug 1308329 wrote code for the three main platforms to retrieve
> 12/24h bit from the OS settings. I'm worried that adding date/time
> formatting settings to the browser won't scale and scenario in which someone
> wants their OS to use h12 and their browser to use h24 is virtually
> non-existing.
> 
> My suggestion would be rather to attempt to retrieve the bit from the OS,
> and stick to CLDR defaults if we cannot.
> Does it sound reasonable?

Yes, for Firefox and WebEngine.  OS date time settings are too flexible.  If others such as Thunderbird require flexible format like OS setting, they can implement it into their code.
Flags: needinfo?(m_kato)
Priority: -- → P3
(Assignee)

Comment 12

2 years ago
Comment on attachment 8831044 [details]
Bug 1329904 - Introduce mozIntl.DateTimeFormat with support for pattern.

So, for now, in order to support the first two features from the OS, I'm going to:

 - create the DateTimeFormat to be able to create it with an additional "mozExtensions" param which will handle `pattern` option
 - extend OSPreferences to provide me the pattern for styles from the OS and hour12/24 format
 - create a thin wrapper in toolkit's mozIntl that allows user to pass `style` option. If the style is passed, it'll use the OSPreferences pattern and pass it to the extended DateTimeFormat.

This is the SM part of the patch and I need a bit of help to figure out how to best pass the "mozExtensions" boolean so that the InitializeDateTimeFormat can use it to handle additional option.
Andre, can you look at the patch and give me a general feedback on whether it seems to be going in the right direction and also help me pass the param to the right place?

Thanks
Attachment #8831044 - Flags: feedback?(andrebargull)
Posted patch bug1329904-changes.patch (obsolete) — Splinter Review
Applies on top of your patch. 

I've removed the extra checks which were copied from Intl.PluralRules because they aren't needed here. I think the easiest way to pass the mozExtensions parameter to InitializeDateTimeFormat, is a separate constructor entry function (MozDateTimeFormat in the patch).

I didn't perform extensive testing, but everything seems to work so far - the existing Intl tests still pass and the new "pattern" option is available after calling addIntlExtras:
---
js> addIntlExtras(Intl)
js> new Intl.DateTimeFormat("pl", {pattern: "zzzz"}).format()
"Czas pacyficzny standardowy"
---
(In reply to André Bargull from comment #13)
> Applies on top of your patch. 

I forgot to update Intl_DateTimeFormat_resolvedOptions to exclude the "pattern" option for standard Intl.DateTimeFormat instances.
Do we need to validate the "pattern" option before passing it to ICU? Or do we expect that the pattern was already validated before it was passed to mozIntl.DateTimeFormat?

udat_format appears to be a bit more lenient compared to udat_formatForFields when invalid patterns are used:

js> addIntlExtras(Intl)
js> new Intl.DateTimeFormat(undefined, {pattern: null}).formatToParts()
typein:2:1 Error: internal error while computing Intl data
js> new Intl.DateTimeFormat(undefined, {pattern: null}).format()
""
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
Ugh, I realized that I do need the dateStyle/timeStyle to work on SM DateTimeFormat because we may not be able to retrieve the pattern from the OS and then we will want to use the CLDR one. Updated the patch.
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
> Do we need to validate the "pattern" option before passing it to ICU? Or do we expect that the pattern was already validated before it was passed to mozIntl.DateTimeFormat?

I didn't find a way in udat_* or udatpg_* to validate the pattern.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #21)
> > Do we need to validate the "pattern" option before passing it to ICU? Or do we expect that the pattern was already validated before it was passed to mozIntl.DateTimeFormat?
> 
> I didn't find a way in udat_* or udatpg_* to validate the pattern.

Worst case is we need to write our own validator based on the structure described in http://unicode.org/reports/tr35/tr35-dates.html#Date_Format_Patterns. In that case we should probably try to follow http://unicode.org/reports/tr35/tr35.html#Invalid_Patterns.
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Summary: Introduce mozIntl.DateTimeFormat → Add pattern/timeStyle/dateStyle for Intl.DateTimeFormat
(Assignee)

Updated

2 years ago
Component: Internationalization → JavaScript: Internationalization API
(Assignee)

Updated

2 years ago
Blocks: 1339650

Comment 23

2 years ago
mozreview-review
Comment on attachment 8831044 [details]
Bug 1329904 - Introduce mozIntl.DateTimeFormat with support for pattern.

https://reviewboard.mozilla.org/r/107694/#review113938

::: js/src/builtin/Intl.h:419
(Diff revision 5)
>  /**
> + * Return a pattern in the date-time format pattern language of Unicode
> + * Technical Standard 35, Unicode Locale Data Markup Language, for the
> + * best-fit date-time style for the given locale.
> + *
> + * Usage: pattern = intl_patternForStyle(locale, dateStyle, timeStyle)

This should document what the various arguments can be: `locale`, a valid language tag; and for `dateStyle` and `timeStyle`, either `undefined` or one of the limited set of permissible strings.  And it should say something about what those strings map to -- to patterns obeying the rough progression established by the strings.  Or something like that.

::: js/src/builtin/Intl.cpp:822
(Diff revision 5)
>  }
>  
>  static bool
>  LegacyIntlInitialize(JSContext* cx, HandleObject obj, Handle<PropertyName*> initializer,
>                       HandleValue thisValue, HandleValue locales, HandleValue options,
> -                     MutableHandleValue result)
> +                     mozilla::Maybe<bool> mozExtensions, MutableHandleValue result)

A change should be landing shortly to forbid use of `Maybe` as a parameter, passed by value.  So to get ahead of that, make this

    const mozilla::Maybe<bool>& mozExtensions

That said, it's probably a better/simpler idea to just make the argument non-optional and always pass it.  *Only* this particular case would pass true, and all the others stil false, and so no behavior change would occur for them.  Let's do that instead of using `Maybe` at all.

::: js/src/builtin/Intl.cpp:822
(Diff revision 5)
>  }
>  
>  static bool
>  LegacyIntlInitialize(JSContext* cx, HandleObject obj, Handle<PropertyName*> initializer,
>                       HandleValue thisValue, HandleValue locales, HandleValue options,
> -                     MutableHandleValue result)
> +                     mozilla::Maybe<bool> mozExtensions, MutableHandleValue result)

Add at the top of the file, in the list of other `using mozilla::*;`, alphabetically, `using mozilla::Maybe;`.  And similar for the other `mozilla::` symbols you're using in this file.

::: js/src/builtin/Intl.cpp:2371
(Diff revision 5)
>   * 12.2.1 Intl.DateTimeFormat([ locales [, options]])
>   *
>   * ES2017 Intl draft rev 94045d234762ad107a3d09bb6f7381a65f1a2f9b
>   */
>  static bool
> -DateTimeFormat(JSContext* cx, const CallArgs& args, bool construct)
> +DateTimeFormat(JSContext* cx, const CallArgs& args, bool construct, bool mozExtensions = false)

Add

    enum class DateTimeFormatOptions
    {
        Standard,
        EnableMozExtensions,
    };

and make that the argument type.  Also, don't give it a default -- change all the callers to explicitly pass what they want.

::: js/src/builtin/Intl.cpp:2456
(Diff revision 5)
> -                              MutableHandleObject constructor)
> +                              MutableHandleObject constructor, bool mozExtensions = false)
>  {
>      RootedFunction ctor(cx);
> -    ctor = GlobalObject::createConstructor(cx, &DateTimeFormat, cx->names().DateTimeFormat, 0);
> +    ctor = mozExtensions
> +           ? GlobalObject::createConstructor(cx, &MozDateTimeFormat, cx->names().DateTimeFormat, 0)
> +           : GlobalObject::createConstructor(cx, &DateTimeFormat, cx->names().DateTimeFormat, 0);

I'm extremely uncomfortable with having this function literally overwrite the existing, saved `Intl.DateTimeFormat` value and `Intl.DateTimeFormat.prototype` internally-cached value.  I'm pretty sure if you allow this, this code will fail:

    var DF = Intl.DateTimeFormat; // the original
    addIntlExtras(Intl); // overwrites Intl.DTF *but also* the cahced prototype
    assertEq(Object.getPrototypeOf(new DF), DF.prototype,
             "constructor returning objects having different prototype over time!");

Not cool.  It seems to me there are two plausible options to fix this.

One, we can add a global flag -- i.e. another entry in `GlobalObject::WarnOnceFlag`.  This would semi-silently change the behavior of the *existing* constructor, tho, so it'd be hard to tell exactly who was depending on the behavior.

Or two, we can have a *separate* `Intl.MozDateTimeFormat` (spelling bikesheddable, I don't care).  Separate function that doesn't affect the original.

I think I prefer the second one.  Please add two new `DATE_TIME_FORMAT` and `DATE_TIME_FORMAT_PROTO` slots to `GlobalObject`, then make the extensions argument to this function (which should be the `enum` mentioned above) determine name/function pointer to use to define the function on the provided `Intl` object.  Then set the slots in `AddDateTimeFormatConstructor` (which should be renamed to be consistent with the fresh name for this constructor function), similar to how `GlobalObject::initIntlObject` does it.

Also, a nitpick while you're changing this, remove the `&` before `DateTimeFormat` referring to the function pointer -- it isn't necessary.

::: js/src/builtin/Intl.cpp:3096
(Diff revision 5)
> +            dateStyle = UDAT_LONG;
> +        } else if (StringEqualsAscii(dateStyleStr, "medium")) {
> +            dateStyle = UDAT_MEDIUM;
> +        } else if (StringEqualsAscii(dateStyleStr, "short")) {
> +            dateStyle = UDAT_SHORT;
> +        }

Add

    else {
        MOZ_ASSERT_NOT_REACHED("unexpected dateStyle");
    }

or however the macro-name is spelled, to this and the other below, so we find out about any programming mistakes that might result in a bad style appearing.

::: js/src/builtin/Intl.cpp:3115
(Diff revision 5)
> +        }
> +    }
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    UDateFormat* df = udat_open(
> +            timeStyle,

Style should be

    UDateFormat* df = udat_open(timeStyle, dateStyle, icuLocale(locale.ptr()), nullptr, -1,
                                ...);

wrapped at 99ch.

Why are you passing `nullptr` and `-1` for the time zone?  You should be doing things similar to what `NewUDateFormat` does to compute and pass relevant values to `udat_open`, seems to me -- and you should pass in a time zone from the caller of this function, which certainly has it at hand.

::: js/src/builtin/Intl.cpp:3131
(Diff revision 5)
> +    }
> +    ScopedICUObject<UDateFormat, udat_close> toClose(df);
> +
> +    Vector<char16_t, INITIAL_CHAR_BUFFER_SIZE> chars(cx);
> +    if (!chars.resize(INITIAL_CHAR_BUFFER_SIZE))
> +        return false;

I believe anba's landed some helper code to do this buffer-resizing stuff automagically -- please use that now.

::: js/src/builtin/Intl.js:2236
(Diff revision 5)
>      // Step 18.
>      var formatOpt = lazyDateTimeFormatData.formatOpt;
>  
>      // Steps 27-28, more or less - see comment after this function.
> -    var pattern = toBestICUPattern(dataLocale, formatOpt);
> +    var pattern;
> +

Remove the blank line here.

::: js/src/builtin/Intl.js:2238
(Diff revision 5)
>  
>      // Steps 27-28, more or less - see comment after this function.
> -    var pattern = toBestICUPattern(dataLocale, formatOpt);
> +    var pattern;
> +
> +    if (lazyDateTimeFormatData.mozExtensions) {
> +        var pattern;

Remove the spurious declaration.

::: js/src/builtin/Intl.js:2400
(Diff revision 5)
>      // Step 18.
>      var formatOpt = new Record();
>      lazyDateTimeFormatData.formatOpt = formatOpt;
>  
> +    if (mozExtensions) {
> +        lazyDateTimeFormatData.mozExtensions = true;

You haven't updated the documentation comment a fair bit earlier in this file to document this property, or any of the other optional properties beneath here.  That should be the *first* thing to change when making modifications, not the last (or entirely forgotten), so you have a clear written record of what you've done, that doesn't require grokking a bunch of code.

::: js/src/builtin/Intl.js:2403
(Diff revision 5)
>  
> +    if (mozExtensions) {
> +        lazyDateTimeFormatData.mozExtensions = true;
> +
> +        let pattern = GetOption(options, "pattern", "string", undefined, undefined);
> +        lazyDateTimeFormatData.patternOption = pattern;

So if someone passes in "" as the pattern, are you definitely sure you want the default-ful behavior, as opposed to acting as if the pattern really were the empty string?  Because right now, the way you've done this, by testing |if (lazyDateTimeFormatData.patternOption)|, you've got the default-ful behavior.

::: js/src/builtin/Intl.js:2517
(Diff revision 5)
>  // DateTimeFormat options to ICU skeletons, and then lets ICU map skeletons to
>  // actual ICU patterns. The pattern may not directly correspond to what the
>  // skeleton requests, as the mapper (UDateTimePatternGenerator) is constrained
>  // by the available locale data for the locale. The resulting ICU pattern is
> -// kept as the DateTimeFormat's [[pattern]] internal property and passed to ICU
> -// in the format method.
> +// kept as the DateTimeFormat's [[pattern]] internal property and
> +// passed to ICU in the format method.

Not sure why this change...

::: js/src/builtin/Intl.js:2935
(Diff revision 5)
> +
> +        if (internals.dateStyle) {
> +          result.dateStyle = internals.dateStyle;
> +        }
> +        if (internals.timeStyle) {
> +          result.timeStyle = internals.timeStyle;

It seems to me that when `pattern` derives wholly from `dateStyle` and `timeStyle` by internal algorithm, we shouldn't include it in the output.  Rather, I'd say this should be like so:

    if (internals.mozExtensions) {
        if (internals.dateStyle || internals.timeStyle) {
            result.dateStyle = internals.dateStyle;
            result.timeStyle = internals.timeStyle;
        } else {
            result.pattern = internals.pattern;
        }
    }

::: js/src/jsfriendapi.h:2871
(Diff revision 5)
>  AddPluralRulesConstructor(JSContext* cx, JS::Handle<JSObject*> intl);
>  
> +// Create and add the Intl.DateTimeFormat constructor function to the provided
> +// object.
> +extern bool
> +AddDateTimeFormatConstructor(JSContext* cx, JS::Handle<JSObject*> intl);

This wants renaming consistent with the renaming mentioned elsewhere.

::: js/src/tests/Intl/DateTimeFormat/mozExtensions.js:14
(Diff revision 5)
> +let mozIntl = {};
> +addIntlExtras(mozIntl);
> +
> +// Pattern
> +
> +var dtf = new Intl.DateTimeFormat("en-US", {pattern: "HH:mm MM/dd/YYYY"});

I somewhat share anba's worry about pattern validation.  This *must* be ironed out before this can land.

However: maybe passing in bad patterns just gets semi-garbage output out the other side?  If that's the case, that's landable -- but possibly not standardizable.  For this to ever be standardized, a precise algorithm describing pattern syntax is going to have to be given.

But how does pattern syntax work, exactly, if the pattern has grammatically-sensitive parts in it?  Is this what the `false` argument to `udat_toPattern` ensures will never happen, so we'll always get something that's a string with raw substitutions in it?  If so, it seems to me that trying to overload this into `DateTimeFormat` -- which produces locale-sensitive output that could conceivably have grammatically-sensitive bits to it -- isn't quite the right thing to do.  It might be preferable to have another constructor to handle this, whose name somehow makes clear that it's ignoring these sorts of fine-grained locale details.

::: js/src/tests/Intl/DateTimeFormat/mozExtensions.js:29
(Diff revision 5)
> +
> +var mozDtf = new mozIntl.DateTimeFormat("en-US", {dateStyle: 'long'});
> +assertEq(mozDtf.resolvedOptions().dateStyle, 'long');
> +assertEq(mozDtf.resolvedOptions().hasOwnProperty('year'), true);
> +assertEq(mozDtf.resolvedOptions().hasOwnProperty('month'), true);
> +assertEq(mozDtf.resolvedOptions().hasOwnProperty('day'), true);

I note that you're not testing that resolved options contains/doesn't contain a `pattern` property for these date/time style cases.  That seems to lend credence to my sense that in such cases, a `pattern` should not be exposed.
Attachment #8831044 - Flags: review?(jwalden+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8831044 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8837830 [details]
Bug 1329904 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/112818/#review115782

::: js/src/builtin/Intl.h:423
(Diff revision 3)
> + * The function takes four arguments:
> + *
> + *   locale
> + *     BCP47 compliant locale string
> + *   dateStyle
> + *     A string with values: full or long or medium or short

dateStyle/timeStyle need to document that `undefined` is an acceptable value to pass, and what behavior ensues.

::: js/src/builtin/Intl.h:429
(Diff revision 3)
> + *   timeStyle
> + *     A string with values: full or long or medium or short
> + *   timeZone
> + *     IANA time zone name
> + *
> + * date and time style categories map to CLDR time/date standard format patterns

Capitalize/add period for complete sentence.

::: js/src/builtin/Intl.cpp:15
(Diff revision 3)
>   */
>  
>  #include "builtin/Intl.h"
>  
>  #include "mozilla/Casting.h"
> +#include "mozilla/Maybe.h"

This (and the `using mozilla::Maybe;`) isn't necessary when you don't actually add a `Maybe` use -- I was speaking in the hypothetical situation where you were doing so, but it turned out that wasn't needed in this case.

::: js/src/builtin/Intl.cpp:830
(Diff revision 3)
>  }
>  
>  static bool
>  LegacyIntlInitialize(JSContext* cx, HandleObject obj, Handle<PropertyName*> initializer,
>                       HandleValue thisValue, HandleValue locales, HandleValue options,
> -                     MutableHandleValue result)
> +                     bool mozExtensions, MutableHandleValue result)

This argument should be a `DateTimeFormatOptions` enum.

::: js/src/builtin/Intl.cpp:2508
(Diff revision 3)
> +{
> +    Handle<GlobalObject*> global = cx->global();
> +
> +    RootedObject mozDateTimeFormatProto(cx), mozDateTimeFormat(cx);
> +    mozDateTimeFormatProto =
> +        CreateDateTimeFormatPrototype(cx, intl, global, &mozDateTimeFormat, DateTimeFormatOptions::EnableMozExtensions);

Just

    JSObject* mozDateTimeFormatProto = CDTFP(...);
    return mozDateTimeFormatProto != nullptr;

No need for a root if the object's basically not going to be used except for non-nullness.

::: js/src/builtin/Intl.cpp:3103
(Diff revision 3)
> +
> +    UDateFormatStyle dateStyle = UDAT_NONE;
> +    UDateFormatStyle timeStyle = UDAT_NONE;
> +
> +    if (args[1].isString()) {
> +        JSLinearString* dateStyleStr = args[1].toString()->ensureLinear(cx);

This requires a null-check.

::: js/src/builtin/Intl.cpp:3105
(Diff revision 3)
> +    UDateFormatStyle timeStyle = UDAT_NONE;
> +
> +    if (args[1].isString()) {
> +        JSLinearString* dateStyleStr = args[1].toString()->ensureLinear(cx);
> +
> +        if (StringEqualsAscii(dateStyleStr, "full")) {

Because all these if-elses in this series are all one line, they shouldn't be braced.

::: js/src/builtin/Intl.cpp:3119
(Diff revision 3)
> +            MOZ_ASSERT_UNREACHABLE("unexpected dateStyle");
> +        }
> +    }
> +
> +    if (args[2].isString()) {
> +        JSLinearString* timeStyleStr = args[2].toString()->ensureLinear(cx);

Null-check.

::: js/src/builtin/Intl.cpp:3121
(Diff revision 3)
> +    }
> +
> +    if (args[2].isString()) {
> +        JSLinearString* timeStyleStr = args[2].toString()->ensureLinear(cx);
> +
> +        if (StringEqualsAscii(timeStyleStr, "full")) {

No bracing, same as above.

::: js/src/builtin/Intl.cpp:3154
(Diff revision 3)
> +
> +    Vector<char16_t, INITIAL_CHAR_BUFFER_SIZE> chars(cx);
> +    if (!chars.resize(INITIAL_CHAR_BUFFER_SIZE))
> +        return false;
> +
> +    int32_t size = udat_toPattern(df, false, Char16ToUChar(chars.begin()), INITIAL_CHAR_BUFFER_SIZE, &status);

Bug 1333844 should have added a helper function to use for this by the next time you touch this, I think.

::: js/src/builtin/Intl.js:2203
(Diff revision 3)
> +    //     mozExtensions: true / false,
> +    //
> +    //
> +    //     // If mozExtensions is true:
> +    //
> +    //     timeStyle: "full" / "long" / "medium" / "short",

Or `undefined`, right?  Same for dateStyle.

Nitpickily, I'd prefer dateStyle be listed first because it's added to the internals object first, farther down.

::: js/src/builtin/Intl.js:2207
(Diff revision 3)
> +    //
> +    //     timeStyle: "full" / "long" / "medium" / "short",
> +    //
> +    //     dateStyle: "full" / "long" / "medium" / "short",
> +    //
> +    //     patternOption: String representing LDML Date Format pattern

Or `undefined`.

::: js/src/builtin/Intl.js:2845
(Diff revision 3)
> +    if (internals.mozExtensions) {
> +        if (internals.dateStyle || internals.timeStyle) {
> +            result.dateStyle = internals.dateStyle;
> +            result.timeStyle = internals.timeStyle;
> +        } else {
> +            result.pattern = internals.pattern;

This must mirror the treatment above, to accurately reveal the resolved options actually used above:

    if (internals.pattern !== undefined) {
        result.pattern = internals.pattern;
    } else if (internals.dateStyle || internals.timeStyle) {
        result.dateStyle = internals.dateStyle;
        result.timeStyle = internals.timeStyle;
    }

::: js/src/jsfriendapi.h:2885
(Diff revision 3)
>  // object.  This function throws if called more than once per realm/global
>  // object.
>  extern bool
>  AddPluralRulesConstructor(JSContext* cx, JS::Handle<JSObject*> intl);
>  
> +// Create and add the Intl.DateTimeFormat constructor function to the provided

...the Intl.MozDateTimeFormat constructor...

::: js/src/jsfriendapi.h:2888
(Diff revision 3)
>  AddPluralRulesConstructor(JSContext* cx, JS::Handle<JSObject*> intl);
>  
> +// Create and add the Intl.DateTimeFormat constructor function to the provided
> +// object.
> +extern bool
> +AddDateTimeFormatConstructor(JSContext* cx, JS::Handle<JSObject*> intl);

AddMozDateTimeFormatConstructor
Attachment #8837830 - Flags: review?(jwalden+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 years ago
Thanks :waldo!

Updated the patch to your feedback, rerequesting r?.

As often, the inter-diff is useless because I had to rebase the bookmark on top of central to get the helper functions and that breaks inter-diff.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

2 years ago
:anba, I need a bit of your magic.

This patch triggers some weird test262 bug in PluralRules and I can't connect the dots between the changes I am making and this error. 

It's easily reproducible localy, so hopefully easy to spot for someone who understands the code.

Can you take a look at this? https://treeherder.mozilla.org/#/jobs?repo=try&revision=105453505e0e
Flags: needinfo?(andrebargull)
Depends on: 1342648
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #32)
> This patch triggers some weird test262 bug in PluralRules and I can't
> connect the dots between the changes I am making and this error. 

The set-up script [1] for the Intl.PluralRules tests calls `addIntlExtras(Intl);`, so when this patch is applied the standard Intl.DateTimeFormat is overwritten with the new mozDateTimeFormat constructor. This will lead to a TypeError when mozDateTimeFormat is new'ed in [2]. I'll prepare a patch for it (bug 1342648).

[1] http://searchfox.org/mozilla-central/rev/c9ccb8ec1f5acda78e6344ffb87aa3a409031e3d/js/src/tests/test262/intl402/PluralRules/shell.js
[2] https://github.com/tc39/test262/blob/098f9ca3de5043c376e0cb68c14acf535ad8d66e/test/intl402/PluralRules/this-not-ignored.js#L15
Flags: needinfo?(andrebargull)

Comment 34

2 years ago
mozreview-review
Comment on attachment 8837830 [details]
Bug 1329904 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/112818/#review116944

r=me with the two comments added/addressed.

::: js/src/builtin/Intl.h:430
(Diff revision 6)
> + *     A string with values: full or long or medium or short, or `undefined`
> + *   timeZone
> + *     IANA time zone name
> + *
> + * Date and time style categories map to CLDR time/date standard
> + * format patterns.

Please add a link to something in ICU/CLDR, or a path to a file in our tree in ICU/CLDR source, that documents these patterns, before you land this.

::: js/src/jsfriendapi.h:2892
(Diff revision 6)
>  // object.
>  extern bool
>  AddPluralRulesConstructor(JSContext* cx, JS::Handle<JSObject*> intl);
>  
> +// Create and add the Intl.MozDateTimeFormat constructor function to the provided
> +// object.

Please add a comment here like so:

"""
This custom date/time formatter constructor gives users the ability to specify a custom format pattern.  This pattern is passed *directly* to ICU with NO SYNTAX PARSING OR VALIDATION WHATSOEVER.  ICU appears to have a a modicum of testing of this, and it won't fall over completely if passed bad input.  But the current behavior is entirely under-specified and emphatically not shippable on the web, and it *must* be fixed before this functionality can be exposed in the real world.  (There are also some questions about whether the format exposed here is the *right* one to standardize, that will also need to be resolved to ship this.)
"""

(To elaborate on that last bit: the traditional way to have patterns/formats like this would be something with an escape-sequence mechanism, so that the pattern can contain arbitrary non-significant characters.  e.g. look at template literal syntax with interpolations -- but that still let you put any characters in the string.  There appears to be no such mechanism here, or at least *at a minimum* the mechanism is non-obvious.  And whether "MMMM" and similar are good ways to request particular functionality, and JS-y, is an important question as well.  I'm not convinced these are the right thing to use, but *some* forward progress seems desirable over holding this back from purely-internal use for pattern-syntax considerations.)

Just to double-check, we *are* only going to be using this in trusted code, right?  And this *will not* be exposed to web code, and it won't be exposed in some easily-packaged form to extensions (and certainly not to web extensions), and it will *only* be exposed in chrome code that's jumped through significant hoops, right?
Attachment #8837830 - Flags: review?(jwalden+bmo) → review+
Attachment #8831066 - Attachment is obsolete: true

Comment 35

2 years ago
mozreview-review
Comment on attachment 8837830 [details]
Bug 1329904 - Introduce mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/112818/#review117192

::: js/src/builtin/Intl.cpp:844
(Diff revision 6)
>  LegacyIntlInitialize(JSContext* cx, HandleObject obj, Handle<PropertyName*> initializer,
>                       HandleValue thisValue, HandleValue locales, HandleValue options,
> -                     MutableHandleValue result)
> +                     DateTimeFormatOptions dtfOptions, MutableHandleValue result)
>  {
> -    FixedInvokeArgs<4> args(cx);
> +    InvokeArgs args(cx);
> +    if (!args.init(cx, 5))

This should be changed back FixedInvokeArgs if the number of arguments is fixed:
```
FixedInvokeArgs<5> args(cx);
```
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)
> (To elaborate on that last bit: the traditional way to have patterns/formats
> like this would be something with an escape-sequence mechanism, so that the
> pattern can contain arbitrary non-significant characters.  e.g. look at
> template literal syntax with interpolations -- but that still let you put
> any characters in the string.  There appears to be no such mechanism here,
> or at least *at a minimum* the mechanism is non-obvious. [...] )

Anything enclosed in single quotes should be left as is.
(Assignee)

Comment 37

2 years ago
Thanks Jeff, Andre!

I applied your feedback, and will land it if the Try allows tomorrow, but a few comments to your comments:

>   (There are also some questions about whether the format exposed here is the *right* one to standardize, that will also need to be resolved to ship this.)

LDML pattern is the right date/time pattern format to standardize. If we decide to expose a pattern field, it will be based on the LDML definition and syntax.

I assume that what you mean by this comment is that we have an ongoing conversation about whether we will expose pattern, or skeleton.

> There appears to be no such mechanism here

As Andre pointed out, single quoted strings are that.

> Just to double-check, we *are* only going to be using this in trusted code, right?  And this *will not* be exposed to web code, and it won't be exposed in some easily-packaged form to extensions (and certainly not to web extensions), and it will *only* be exposed in chrome code that's jumped through significant hoops, right?

This code is going to be used only by our internal MozIntl API which is chrome-only. What's more, the API will not expose this feature to our developers either.

Instead, the developers will be able to specify "dateStyle" and "timeStyle" parameters, and mozIntl will attempt to use OSPreferences API (internal) to retrieve the pattern from the OS.
This pattern, retrieved either from the OS (so, the validation of the pattern will be done by Mac/Windows/Android) or from ICU directly (in case of Gtk/Linux) is going to be passed to MozDateTimeFormat.

This is the right way to give us ability to adapt our date/time strings to user regional preferences in their OS, while getting more of the ECMA402 based Intl.DateTimeFormat to replace and deprecate our proprietary date/time formatting APIs.
And I do believe that since the only source of those patterns will be pattern-specific-retrieving APIs of the OS, or ICU, we should be safe as no user will write the pattern by hand.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41f3d9a17672
Introduce mozIntl.DateTimeFormat. r=Waldo

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41f3d9a17672
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1324133
You need to log in before you can comment on or make changes to this bug.