Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586763002:
[intl] Create the Intl constructors to C++ (Closed)
|
Created:
4 years ago by Dan Ehrenberg Modified:
3 years, 12 months ago Reviewers:
Yang CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[intl] Create the Intl constructors to C++
This patch moves the creation of the Intl constructors from JavaScript
to C++ in bootstrapper.cc, to match all of the other builtins exposed
to the web.
BUG=v8:5751
Review-Url: https://codereview.chromium.org/2586763002
Cr-Commit-Position: refs/heads/master@{#41959}
Committed: https://chromium.googlesource.com/v8/v8/+/e0359c36294e7fda024d075eca40c3b773f5abb9
Patch Set 1 #Patch Set 2 : Exclude unnecessary variable #Patch Set 3 : Set Intl.*.prototype.constructor properly #Patch Set 4 : Rebase #Patch Set 5 : Fix factory references #
Total comments: 3
Patch Set 6 : Rebase and fix long lines #Patch Set 7 : Fix long lines more #
Messages
Total messages: 31 (25 generated)
The CQ bit was checked by littledan@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: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by littledan@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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by littledan@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: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by littledan@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: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18217) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14633)
The CQ bit was checked by littledan@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.
littledan@chromium.org changed reviewers: + yangguo@chromium.org
This patch is set up as depending on the new constructor semantics, but it could land before if needed. There's just a lot of conflicts as they are updating adjacent code, which is why I put them in series, and the follow-on patch depends on both.
https://codereview.chromium.org/2586763002/diff/80001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2586763002/diff/80001/src/bootstrapper.cc#new... src/bootstrapper.cc:2361: number_format_prototype, Builtins::kIllegal); What's the point of installing half the function here and half of it later using %SetCode, as opposed to just calling InstallFunction in i18n.js? Seems like we are arriving at the same thing (installing a built-in implemented in JS), but with more code.
https://codereview.chromium.org/2586763002/diff/80001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2586763002/diff/80001/src/bootstrapper.cc#new... src/bootstrapper.cc:2361: number_format_prototype, Builtins::kIllegal); On 2016/12/21 13:23:13, Yang wrote: > What's the point of installing half the function here and half of it later using > %SetCode, as opposed to just calling InstallFunction in i18n.js? Seems like we > are arriving at the same thing (installing a built-in implemented in JS), but > with more code. In a follow-on patch, i'll change the allocated object size, which is only possible from C++. That patch is getting pretty huge, so I thought it'd be better to break out this part.
lgtm with nit https://codereview.chromium.org/2586763002/diff/80001/test/intl/toStringTag.js File test/intl/toStringTag.js (right): https://codereview.chromium.org/2586763002/diff/80001/test/intl/toStringTag.j... test/intl/toStringTag.js:7: descriptor = Object.getOwnPropertyDescriptor(Intl.DateTimeFormat.prototype, Symbol.toStringTag); 80char limit
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2586763002/#ps120001 (title: "Fix long lines more")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482857045946260,
"parent_rev": "d20c23cd13c1affa8e4975dfbf42904282769b31", "commit_rev":
"e0359c36294e7fda024d075eca40c3b773f5abb9"}
Message was sent while issue was closed.
Description was changed from ========== [intl] Create the Intl constructors to C++ This patch moves the creation of the Intl constructors from JavaScript to C++ in bootstrapper.cc, to match all of the other builtins exposed to the web. BUG=v8:5751 ========== to ========== [intl] Create the Intl constructors to C++ This patch moves the creation of the Intl constructors from JavaScript to C++ in bootstrapper.cc, to match all of the other builtins exposed to the web. BUG=v8:5751 Review-Url: https://codereview.chromium.org/2586763002 Cr-Commit-Position: refs/heads/master@{#41959} Committed: https://chromium.googlesource.com/v8/v8/+/e0359c36294e7fda024d075eca40c3b773f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/e0359c36294e7fda024d075eca40c3b773f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
