Bug 162768 – [INTL] Implement Intl.getCanonicalLocales

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy VanWagoner]

[INTL] Implement Intl.getCanonicalLocales

[Andy VanWagoner]

Created attachment 290292 [details]
Patch

[Geoffrey Garen]

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.

[Andy VanWagoner]

Created attachment 290330 [details]
Patch

[Benjamin Poulain]

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) {
        ...

[Andy VanWagoner]

Created attachment 290751 [details]
Patch

[Andy VanWagoner]

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.

[Benjamin Poulain]

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".

[Benjamin Poulain]

Created attachment 290762 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290762 [details]
Patch for landing

Clearing flags on attachment: 290762

Committed r206837: <http://trac.webkit.org/changeset/206837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.