Closed Bug 1260858 Opened 4 years ago Closed 3 years ago

Update the DateTimeFormat.prototype.formatToParts to the final spec

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

In bug 1216150 we introduced `formatToParts` method for DateTimeFormat. We need it for a lot of UI/UX code in FxOS/CD.

We since then moved forward with standardization, we've reached Stage 3 and we're on the path to Stage 4.

We have several implementations including Gecko, Intl.js and we have a good reason to hope to see it relatively soon in Chrome.

There are several cosmetic changes to the spec since we implemented it and before we expose it to the Web we'll need to make sure that the spec is final and apply the changes to our code.

This bug will track that.

So far we know of three changes:

1) formatToParts is not bound
2) `dayperiod` token name changed to `dayPeriod` for compatibility with `timeZoneName`
3) the `separator` label changed to `literal`.

I'd like to give it a month or two to see if any other things come up between Stage 3 and Stage 4, and then will write a patch to align our implementation with the spec and ask to expose the feature to the Web.
See Also: → 1216150
Assignee: nobody → gandalf
Attached patch patch (obsolete) — Splinter Review
Ok, I think we're ready to finalize the work. No changes has been proposed for some time, and I'm going to propose moving the proposal to Stage 4 soon, so let's update the implementation.
Attachment #8754571 - Flags: review?(jwalden+bmo)
Comment on attachment 8754571 [details] [diff] [review]
patch

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

Where's the latest spec language for all this?  It's not yet in tc39, presumably because it's not officially part of the standard quite yet.  And I'm not 100% certain which user repo I should be looking at.  With some assumptions made, tho, this looks well on the right track.

::: js/src/builtin/Intl.cpp
@@ +1764,2 @@
>      // If the still-experimental DateTimeFormat.prototype.formatToParts method
>      // is enabled, also add its getter.

Comment needs changing.

::: js/src/builtin/Intl.js
@@ -2747,5 @@
>  
>  
> -function Intl_DateTimeFormat_formatToParts_get() {
> -    // Check "this DateTimeFormat object" per introduction of section 12.3.
> -    var internals = getDateTimeFormatInternals(this, "formatToParts");

Eliminating the getter-caching/binding, also eliminates this check.  If I follow into Intl.cpp, js::intl_FormatDateTime right off asserts that args[0] is an object.  That's no longer true without this check, e.g.:

  Intl.DateTimeFormat.prototype.formatToParts.call(0);

I believe to fix this you'll need to add

  getDateTimeFormatInternals(this, "formatToParts");

to the top of the formatToParts function to do the necessary this-testing.  We'll also want tests of passing various primitive values, and non-DateTimeFormat objects, as |this|, as the above does.
Attachment #8754571 - Flags: review?(jwalden+bmo) → review-
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Where's the latest spec language for all this?  

https://tc39.github.io/ecma402/#sec-intl.getcanonicallocales
Ugh, sorry, wrong bug. And my patch shouldn't have remove the experimental guard on the feature. Updating.
Attached patch patch v2 (obsolete) — Splinter Review
Updated version to your feedback.

The proposal has not yet been merged into master. But it got r+ and is just waiting for 3rd edition release.

The patch for spec is here: https://github.com/tc39/ecma402/pull/64
Attachment #8754571 - Attachment is obsolete: true
Attachment #8755698 - Flags: review?(jwalden+bmo)
Comment on attachment 8755698 [details] [diff] [review]
patch v2

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

::: js/src/builtin/Intl.js
@@ +2742,1 @@
>      // Steps 1.a.i-ii

Add a blank line before this "// Steps" comment -- separate distinct operations in the algorithm deserve separate blocks of code.
Attachment #8755698 - Flags: review?(jwalden+bmo) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attached patch patch, v3Splinter Review
carry-over r+
Attachment #8755698 - Attachment is obsolete: true
Attachment #8759881 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b4391bfdd4
"Update the DateTimeFormat.prototype.formatToParts to the final spec". r=waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5b4391bfdd4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> I'd like to give it a month or two to see if any other things come up
> between Stage 3 and Stage 4, and then will write a patch to align our
> implementation with the spec and ask to expose the feature to the Web.

Looks like the missing pieces were implemented here, so I've filed bug 1289340 to expose this feature to the Web.

Sebastian
You need to log in before you can comment on or make changes to this bug.