Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824173004:
Fix packing localized strings for //remoting. (Closed)
DescriptionFix packing localized strings for //remoting.
On iOS, the locales "pt-BR" and "es-419" are called respectively
"pt" and "es-MX" (with on-disk files named "pt" and "es_MX").
Fix remoting_strings.grd to use the correct rules when listing the
output files (i.e. add conditionals around "pt" and "es-MX" when
generating both message.json and $locale.pak files).
Add the corresponding fixes to the declaration of the remoting_locales
and remoting_locales_with_underscores variables.
Fix warning about multiple target generating the same files when doing
a fat build on iOS by using root_out_dir (that depends on the toolchain)
instead of root_build_dir (independent of the toolchain) for the output
paths.
BUG=711551
Review-Url: https://codereview.chromium.org/2824173004
Cr-Commit-Position: refs/heads/master@{#465934}
Committed: https://chromium.googlesource.com/chromium/src/+/e79e858ded5b3c99d94fa74ffabeec16ef2b2a78
Patch Set 1 #Patch Set 2 : Fix fat builds. #
Total comments: 11
Patch Set 3 : Address comments. #
Messages
Total messages: 36 (26 generated)
The CQ bit was checked by sdefresne@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...
sdefresne@chromium.org changed reviewers: + nicholss@chromium.org, wez@chromium.org
nicholss: please take a look. wez: for OWNERS stamp.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix packing localized strings for //remoting. On iOS, the locales "en", "pt-BR" and "es-419" are called respectively "en-US", "pt" and "es-MX" (with on-disk files named "en_US", "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "en-US", "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix uses of root_gen_dir and target_gen_dir to use root_out_dir to avoid having multiple target generating the same files when doing a fat build on iOS. BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "en", "pt-BR" and "es-419" are called respectively "en-US", "pt" and "es-MX" (with on-disk files named "en_US", "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "en-US", "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS (by using groups to forward the target in non-default toolchain to the default ones). BUG=711551 ==========
The CQ bit was checked by sdefresne@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 ========== Fix packing localized strings for //remoting. On iOS, the locales "en", "pt-BR" and "es-419" are called respectively "en-US", "pt" and "es-MX" (with on-disk files named "en_US", "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "en-US", "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS (by using groups to forward the target in non-default toolchain to the default ones). BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "en", "pt-BR" and "es-419" are called respectively "en-US", "pt" and "es-MX" (with on-disk files named "en_US", "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "en-US", "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. BUG=711551 ==========
The CQ bit was checked by sdefresne@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... File remoting/remoting_locales.gni (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... remoting/remoting_locales.gni:95: if (is_chromeos || is_ios) { nit: Let's make sure to retain comments that explain why we need these ugly special-cases. https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... remoting/remoting_locales.gni:100: if (is_ios) { nit: This might be more readable as an if...else with just += clauses for the two cases. https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... File remoting/resources/BUILD.gn (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... remoting/resources/BUILD.gn:50: stampfile = "$root_out_dir/remoting_resources_verified_stamp" These changes don't seem related to the CL description? https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... remoting/resources/remoting_strings.grd:24: <if expr="is_ios"> nit: If it is acceptable in this file format, consider putting a line of whitespace before and after the is_ios / not is_ios conditional block so that it stands out as a single logical block, and put the explanatory comment preceding the block (i.e. before the "if" immediately above) rather than inside it, i.e: blank line <!- ... > <if> </if> <if> </if> blank line
Description was changed from ========== Fix packing localized strings for //remoting. On iOS, the locales "en", "pt-BR" and "es-419" are called respectively "en-US", "pt" and "es-MX" (with on-disk files named "en_US", "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "en-US", "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. Always generate "en-US" locale. BUG=711551 ==========
Description was changed from ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. Always generate "en-US" locale. BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. BUG=711551 ==========
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... File remoting/remoting_locales.gni (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... remoting/remoting_locales.gni:95: if (is_chromeos || is_ios) { On 2017/04/18 18:05:26, Wez wrote: > nit: Let's make sure to retain comments that explain why we need these ugly > special-cases. Done. BTW, do you know why remoting uses "en-GB", "en" and "en-US" (only for ChromeOS and Chrome on iOS) while Chrome itself only "en-GB" and "en-US" (for all platforms). https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... remoting/remoting_locales.gni:100: if (is_ios) { On 2017/04/18 18:05:26, Wez wrote: > nit: This might be more readable as an if...else with just += clauses for the > two cases. Done. https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... File remoting/resources/BUILD.gn (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... remoting/resources/BUILD.gn:50: stampfile = "$root_out_dir/remoting_resources_verified_stamp" On 2017/04/18 18:05:26, Wez wrote: > These changes don't seem related to the CL description? Those changes correspond to the following part of the CL description: Fix warning about multiple target generating the same files when doing a fat build on iOS. Without those, I get warning about multiple targets generating the same file; the targets are //remoting/resources:verify_resources and //remoting/resources:verify_resources(//build/toolchain/mac:ios_clang_arm). https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... remoting/resources/remoting_strings.grd:24: <if expr="is_ios"> On 2017/04/18 18:05:26, Wez wrote: > nit: If it is acceptable in this file format, consider putting a line of > whitespace before and after the is_ios / not is_ios conditional block so that it > stands out as a single logical block, and put the explanatory comment preceding > the block (i.e. before the "if" immediately above) rather than inside it, i.e: > > blank line > <!- ... > > <if> > </if> > <if> > </if> > blank line It is acceptable, however, all the other .grd files that are compiled on both iOS and other platforms in the chromium repository use the current format (look for example at components/components_strings.grd). If it is okay with you I would prefer having consistency in that file too.
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.
The CQ bit was checked by nicholss@chromium.org
lgtm LGTM, Thanks a bunch for helping with that!
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
LGTM w/ nit. I would suggest landing this, ideally with the CL description issue addressed, and letting Scott follow-up w/ Jamie & Sergey on the question of the "en" locale identifier. https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... File remoting/remoting_locales.gni (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... remoting/remoting_locales.gni:95: if (is_chromeos || is_ios) { On 2017/04/19 08:59:03, sdefresne wrote: > On 2017/04/18 18:05:26, Wez wrote: > > nit: Let's make sure to retain comments that explain why we need these ugly > > special-cases. > > Done. > > BTW, do you know why remoting uses "en-GB", "en" and "en-US" (only for ChromeOS > and Chrome on iOS) while Chrome itself only "en-GB" and "en-US" (for all > platforms). No, I'm afraid you will need to ask jamiewalch@ or sergeyu@ about that (I am OWNERS alumnus so wasn't expecting tough questions ;) https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... File remoting/resources/BUILD.gn (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... remoting/resources/BUILD.gn:50: stampfile = "$root_out_dir/remoting_resources_verified_stamp" On 2017/04/19 08:59:03, sdefresne wrote: > On 2017/04/18 18:05:26, Wez wrote: > > These changes don't seem related to the CL description? > > Those changes correspond to the following part of the CL description: > > Fix warning about multiple target generating the same files when doing > a fat build on iOS. > > Without those, I get warning about multiple targets generating the same file; > the targets are //remoting/resources:verify_resources and > //remoting/resources:verify_resources(//build/toolchain/mac:ios_clang_arm). Acknowledged, but consider clarifying in the description that you're fixing this by fixing the output filenames of these steps. :) https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... remoting/resources/remoting_strings.grd:24: <if expr="is_ios"> On 2017/04/19 08:59:03, sdefresne wrote: > On 2017/04/18 18:05:26, Wez wrote: > > nit: If it is acceptable in this file format, consider putting a line of > > whitespace before and after the is_ios / not is_ios conditional block so that > it > > stands out as a single logical block, and put the explanatory comment > preceding > > the block (i.e. before the "if" immediately above) rather than inside it, i.e: > > > > blank line > > <!- ... > > > <if> > > </if> > > <if> > > </if> > > blank line > > It is acceptable, however, all the other .grd files that are compiled on both > iOS and other platforms in the chromium repository use the current format (look > for example at components/components_strings.grd). If it is okay with you I > would prefer having consistency in that file too. Acknowledged.
Description was changed from ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS. BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS by using root_out_dir (that depends on the toolchain) instead of root_build_dir (independent of the toolchain) for the output paths. BUG=711551 ==========
Thank you for the review. On 2017/04/20 03:41:22, Wez wrote: > LGTM w/ nit. > > I would suggest landing this, ideally with the CL description issue addressed, > and letting Scott follow-up w/ Jamie & Sergey on the question of the "en" locale > identifier. > > https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... > File remoting/remoting_locales.gni (right): > > https://codereview.chromium.org/2824173004/diff/40001/remoting/remoting_local... > remoting/remoting_locales.gni:95: if (is_chromeos || is_ios) { > On 2017/04/19 08:59:03, sdefresne wrote: > > On 2017/04/18 18:05:26, Wez wrote: > > > nit: Let's make sure to retain comments that explain why we need these ugly > > > special-cases. > > > > Done. > > > > BTW, do you know why remoting uses "en-GB", "en" and "en-US" (only for > ChromeOS > > and Chrome on iOS) while Chrome itself only "en-GB" and "en-US" (for all > > platforms). > > No, I'm afraid you will need to ask jamiewalch@ or sergeyu@ about that (I am > OWNERS alumnus so wasn't expecting tough questions ;) Ack. > https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... > File remoting/resources/BUILD.gn (right): > > https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/BUIL... > remoting/resources/BUILD.gn:50: stampfile = > "$root_out_dir/remoting_resources_verified_stamp" > On 2017/04/19 08:59:03, sdefresne wrote: > > On 2017/04/18 18:05:26, Wez wrote: > > > These changes don't seem related to the CL description? > > > > Those changes correspond to the following part of the CL description: > > > > Fix warning about multiple target generating the same files when doing > > a fat build on iOS. > > > > Without those, I get warning about multiple targets generating the same file; > > the targets are //remoting/resources:verify_resources and > > //remoting/resources:verify_resources(//build/toolchain/mac:ios_clang_arm). > > Acknowledged, but consider clarifying in the description that you're fixing this > by fixing the output filenames of these steps. :) Done. > https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... > File remoting/resources/remoting_strings.grd (right): > > https://codereview.chromium.org/2824173004/diff/40001/remoting/resources/remo... > remoting/resources/remoting_strings.grd:24: <if expr="is_ios"> > On 2017/04/19 08:59:03, sdefresne wrote: > > On 2017/04/18 18:05:26, Wez wrote: > > > nit: If it is acceptable in this file format, consider putting a line of > > > whitespace before and after the is_ios / not is_ios conditional block so > that > > it > > > stands out as a single logical block, and put the explanatory comment > > preceding > > > the block (i.e. before the "if" immediately above) rather than inside it, > i.e: > > > > > > blank line > > > <!- ... > > > > <if> > > > </if> > > > <if> > > > </if> > > > blank line > > > > It is acceptable, however, all the other .grd files that are compiled on both > > iOS and other platforms in the chromium repository use the current format > (look > > for example at components/components_strings.grd). If it is okay with you I > > would prefer having consistency in that file too. > > Acknowledged.
The CQ bit was checked by sdefresne@chromium.org
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": 60001, "attempt_start_ts": 1492673063028620,
"parent_rev": "167501dfedb9ac4c975d750a5fd40c724e15e8ef", "commit_rev":
"e79e858ded5b3c99d94fa74ffabeec16ef2b2a78"}
Message was sent while issue was closed.
Description was changed from ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS by using root_out_dir (that depends on the toolchain) instead of root_build_dir (independent of the toolchain) for the output paths. BUG=711551 ========== to ========== Fix packing localized strings for //remoting. On iOS, the locales "pt-BR" and "es-419" are called respectively "pt" and "es-MX" (with on-disk files named "pt" and "es_MX"). Fix remoting_strings.grd to use the correct rules when listing the output files (i.e. add conditionals around "pt" and "es-MX" when generating both message.json and $locale.pak files). Add the corresponding fixes to the declaration of the remoting_locales and remoting_locales_with_underscores variables. Fix warning about multiple target generating the same files when doing a fat build on iOS by using root_out_dir (that depends on the toolchain) instead of root_build_dir (independent of the toolchain) for the output paths. BUG=711551 Review-Url: https://codereview.chromium.org/2824173004 Cr-Commit-Position: refs/heads/master@{#465934} Committed: https://chromium.googlesource.com/chromium/src/+/e79e858ded5b3c99d94fa74ffabe... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e79e858ded5b3c99d94fa74ffabe... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
