Summary: | [INTL] Implement Intl.getCanonicalLocales | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy VanWagoner <andy> | ||||||||||
Component: | JavaScriptCore | Assignee: | Andy VanWagoner <andy> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, keith_miller, mark.lam, msaboff, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Andy VanWagoner
2016-09-29 21:24:40 PDT
Created attachment 290292 [details]
Patch
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. Created attachment 290330 [details]
Patch
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) { ... Created attachment 290751 [details]
Patch
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 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". Created attachment 290762 [details]
Patch for landing
Comment on attachment 290762 [details] Patch for landing Clearing flags on attachment: 290762 Committed r206837: <http://trac.webkit.org/changeset/206837> All reviewed patches have been landed. Closing bug. |