Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Can't log into MozReview:
---
Something broke! (Error 500)

It appears something broke when you tried to go to here. This is either a bug in Review Board or a server configuration error. Please report this to your administrator. 
---



Review comments:
---
RelativeTimeFormat.js, resolveRelativeTimeFormatInternals function

- Nit: Steps updates to "Step 17." and "Step 18". don't match the current spec text, e.g. the spec calls ResolveLocale in step 10. 


RelativeTimeFormat.cpp

- Should we rename |RelativeTimeType| to match the new option values?  
---



Spec change comments:
---
I kind of wonder if the "numeric" option should have been spec'ed as a boolean, similar to the "numeric" option for Intl.Collator. The "auto" and "always" names feel a bit alien to me, but maybe I just need to get used to them.
---
Attachment #8949539 - Flags: review?(andrebargull) → review+
Comment hidden (mozreview-request)
Comment on attachment 8949539 [details]
Bug 1436841 - Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto.

Log-in to MozReview is still not possible for me.
Attachment #8949539 - Flags: review?(andrebargull) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8949539 [details]
Bug 1436841 - Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto.

https://reviewboard.mozilla.org/r/218886/#review224636
(In reply to André Bargull [:anba] from comment #4)
> Log-in to MozReview is still not possible for me.

Because I'm dumb and didn't enable cookies for reviewboard.mozilla.org. :-P
> I kind of wonder if the "numeric" option should have been spec'ed as a boolean, similar to the "numeric" option for Intl.Collator. The "auto" and "always" names feel a bit alien to me, but maybe I just need to get used to them.

We did discuss it, but the value of being able to add third or fourth option in the future seems to be the reason to not use boolean here.
Ah, I see. Hopefully generic terms like "auto" and "always" won't create an obstacle if there's ever another option added to "numeric".
(In reply to André Bargull [:anba] from comment #2)
> Can't log into MozReview:
> ---
> Something broke! (Error 500)
> 
> It appears something broke when you tried to go to here. This is either a
> bug in Review Board or a server configuration error. Please report this to
> your administrator. 
> ---
> 
> 
> 
> Review comments:
> ---
> RelativeTimeFormat.js, resolveRelativeTimeFormatInternals function
> 
> - Nit: Steps updates to "Step 17." and "Step 18". don't match the current
> spec text, e.g. the spec calls ResolveLocale in step 10. 

Oh, good catch. Ok, I can update ResolveLocale to step 10, but not sure what to rename current "Step 18." to as I don't think it has a direct correspondent step in the spec?
> 
> RelativeTimeFormat.cpp
> 
> - Should we rename |RelativeTimeType| to match the new option values?  

I decided to keep it because in C++ we do use Type Text|Numeric.
Is that ok with you?

btw. the split into dir per API in Intl is amazing. It's so much easier to write patches like this. Kudos to :waldo!
Flags: needinfo?(andrebargull)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Oh, good catch. Ok, I can update ResolveLocale to step 10, but not sure what
> to rename current "Step 18." to as I don't think it has a direct
> correspondent step in the spec?

I think I'd go with:
---
// Step 11.
internalProps.locale = r.locale;

// Step 14.
internalProps.style = lazyRelativeTimeFormatData.style;

// Step 16.
internalProps.numeric = lazyRelativeTimeFormatData.numeric;
---

In our implementation step 14 and step 16 can be used as an annotation in InitializeRelativeTimeFormat and resolveRelativeTimeFormatInternals.


> > 
> > RelativeTimeFormat.cpp
> > 
> > - Should we rename |RelativeTimeType| to match the new option values?  
> 
> I decided to keep it because in C++ we do use Type Text|Numeric.
> Is that ok with you?

We could change RelateTimeType to:
---
enum class RelativeTimeNumeric
{
    /**
     * Only strings with numeric components like `1 day ago`.
     */
    Always,

    /**
     * Natural-language strings like `yesterday` when possible,
     * otherwise strings with numeric components as in `7 months ago`.
     */
    Auto,
};
---
Flags: needinfo?(andrebargull)
Comment hidden (mozreview-request)
Thanks anba! Pushed an updated patch to try

Comment 13

a year ago
mozreview-review
Comment on attachment 8949539 [details]
Bug 1436841 - Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto.

https://reviewboard.mozilla.org/r/218886/#review224642

::: js/src/builtin/intl/RelativeTimeFormat.cpp:343
(Diff revisions 2 - 3)
>          }
>      }
>  
>      JSString* str =
> -        CallICU(cx, [rtf, t, relDateTimeUnit, relDateTimeType](UChar* chars, int32_t size,
> +        CallICU(cx, [rtf, t, relDateTimeUnit, relDateTimeNumeric](UChar* chars, int32_t size,
>                                                                 UErrorCode* status)

Nit: indent needs to be fixed, so UErrorCode aligns with UChar.
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b24e1b9770b
Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto. r=anba

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b24e1b9770b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.