Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239523002:
Expose getCanonicalLocales() for Intl object (Closed)
|
Created:
4 years, 4 months ago by jungshik at Google Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionExpose getCanonicalLocales() for Intl object.
Also add a test for the return object of getCanonicalLocaleList().
See https://github.com/tc39/test262/issues/745 for more details.
BUG=v8:5012
TEST=test262/intl402/Intl/getCanonicalLocales/*
TEST=intl/general/getCanonicalLocales
Committed: https://crrev.com/520f38fce79bf6370f46c4d3326e52bbd962030c
Cr-Commit-Position: refs/heads/master@{#38733}
Patch Set 1 #Patch Set 2 : fix a case with negative length #
Total comments: 1
Patch Set 3 : disable failing tests with comments #
Total comments: 1
Patch Set 4 : do not freeze array #
Total comments: 1
Patch Set 5 : add a test for the mutability #Patch Set 6 : make individual elements frozen, but let the result array be expandable #Patch Set 7 : do not use boolean #Patch Set 8 : go back to PS 6, but explicitly set the array length regardless of expandability #Patch Set 9 : fix a typo in test262 bug reference #Patch Set 10 : return array is mutable #
Total comments: 1
Patch Set 11 : check if returned object is an array #
Messages
Total messages: 54 (31 generated)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jshin@chromium.org changed reviewers: + littledan@chromium.org
PTAL. Thanks https://codereview.chromium.org/2239523002/diff/20001/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2239523002/diff/20001/src/js/i18n.js#newcode781 src/js/i18n.js:781: %_Call(ArrayPush, seen, tag); intl402/Intl/getCanonicalLocales/overriden-push.js tests if 'push' is used by overriding Array.prototype.push. However, v8 doesn't throw because we're using InternalArray. So, there should not be an issue (even though we don't follow the spec to its letters: build a list and convert to an array at the end).
https://codereview.chromium.org/2239523002/diff/40001/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2239523002/diff/40001/src/js/i18n.js#newcode906 src/js/i18n.js:906: return initializeLocaleList(locales); This outputs a frozen Array, but the spec indicates a full, ordinary mutable Array. In addition to fixing the bug here, it'd be nice to file a bug against test262 to ask for tests of this property, given that what you've done here is a pretty natural implementation strategy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/12382) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dan, PS #4 fixed that. can you take a look? Thanks
Could you make a local test to tide us over for this patch until the test262 tests come in? https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js#newcode796 src/js/i18n.js:796: return freezeArray(canonicalizeLocaleList(locales)); This seems to make an extra copy of the array, which is slower but probably OK as these functions are not performance-sensitive.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/12 18:59:19, Dan Ehrenberg wrote: > Could you make a local test to tide us over for this patch until the test262 > tests come in? Added, but given the comments in the github bug, I need to change it again. > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js > File src/js/i18n.js (right): > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js#newcode796 > src/js/i18n.js:796: return freezeArray(canonicalizeLocaleList(locales)); > This seems to make an extra copy of the array, which is slower but probably OK > as these functions are not performance-sensitive. Nonetheless, I made it better. However, given your comment, it looks like I need to freeze individual elements while keeping the array expandable. Is it based on the interpretation of https://tc39.github.io/ecma262/#sec-createarrayfromlist ?
On 2016/08/12 at 22:22:41, jshin wrote: > On 2016/08/12 18:59:19, Dan Ehrenberg wrote: > > Could you make a local test to tide us over for this patch until the test262 > > tests come in? > > Added, but given the comments in the github bug, I need to change it again. > > > > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js > > File src/js/i18n.js (right): > > > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js#newcode796 > > src/js/i18n.js:796: return freezeArray(canonicalizeLocaleList(locales)); > > This seems to make an extra copy of the array, which is slower but probably OK > > as these functions are not performance-sensitive. > > Nonetheless, I made it better. > > However, given your comment, it looks like I need to freeze individual elements while keeping the array expandable. Is it based on the interpretation of https://tc39.github.io/ecma262/#sec-createarrayfromlist ? No, it's not in this case but in the other path that you need to freeze the individual elements (as we already do) but leave the length writable. This is deleting one line from freezeArray.
On 2016/08/12 22:28:48, Dan Ehrenberg wrote: > On 2016/08/12 at 22:22:41, jshin wrote: > > On 2016/08/12 18:59:19, Dan Ehrenberg wrote: > > > Could you make a local test to tide us over for this patch until the test262 > > > tests come in? > > > > Added, but given the comments in the github bug, I need to change it again. > > > > > > > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js > > > File src/js/i18n.js (right): > > > > > > > https://codereview.chromium.org/2239523002/diff/60001/src/js/i18n.js#newcode796 > > > src/js/i18n.js:796: return freezeArray(canonicalizeLocaleList(locales)); > > > This seems to make an extra copy of the array, which is slower but probably > OK > > > as these functions are not performance-sensitive. > > > > Nonetheless, I made it better. > > > > However, given your comment, it looks like I need to freeze individual > elements while keeping the array expandable. Is it based on the interpretation > of https://tc39.github.io/ecma262/#sec-createarrayfromlist ? > > No, it's not in this case but in the other path that you need to freeze the > individual elements (as we already do) but leave the length writable. This is > deleting one line from freezeArray. Yes, that's exactly what I meant that I need to do given your comment in the github (PS 5 was written before I saw your comment).
lgtm
As a matter of style, you may want to just return the InternalArray and then freezeArray/makeArray it rather than passing the boolean parameter, but up to you.
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/13 16:20:31, Dan Ehrenberg wrote: . Oops. Sorry for the confusion. PS #5 does not yet take care of your github comment (individual elements should be frozen while the array being expandable) because the return object of getCanonicalLocales() has elements that are NOT frozen. PS #6 does that. The return object is an array whose elements are frozen but the array is still expandable. I also updated the change both in this CL and my github pull request to test262. Can you take another look? Thanks > As a matter of style, you may want to just return the InternalArray and then > freezeArray/makeArray it rather than passing the boolean parameter, but up to > you I did that because there are multiple callers that I think would benefit from the frozen array. If I freeze internal array at all those call sites, it'd be more code. Anyway, can you take a look?
On 2016/08/14 07:25:52, jungshik at google wrote: > > As a matter of style, you may want to just return the InternalArray and then > > freezeArray/makeArray it rather than passing the boolean parameter, but up to > > you Ignore the following reply. It's off the mark (I was mistaken about # of callers). > I did that because there are multiple callers that I think would benefit from > the frozen array. If I freeze internal array at all those call sites, it'd be > more code. Anyway, can you take a look? PS #7 does what you suggested. My worry is that there's one more copy made for internal use cases of canonilcaizeLocaleList (whenever locale list is an argument, it's subject to this operation). What do you think?
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose getCanonicalLocales() for Intl object BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* ========== to ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dan, can you answer caridy's question at https://github.com/tc39/test262/pull/746 ? Thanks
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Based on the discussion at github / test262, I updated the CL to return a mutable array(elements as well as the array itself). PTAL Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2239523002/diff/180001/src/js/i18n.js File src/js/i18n.js (right): https://codereview.chromium.org/2239523002/diff/180001/src/js/i18n.js#newcode570 src/js/i18n.js:570: * When |expandable| is true, the result array can be expanded. Ick. I'll get rid of this line.
littledan@chromium.org changed reviewers: + adamk@chromium.org
This patch both implements and ships the feature, rather than guarding it behind a flag. Although this feature is very simple and already part of an established standard, strictly speaking, we should actually follow the feature addition process at https://github.com/v8/v8/wiki/Feature%20Launch%20Process . Typically, that means implementing it behind a flag, and sending out an intent to ship email to v8-dev. Maybe we could skip the flag as this is so trivial, but we should probably at least send such an email when shipping a feature.
Description was changed from ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* ========== to ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* TEST=intl/general/getCanonicalLocales ==========
On 2016/08/16 15:08:49, Dan Ehrenberg wrote: > This patch both implements and ships the feature, rather than guarding it behind > a flag. Although this feature is very simple and already part of an established > standard, strictly speaking, we should actually follow the feature addition > process at https://github.com/v8/v8/wiki/Feature%20Launch%20Process . Typically, > that means implementing it behind a flag, and sending out an intent to ship > email to v8-dev. Maybe we could skip the flag as this is so trivial, but we > should probably at least send such an email when shipping a feature. Ok. I tried to post (groups web ui) or send (email) to v8-users group/list (the doc says that it is v8-users rather than v8-dev), but my post/email (sent/posted from my chromium.org account ) just went to /dev/null. None of them (I tried 3 times) showed up in v8-users. (I did subscribe to the group with my chromium.org account). Very strange !
On 2016/08/16 18:27:40, jungshik at google wrote: > On 2016/08/16 15:08:49, Dan Ehrenberg wrote: > > This patch both implements and ships the feature, rather than guarding it > behind > > a flag. Although this feature is very simple and already part of an > established > > standard, strictly speaking, we should actually follow the feature addition > > process at https://github.com/v8/v8/wiki/Feature%20Launch%20Process . > Typically, > > that means implementing it behind a flag, and sending out an intent to ship > > email to v8-dev. Maybe we could skip the flag as this is so trivial, but we > > should probably at least send such an email when shipping a feature. > > Ok. I tried to post (groups web ui) or send (email) to v8-users group/list (the > doc says that it is v8-users rather than v8-dev), but my post/email (sent/posted > from my http://chromium.org account ) > just went to /dev/null. None of them (I tried 3 times) showed up in v8-users. (I > did subscribe to the group with my http://chromium.org account). > > Very strange ! Contacted the owner of v8-users. Turned out that my post/mail was misclassified as spam. The owner manually posted it to the group.
On 2016/08/17 21:55:13, jungshik at google wrote: > On 2016/08/16 18:27:40, jungshik at google wrote: > > On 2016/08/16 15:08:49, Dan Ehrenberg wrote: > > > This patch both implements and ships the feature, rather than guarding it > > behind > > > a flag. Although this feature is very simple and already part of an > > established > > > standard, strictly speaking, we should actually follow the feature addition > > > process at https://github.com/v8/v8/wiki/Feature%20Launch%20Process . > > Typically, > > > that means implementing it behind a flag, and sending out an intent to ship > > > email to v8-dev. Maybe we could skip the flag as this is so trivial, but we > > > should probably at least send such an email when shipping a feature. > > > > Ok. I tried to post (groups web ui) or send (email) to v8-users group/list > (the > > doc says that it is v8-users rather than v8-dev), but my post/email > (sent/posted > > from my http://chromium.org account ) > > just went to /dev/null. None of them (I tried 3 times) showed up in v8-users. > (I > > did subscribe to the group with my http://chromium.org account). > > > > Very strange ! > > Contacted the owner of v8-users. Turned out that my post/mail was misclassified > as spam. The owner > manually posted it to the group. No response so far. (it's so trivial that nobody cares ? ). Will wait for a while.
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/18 07:45:11, jungshik at google wrote: > On 2016/08/17 21:55:13, jungshik at google wrote: > > On 2016/08/16 18:27:40, jungshik at google wrote: > > > On 2016/08/16 15:08:49, Dan Ehrenberg wrote: > > > > This patch both implements and ships the feature, rather than guarding it > > > behind > > > > a flag. Although this feature is very simple and already part of an > > > established > > > > standard, strictly speaking, we should actually follow the feature > addition > > > > process at https://github.com/v8/v8/wiki/Feature%20Launch%20Process . > > > Typically, > > > > that means implementing it behind a flag, and sending out an intent to > ship > > > > email to v8-dev. Maybe we could skip the flag as this is so trivial, but > we > > > > should probably at least send such an email when shipping a feature. > > > > > > Ok. I tried to post (groups web ui) or send (email) to v8-users group/list > > (the > > > doc says that it is v8-users rather than v8-dev), but my post/email > > (sent/posted > > > from my http://chromium.org account ) > > > just went to /dev/null. None of them (I tried 3 times) showed up in > v8-users. > > (I > > > did subscribe to the group with my http://chromium.org account). > > > > > > Very strange ! > > > > Contacted the owner of v8-users. Turned out that my post/mail was > misclassified > > as spam. The owner > > manually posted it to the group. > > No response so far. (it's so trivial that nobody cares ? ). Will wait for a > while. Thank you, Adam, for LGTM at v8-users. A Chromestatus entry was added: https://www.chromestatus.com/feature/5659169956823040
lgtm
The CQ bit was checked by jshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* TEST=intl/general/getCanonicalLocales ========== to ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* TEST=intl/general/getCanonicalLocales ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* TEST=intl/general/getCanonicalLocales ========== to ========== Expose getCanonicalLocales() for Intl object. Also add a test for the return object of getCanonicalLocaleList(). See https://github.com/tc39/test262/issues/745 for more details. BUG=v8:5012 TEST=test262/intl402/Intl/getCanonicalLocales/* TEST=intl/general/getCanonicalLocales Committed: https://crrev.com/520f38fce79bf6370f46c4d3326e52bbd962030c Cr-Commit-Position: refs/heads/master@{#38733} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/520f38fce79bf6370f46c4d3326e52bbd962030c Cr-Commit-Position: refs/heads/master@{#38733} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
