Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988023003:
[gn] Port icu_use_data_file_flag in gn (Closed)
|
Created:
4 years, 7 months ago by Michael Achenbach Modified:
4 years, 7 months ago Reviewers:
vogelheim CC:
v8-reviews_googlegroups.com, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Port icu_use_data_file_flag in gn
BUG=chromium:474921
LOG=n
NOTRY=true
Committed: https://crrev.com/517b6599282b41d049e3368567940a95fec9b6e4
Cr-Commit-Position: refs/heads/master@{#36329}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Review #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== [gn] Port icu_use_data_file_flag in gn BUG= ========== to ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n ==========
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988023003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988023003/1
machenbach@chromium.org changed reviewers: + vogelheim@chromium.org
PTAL https://codereview.chromium.org/1988023003/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1988023003/diff/1/BUILD.gn#newcode23 BUILD.gn:23: icu_use_data_file_flag = true I'm not happy to have a non-v8 prefixed name in this list. But I rather keep names equal to the gyp world for now to avoid confusion during the transition. https://codereview.chromium.org/1988023003/diff/1/build_overrides/v8.gni File build_overrides/v8.gni (right): https://codereview.chromium.org/1988023003/diff/1/build_overrides/v8.gni#newc... build_overrides/v8.gni:25: icu_use_data_file_flag = false This is the v8 default from here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/v8.gyp&q=ic... https://codereview.chromium.org/1988023003/diff/1/src/v8.gyp File src/v8.gyp (right): https://codereview.chromium.org/1988023003/diff/1/src/v8.gyp#newcode1606 src/v8.gyp:1606: ], This is just moved under the v8_enable_i18n_support==1 section. Like that it's equal to how gn is configured now. The old just leads to unnecessary defines when icu support is off.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988023003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988023003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1988023003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1988023003/diff/20001/BUILD.gn#newcode1846 BUILD.gn:1846: if (is_win) { style nitpick: GN manual uses else-if on one line, like so: } else if (...) { In fairness, practical usage differs: - 96 instances if else-if [1] - 11 instance of else-<newline>-if [2] So.. pick whatever you want. :) [1] https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.gn?$%20else%... [2] https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.gn?$%20else%...
Thanks! https://codereview.chromium.org/1988023003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1988023003/diff/20001/BUILD.gn#newcode1846 BUILD.gn:1846: if (is_win) { On 2016/05/18 14:16:56, vogelheim wrote: > style nitpick: GN manual uses else-if on one line, like so: > } else if (...) { > > In fairness, practical usage differs: > - 96 instances if else-if [1] > - 11 instance of else-<newline>-if [2] > > > So.. pick whatever you want. :) > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.gn?$%20else%... > [2] > https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.gn?$%20else%... Done.
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/1988023003/#ps40001 (title: "Review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988023003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988023003/40001
Description was changed from ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n ========== to ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n NOTRY=true ==========
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988023003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988023003/40001
Message was sent while issue was closed.
Description was changed from ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n NOTRY=true ========== to ========== [gn] Port icu_use_data_file_flag in gn BUG=chromium:474921 LOG=n NOTRY=true Committed: https://crrev.com/517b6599282b41d049e3368567940a95fec9b6e4 Cr-Commit-Position: refs/heads/master@{#36329} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/517b6599282b41d049e3368567940a95fec9b6e4 Cr-Commit-Position: refs/heads/master@{#36329}
Message was sent while issue was closed.
This patch is not complete yet. There is also a flag from ICU upstream: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/co... We should use this instead to control v8's and icu's behavior. Unfortunately there's no way to overwrite args defaults in the build overrides v8.gni. If we specify it there, it's just ignored. I'll post on gn-dev. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
