🍵 Refactor the various locale_paks() templates to be more shared

Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
Committed: https://crrev.com/61973f5b2e2d7cf070c89043768b4a3e0025b19a
Cr-Commit-Position: refs/heads/master@{#418327}

[agrieve, 2016-09-10 02:55:02]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 02:55:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 02:58:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 02:58:51]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[agrieve, 2016-09-11 00:22:02]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-11 00:22:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 00:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-11 00:25:38]

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_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2016-09-11 00:48:21]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-11 00:48:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 00:51:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-11 00:51:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[agrieve, 2016-09-12 18:33:33]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 18:33:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2016-09-12 19:07:48]

agrieve@chromium.org changed reviewers:
+ michaelbai@chromium.org, sdefresne@chromium.org, thakis@chromium.org

[agrieve, 2016-09-12 19:07:49]

michaelbai@chromium.org: Please review changes in android_webview

thakis@chromium.org: Please review changes in tools/grit, chrome/

sdefresne@chromium.org: Please review changes in ios/

[agrieve, 2016-09-12 19:35:16]

Description was changed from

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
==========

to

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
==========

[agrieve, 2016-09-12 19:35:16]

agrieve@chromium.org changed reviewers:
+ estevenson@chromium.org

[agrieve, 2016-09-12 19:35:33]

On 2016/09/12 19:07:49, agrieve wrote:
> mailto:michaelbai@chromium.org: Please review changes in android_webview
> 
> mailto:thakis@chromium.org: Please review changes in tools/grit, chrome/
> 
> mailto:sdefresne@chromium.org: Please review changes in ios/

[commit-bot: I haz the power, 2016-09-12 19:43:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 19:43:56]

Dry run: This issue passed the CQ dry run.

[michaelbai, 2016-09-12 21:10:54]

LGTM android_webview, thanks for refactoring this.

[sdefresne, 2016-09-13 07:59:19]

https://codereview.chromium.org/2320883006/diff/100001/ios/chrome/extension_r...
File ios/chrome/extension_repack.gni (right):

https://codereview.chromium.org/2320883006/diff/100001/ios/chrome/extension_r...
ios/chrome/extension_repack.gni:48: define_bundle_data = true
Should be:

define_bundle_data = invoker.copy_data_to_bundle

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni
File tools/grit/repack.gni (right):

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni#...
tools/grit/repack.gni:103: #       input names. Mac uses different names in some
cases.
nit: Mac and iOS uses different names in some cases.

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni#...
tools/grit/repack.gni:123: # GN's subscript can't handle
invoker.output_locales[foo].
nit: can you add reference to crbug.com/614747 (which track adding support for
such expressions).

[agrieve, 2016-09-13 14:06:58]

Also changed define_bundle_data -> copy_data_to_bundle to match existing term.

https://codereview.chromium.org/2320883006/diff/100001/ios/chrome/extension_r...
File ios/chrome/extension_repack.gni (right):

https://codereview.chromium.org/2320883006/diff/100001/ios/chrome/extension_r...
ios/chrome/extension_repack.gni:48: define_bundle_data = true
On 2016/09/13 07:59:19, sdefresne wrote:
> Should be:
> 
> define_bundle_data = invoker.copy_data_to_bundle

Refactored this whole file to use the same locale_paks() template (somehow
missed this the first time around).

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni
File tools/grit/repack.gni (right):

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni#...
tools/grit/repack.gni:103: #       input names. Mac uses different names in some
cases.
On 2016/09/13 07:59:19, sdefresne wrote:
> nit: Mac and iOS uses different names in some cases.

Done.

https://codereview.chromium.org/2320883006/diff/100001/tools/grit/repack.gni#...
tools/grit/repack.gni:123: # GN's subscript can't handle
invoker.output_locales[foo].
On 2016/09/13 07:59:19, sdefresne wrote:
> nit: can you add reference to crbug.com/614747 (which track adding support for
> such expressions).

Done.

[sdefresne, 2016-09-13 14:25:26]

lgtm

[Nico, 2016-09-13 14:33:12]

Looks fine as I can tell, but I don't know this code super well. Looks like
brettw wrote chrome_repack_locales.gni, so cc'ing him in case he wants to take a
look. But since this is only moving stuff around and reducing duplication,
rs-lgtm if he doesn't want to look.

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
chrome/chrome_repack_locales.gni:104: if (is_mac || is_ios) {
Do you know why we use a different default directory on mac and ios?

[sdefresne, 2016-09-13 14:38:45]

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
chrome/chrome_repack_locales.gni:104: if (is_mac || is_ios) {
On 2016/09/13 14:33:10, Nico wrote:
> Do you know why we use a different default directory on mac and ios?

I don't know about Mac, but on iOS we do generate other version of the packed
locales in another directory (to include the downstream only strings).

[agrieve, 2016-09-13 15:22:29]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 15:22:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 16:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 16:37:34]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2016-09-13 17:16:34]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2016-09-13 17:16:35]

LGTM. But please run a clobber tryjob on Mac to make sure this is OK.

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
File chrome/chrome_repack_locales.gni (right):

https://codereview.chromium.org/2320883006/diff/120001/chrome/chrome_repack_l...
chrome/chrome_repack_locales.gni:104: if (is_mac || is_ios) {
On 2016/09/13 14:38:45, sdefresne wrote:
> On 2016/09/13 14:33:10, Nico wrote:
> > Do you know why we use a different default directory on mac and ios?
> 
> I don't know about Mac, but on iOS we do generate other version of the packed
> locales in another directory (to include the downstream only strings).

I don't think there's a good reason, but changing it requires changing a few
scripts, should be done separately if desired.

[agrieve, 2016-09-13 17:21:52]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 17:22:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 17:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 17:26:54]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-09-13 17:53:08]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2016-09-13 17:53:09]

rs lgtm

[agrieve, 2016-09-13 19:12:51]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-09-13 19:12:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org
Link to the patchset: https://codereview.chromium.org/2320883006/#ps120001
(title: "sdefresne comments")

[commit-bot: I haz the power, 2016-09-13 19:13:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 19:19:21]

Description was changed from

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
==========

to

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
==========

[commit-bot: I haz the power, 2016-09-13 19:19:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-13 19:21:16]

Description was changed from

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716
==========

to

==========
Refactor the various locale_paks() templates to be more shared

Main motivation for this is to more easily add per-target
repack_whitelist.

BUG=645716

Committed: https://crrev.com/61973f5b2e2d7cf070c89043768b4a3e0025b19a
Cr-Commit-Position: refs/heads/master@{#418327}
==========

[commit-bot: I haz the power, 2016-09-13 19:21:20]

Patchset 7 (id:??) landed as
https://crrev.com/61973f5b2e2d7cf070c89043768b4a3e0025b19a
Cr-Commit-Position: refs/heads/master@{#418327}

[agrieve, 2016-09-13 23:45:49]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2340673002/ by agrieve@chromium.org.

The reason for reverting is: gn gen fails when
enable_resource_whitelist_generation=true.