Bug 162768

Summary: [INTL] Implement Intl.getCanonicalLocales
Product: WebKit Reporter: Andy VanWagoner <andy>
Component: JavaScriptCoreAssignee: Andy VanWagoner <andy>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Andy VanWagoner 2016-09-29 21:24:40 PDT
[INTL] Implement Intl.getCanonicalLocales
Comment 1 Andy VanWagoner 2016-09-29 21:33:46 PDT
Created attachment 290292 [details]
Patch
Comment 2 Geoffrey Garen 2016-09-29 22:41:10 PDT
Comment on attachment 290292 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/IntlObject.cpp:59
> +static EncodedJSValue JSC_HOST_CALL IntlObjectFuncGetCanonicalLocales(ExecState*);

First letter should be lower case.
Comment 3 Andy VanWagoner 2016-09-30 07:07:59 PDT
Created attachment 290330 [details]
Patch
Comment 4 Benjamin Poulain 2016-10-05 12:29:52 PDT
Comment on attachment 290330 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:1032
> +    JSArray* localeArray = JSArray::tryCreateUninitialized(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), 0);

You could create an array of shape ArrayWithContiguous and the initial size already set to localeList.size().
That way you avoid growing the butterfly dynamically on push().

> Source/JavaScriptCore/runtime/IntlObject.cpp:1040
> +    auto len = localeList.size();
> +    for (size_t i = 0; i < len; ++i) {
> +        localeArray->push(state, jsString(state, localeList[i]));

If you use push instead of setting the element at index, you can use the for-in loop of C++11:
    for (const String& locale : localeList) {
        ...
Comment 5 Andy VanWagoner 2016-10-05 14:51:28 PDT
Created attachment 290751 [details]
Patch
Comment 6 Andy VanWagoner 2016-10-05 14:54:04 PDT
Comment on attachment 290330 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlObject.cpp:1032
>> +    JSArray* localeArray = JSArray::tryCreateUninitialized(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), 0);
> 
> You could create an array of shape ArrayWithContiguous and the initial size already set to localeList.size().
> That way you avoid growing the butterfly dynamically on push().

done.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:1040
>> +        localeArray->push(state, jsString(state, localeList[i]));
> 
> If you use push instead of setting the element at index, you can use the for-in loop of C++11:
>     for (const String& locale : localeList) {
>         ...

Setting the initial size required this to switch to initializeIndex, which needs the index number.
Comment 7 Benjamin Poulain 2016-10-05 15:01:02 PDT
Comment on attachment 290751 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:118
> +    putDirectNativeFunction(vm, globalObject, Identifier::fromString(&vm, ASCIILiteral("getCanonicalLocales")), 1, intlObjectFuncGetCanonicalLocales, NoIntrinsic, DontEnum);

Identifier has a constructor taking a static string, you do not have to use ASCIILiteral with them.

> Source/JavaScriptCore/runtime/IntlObject.cpp:1039
> +    auto len = localeList.size();
> +    for (size_t i = 0; i < len; ++i) {

In WebKit, we avoid shortening names. "len" should be "length".
Comment 8 Benjamin Poulain 2016-10-05 16:36:28 PDT
Created attachment 290762 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2016-10-05 17:09:50 PDT
Comment on attachment 290762 [details]
Patch for landing

Clearing flags on attachment: 290762

Committed r206837: <http://trac.webkit.org/changeset/206837>
Comment 10 WebKit Commit Bot 2016-10-05 17:09:54 PDT
All reviewed patches have been landed.  Closing bug.