Bug 227830

Summary: [JSC] Intl Locale Info
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch ross.kirsling: review+

Description Yusuke Suzuki 2021-07-09 01:35:52 PDT 
...
Comment 1 Yusuke Suzuki 2021-07-10 10:13:37 PDT 
Created attachment 433259 [details]
Patch
Comment 2 Yusuke Suzuki 2021-07-10 10:43:22 PDT 
https://github.com/tc39/proposal-intl-locale-info
Comment 3 Yusuke Suzuki 2021-07-10 14:17:54 PDT 
Created attachment 433274 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-07-16 01:36:20 PDT 
<rdar://problem/80674634>
Comment 5 Yusuke Suzuki 2021-08-06 23:07:44 PDT 
Created attachment 435117 [details]
Patch
Comment 6 Yusuke Suzuki 2021-08-20 00:55:42 PDT 
Created attachment 435948 [details]
Patch
Comment 7 Ross Kirsling 2021-08-20 15:18:12 PDT 
Comment on attachment 435948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435948&action=review

r=me with comments

> Source/JavaScriptCore/runtime/IntlLocale.cpp:540
> +static inline JSArray* createArrayFromStringVector(JSGlobalObject* globalObject, Vector<String, 1>&& elements)

Heh, I'm slightly surprised we don't already have something like this.

> Source/JavaScriptCore/runtime/IntlLocale.cpp:581
> +        // Ensure aliases used in language tag are allowed.

Maybe put in a shared location so that IntlDateTimeFormat doesn't have to store this knowledge separately?

> Source/JavaScriptCore/runtime/IntlLocale.cpp:627
> +        // Map keyword values to BCP 47 equivalents.

Maybe put in a shared location so that IntlCollator doesn't have to store this knowledge separately?

> Source/JavaScriptCore/runtime/IntlLocale.cpp:679
> +        unsigned count = 1;

Unused variable? (Seems like it's incremented but never checked.)

> Source/JavaScriptCore/runtime/IntlLocale.cpp:826
> +        case UCAL_WEEKEND:
> +            return UCAL_WEEKEND;
> +        default:
> +            return UCAL_WEEKEND;
> +        }
> +        return UCAL_WEEKEND;

Why not just a single `return UCAL_WEEKEND;` after the `default:`, instead of three?
Comment 8 Yusuke Suzuki 2021-08-21 06:18:38 PDT 
Comment on attachment 435948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435948&action=review

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:826
>> +        return UCAL_WEEKEND;
> 
> Why not just a single `return UCAL_WEEKEND;` after the `default:`, instead of three?

Removed the last one. But I think having

        case UCAL_WEEKEND_CEASE:
        case UCAL_WEEKEND:

is good for readability rather than using `default`, in particular when a new UCalendarWeekdayType is added to ICU.
Comment 9 Yusuke Suzuki 2021-08-21 07:24:15 PDT 
Comment on attachment 435948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435948&action=review

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:581
>> +        // Ensure aliases used in language tag are allowed.
> 
> Maybe put in a shared location so that IntlDateTimeFormat doesn't have to store this knowledge separately?

Done.

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:627
>> +        // Map keyword values to BCP 47 equivalents.
> 
> Maybe put in a shared location so that IntlCollator doesn't have to store this knowledge separately?

Done

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:679
>> +        unsigned count = 1;
> 
> Unused variable? (Seems like it's incremented but never checked.)

Removed.
Comment 10 Yusuke Suzuki 2021-08-21 07:33:31 PDT 
Committed r281374 (240789@main): <https://commits.webkit.org/240789@main>