Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411153006:
Variations string overrides now handles more .grd files (Closed)
DescriptionVariations string overrides now handles more .grd files
R=jwd@chromium.org
BUG=471815
Committed: https://crrev.com/7b630c8fc675af729250429c152fffb11b9c067b
Cr-Commit-Position: refs/heads/master@{#357166}
Patch Set 1 #
Total comments: 2
Patch Set 2 : expanded the comment a bit more #
Total comments: 4
Patch Set 3 : addressed comments on the CL #
Total comments: 4
Patch Set 4 : addressed comments and updated the gyp file (had forgotten to do so ...) #
Total comments: 8
Patch Set 5 : sorted the header files #Patch Set 6 : fixed a comma problem with chrome_resources.gyp file #
Messages
Total messages: 30 (9 generated)
https://codereview.chromium.org/1411153006/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:74: # Deduplicate resources nit: Add a bit more to the comment, like why you need to do this.
https://codereview.chromium.org/1411153006/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:74: # Deduplicate resources On 2015/10/22 19:00:34, Jesse Doherty wrote: > nit: Add a bit more to the comment, like why you need to do this. Done.
https://codereview.chromium.org/1411153006/diff/20001/components/variations/s... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/20001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:75: # files. Unless deduplicated here, collisions will be raised in _CheckForHashCollisions Nit: line too long.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1411153006/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/variations/BUILD.gn (right): https://codereview.chromium.org/1411153006/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/variations/BUILD.gn:15: "$root_gen_dir/chrome/grit/settings_google_chrome_strings.h", We should also add components/strings/grit/components_strings.h. More and more code in Chrome is moving to components, which means without that file, we're otherwise losing support for overriding those strings.
https://codereview.chromium.org/1411153006/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/variations/BUILD.gn (right): https://codereview.chromium.org/1411153006/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/variations/BUILD.gn:15: "$root_gen_dir/chrome/grit/settings_google_chrome_strings.h", On 2015/10/26 15:17:18, Alexei Svitkine (slow) wrote: > We should also add components/strings/grit/components_strings.h. > > More and more code in Chrome is moving to components, which means without that > file, we're otherwise losing support for overriding those strings. Done. https://codereview.chromium.org/1411153006/diff/20001/components/variations/s... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/20001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:75: # files. Unless deduplicated here, collisions will be raised in _CheckForHashCollisions On 2015/10/26 15:11:15, Jesse Doherty wrote: > Nit: line too long. Done.
lgtm
lgtm % nits You still need an owner for chrome/ https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:75: # files. Unless deduplicated here, collisions will be raised in Nit: Is this specific for chromium_ vs. google_chrome_ ? If so, I would mention that case explicitly. Otherwise, this leaves the reader wondering about what these cases are. https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:76: # _CheckForHashCollisions Nit: Add a .
Description was changed from ========== Variations string overrides now handles more .grd files R=jwd@chromium.org BUG=471815 ========== to ========== Variations string overrides now handles more .grd files R=jwd@chromium.org BUG=471815 ==========
mahmadi@chromium.org changed reviewers: + sky@chromium.org
updated the CL based on the comments and updated chrome/chrome_resources.gyp which was left out in the last commit. sky@, I'm passing a list of header files to generate_ui_string_overrider.py in chrome/chrome_resources.gyp. Please see if it looks good to you. Thanks. https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:75: # files. Unless deduplicated here, collisions will be raised in On 2015/10/27 15:54:31, Alexei Svitkine (slow) wrote: > Nit: Is this specific for chromium_ vs. google_chrome_ ? If so, I would mention > that case explicitly. Otherwise, this leaves the reader wondering about what > these cases are. Done. https://codereview.chromium.org/1411153006/diff/40001/components/variations/s... components/variations/service/generate_ui_string_overrider.py:76: # _CheckForHashCollisions On 2015/10/27 15:54:31, Alexei Svitkine (slow) wrote: > Nit: Add a . Done.
https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:315: '<(grit_out_dir)/grit/chromium_strings.h', nit: sort these. https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:319: '<(SHARED_INTERMEDIATE_DIR)/components/strings/grit/components_strings.h', What target are these files listed as outputs of? https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:336: '<(grit_out_dir)/grit/chromium_strings.h', nit: sort
addressed sky's comments https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:315: '<(grit_out_dir)/grit/chromium_strings.h', On 2015/10/28 15:47:54, sky wrote: > nit: sort these. Done. https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:319: '<(SHARED_INTERMEDIATE_DIR)/components/strings/grit/components_strings.h', On 2015/10/28 15:47:54, sky wrote: > What target are these files listed as outputs of? ../components/components_strings.gyp:components_strings https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:336: '<(grit_out_dir)/grit/chromium_strings.h', On 2015/10/28 15:47:54, sky wrote: > nit: sort Done.
https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:319: '<(SHARED_INTERMEDIATE_DIR)/components/strings/grit/components_strings.h', On 2015/10/29 01:13:20, Moe wrote: > On 2015/10/28 15:47:54, sky wrote: > > What target are these files listed as outputs of? > > ../components/components_strings.gyp:components_strings I don't seem them listed as outputs in that target. What am I missing?
https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources... chrome/chrome_resources.gyp:319: '<(SHARED_INTERMEDIATE_DIR)/components/strings/grit/components_strings.h', On 2015/10/29 15:57:01, sky wrote: > On 2015/10/29 01:13:20, Moe wrote: > > On 2015/10/28 15:47:54, sky wrote: > > > What target are these files listed as outputs of? > > > > ../components/components_strings.gyp:components_strings > > I don't seem them listed as outputs in that target. What am I missing? chrome_strings target also doesn't list generated_resources.h as an output. the equivalent grd files though mention the .h file as the output. Sorry I'm pretty new to this. there's a high chance I'm missing something.
sky@chromium.org changed reviewers: + mark@chromium.org
+mark for dependencies questions: https://codereview.chromium.org/1411153006/diff/60001/chrome/chrome_resources...
All grit actions include build/grit_action.gypi, which specifies outputs as
'outputs': [
'<!@pymod_do_main(grit_info <@(grit_defines) <@(grit_additional_defines) '
'<@(grit_whitelist_flag) --outputs \'<(grit_out_dir)\' '
'<(grit_grd_file) -f "<(grit_resource_ids)")',
],
grit_info in this mode should be outputting the paths of the files that grit_cmd
produces.
Thanks! LGTM
The CQ bit was checked by mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1411153006/#ps80001 (title: "sorted the header files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411153006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411153006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jwd@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1411153006/#ps100001 (title: "fixed a comma problem with chrome_resources.gyp file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411153006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411153006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7b630c8fc675af729250429c152fffb11b9c067b Cr-Commit-Position: refs/heads/master@{#357166} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
