Bug 227830 - [JSC] Intl Locale Info
Summary: [JSC] Intl Locale Info
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-09 01:35 PDT by Yusuke Suzuki
Modified: 2021-08-21 07:33 PDT (History)
8 users (show)

See Also:

Attachments
Patch (24.95 KB, patch)
2021-07-10 10:13 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (25.85 KB, patch)
2021-07-10 14:17 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (26.35 KB, patch)
2021-08-06 23:07 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (32.54 KB, patch)
2021-08-20 00:55 PDT, Yusuke Suzuki 		ross.kirsling: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>