Closed Bug 1436841 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript: Internationalization API, enhancement)

Product:
Core
Component:
JavaScript: Internationalization API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Details

Attachments

(1 file)

Bug 1436841 - Update Intl.RelativeTimeFormat option type: numeric/text to numeric always/auto.
59 bytes, text/x-review-board-request
anba
: review+
Details
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 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 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)
Thanks anba! Pushed an updated patch to try
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.
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
https://hg.mozilla.org/mozilla-central/rev/3b24e1b9770b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.