Add support to override output_all_resource_defines from command line.

Add support to override output_all_resource_defines from command line.

If --output_all_resource_defines or --nooutput_all_resource_defines
is specified to grit's build tool, then the value in the .grd will be
overridden.

Because gyp sends the same args to grit_info.py and grit.py, this CL
also updates grit_info.py to accept these arguments to avoid
unsupported argument errors.

BUG=428947
R=newt@chromium.org

Committed: https://code.google.com/p/grit-i18n/source/detail?r=180

[lliabraa, 2014-10-30 19:49:00]

lliabraa@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2014-10-30 20:16:21]

https://codereview.chromium.org/691873002/diff/1/grit/tool/build.py
File grit/tool/build.py (right):

https://codereview.chromium.org/691873002/diff/1/grit/tool/build.py#newcode58
grit/tool/build.py:58: Usage:  grit build [-o OUTPUTDIR] [-D NAME[=VAL]]*
update this usage comment as well

https://codereview.chromium.org/691873002/diff/1/grit/tool/build.py#newcode129
grit/tool/build.py:129: 'output-all-resource-defines',
Alternatively, this could accept an argument, e.g.

  --output-all-resource-defines=1

or

  --output-all-resource-defines=0

This might be easier to integrate with GYP/GN. Either way should be fine though.

https://codereview.chromium.org/691873002/diff/1/grit/tool/build_unittest.py
File grit/tool/build_unittest.py (right):

https://codereview.chromium.org/691873002/diff/1/grit/tool/build_unittest.py#...
grit/tool/build_unittest.py:118: self.input =
util.PathFromRoot('grit/testdata/whitelist_resources.grd')
whitelist_resources.grd and whitelist.txt don't seem to exist. Do you need to
add those to this CL?

https://codereview.chromium.org/691873002/diff/1/grit/tool/build_unittest.py#...
grit/tool/build_unittest.py:138: non_whitelisted_ids = []
wouldn't it be more useful if this list were non-empty?

[lliabraa, 2014-10-31 11:33:14]

Thanks for the review. PTAL

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build.py
File grit/tool/build.py (right):

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build.py#ne...
grit/tool/build.py:58: Usage:  grit build [-o OUTPUTDIR] [-D NAME[=VAL]]*
On 2014/10/30 20:16:20, newt wrote:
> update this usage comment as well

Done.

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build.py#ne...
grit/tool/build.py:129: 'output-all-resource-defines',
On 2014/10/30 20:16:20, newt wrote:
> Alternatively, this could accept an argument, e.g.
> 
>   --output-all-resource-defines=1
> 
> or
> 
>   --output-all-resource-defines=0
> 
> This might be easier to integrate with GYP/GN. Either way should be fine
though.

I think this is a little more explicit because it avoids the ambiguity of what
to put on the command line (--output-all=1, '1', True, true, etc).  Also,
--output-all-resource-defines=0 ends up using the string '0' which doesn't
evaluate to False so it would need a little coercing.

The gyp change is just to add --output-all-resource-defines or
--no-output-all-resource-defines to the list of grit_defines. In the examples
I'm thinking of it's no better/worse than --output-all-resource-defines=1/0, but
maybe you had something specific in mind.

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build_unitt...
File grit/tool/build_unittest.py (right):

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build_unitt...
grit/tool/build_unittest.py:118: self.input =
util.PathFromRoot('grit/testdata/whitelist_resources.grd')
On 2014/10/30 20:16:21, newt wrote:
> whitelist_resources.grd and whitelist.txt don't seem to exist. Do you need to
> add those to this CL?

Done.

https://chromiumcodereview.appspot.com/691873002/diff/1/grit/tool/build_unitt...
grit/tool/build_unittest.py:138: non_whitelisted_ids = []
On 2014/10/30 20:16:20, newt wrote:
> wouldn't it be more useful if this list were non-empty?

This test case covers output_all_resource_defines='true' so the whitelist is
ignored and all IDs end up in the output files. I have a couple other test cases
that cover whitelisting in the CL at
https://chromiumcodereview.appspot.com/669763002/

[lliabraa, 2014-10-31 20:22:07]

I've sync's since landing my other grit CL and made a few fixes. This is ready
for another review.

[lliabraa, 2014-10-31 20:32:27]

On 2014/10/31 20:22:07, lliabraa wrote:
> I've sync's since landing my other grit CL and made a few fixes. This is ready
> for another review.

also, the CL that will use this for iOS is at
https://chromiumcodereview.appspot.com/692233002/

[newt (away), 2014-11-03 19:00:46]

lgtm after comments in grit_info.py

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py
File grit_info.py (right):

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
grit_info.py:115: parser.add_option("--output-all-resource-defines",
action="store_true",
These arguments don't seem necessary in this file, since
--[no-]output-all-resource-defines doesn't affect the list of input or output
files. Similarly, -a and -h aren't arguments to grit_info, even though they're
arguments to grit build.

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
grit_info.py:117: help="Unused")
why help="Unused"?

[lliabraa, 2014-11-03 20:10:19]

Thanks for the review.

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py
File grit_info.py (right):

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
grit_info.py:115: parser.add_option("--output-all-resource-defines",
action="store_true",
On 2014/11/03 19:00:46, newt wrote:
> These arguments don't seem necessary in this file, since
> --[no-]output-all-resource-defines doesn't affect the list of input or output
> files. Similarly, -a and -h aren't arguments to grit_info, even though they're
> arguments to grit build.

Correct...they're unused. However, the gyp/gn files define grit_defines
variables and send them to both grit.py and grit_info.py, so when I add
--output-all-resource-defines to the grit_defines so that the grit build command
gets it, gyp/gn also sends it to grit_info. Without this change to grit_info,
sending --[no-]output-all-resource-defines results in an error.

As for the -a and -h, it doesn't look like these args are ever used in a
grit_defines, so they're not sent to both python scripts.

I'd rather not touch the grit_action/grit_target gypi files (e.g. to support
grit_build_defines and grit_info_defines separately), so these new parameters
need to be handled somehow here. I suppose I could strip the new args from
|argv| before calling parser.parse_args(argv), but I'm not sure that's really
any better. WDYT?

https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
grit_info.py:117: help="Unused")
On 2014/11/03 19:00:46, newt wrote:
> why help="Unused"?

because the values of these parameters are not used by the script (see above).

[newt (away), 2014-11-03 21:08:17]

On 2014/11/03 20:10:19, lliabraa wrote:
> Thanks for the review.
> 
> https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py
> File grit_info.py (right):
> 
>
https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
> grit_info.py:115: parser.add_option("--output-all-resource-defines",
> action="store_true",
> On 2014/11/03 19:00:46, newt wrote:
> > These arguments don't seem necessary in this file, since
> > --[no-]output-all-resource-defines doesn't affect the list of input or
output
> > files. Similarly, -a and -h aren't arguments to grit_info, even though
they're
> > arguments to grit build.
> 
> Correct...they're unused. However, the gyp/gn files define grit_defines
> variables and send them to both grit.py and grit_info.py, so when I add
> --output-all-resource-defines to the grit_defines so that the grit build
command
> gets it, gyp/gn also sends it to grit_info. Without this change to grit_info,
> sending --[no-]output-all-resource-defines results in an error.
> 
> As for the -a and -h, it doesn't look like these args are ever used in a
> grit_defines, so they're not sent to both python scripts.
> 
> I'd rather not touch the grit_action/grit_target gypi files (e.g. to support
> grit_build_defines and grit_info_defines separately), so these new parameters
> need to be handled somehow here. I suppose I could strip the new args from
> |argv| before calling parser.parse_args(argv), but I'm not sure that's really
> any better. WDYT?

This sounds fine to me.

>
https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
> grit_info.py:117: help="Unused")
> On 2014/11/03 19:00:46, newt wrote:
> > why help="Unused"?
> 
> because the values of these parameters are not used by the script (see above).

[lliabraa, 2014-11-04 13:24:12]

On 2014/11/03 21:08:17, newt wrote:
> On 2014/11/03 20:10:19, lliabraa wrote:
> > Thanks for the review.
> > 
> > https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py
> > File grit_info.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
> > grit_info.py:115: parser.add_option("--output-all-resource-defines",
> > action="store_true",
> > On 2014/11/03 19:00:46, newt wrote:
> > > These arguments don't seem necessary in this file, since
> > > --[no-]output-all-resource-defines doesn't affect the list of input or
> output
> > > files. Similarly, -a and -h aren't arguments to grit_info, even though
> they're
> > > arguments to grit build.
> > 
> > Correct...they're unused. However, the gyp/gn files define grit_defines
> > variables and send them to both grit.py and grit_info.py, so when I add
> > --output-all-resource-defines to the grit_defines so that the grit build
> command
> > gets it, gyp/gn also sends it to grit_info. Without this change to
grit_info,
> > sending --[no-]output-all-resource-defines results in an error.
> > 
> > As for the -a and -h, it doesn't look like these args are ever used in a
> > grit_defines, so they're not sent to both python scripts.
> > 
> > I'd rather not touch the grit_action/grit_target gypi files (e.g. to support
> > grit_build_defines and grit_info_defines separately), so these new
parameters
> > need to be handled somehow here. I suppose I could strip the new args from
> > |argv| before calling parser.parse_args(argv), but I'm not sure that's
really
> > any better. WDYT?
> 
> This sounds fine to me.
> 
> >
>
https://chromiumcodereview.appspot.com/691873002/diff/140001/grit_info.py#new...
> > grit_info.py:117: help="Unused")
> > On 2014/11/03 19:00:46, newt wrote:
> > > why help="Unused"?
> > 
> > because the values of these parameters are not used by the script (see
above).

Great. Could you land this CL for me when you get a chance?

[newt (away), 2014-11-04 18:51:52]

Committed patchset #8 (id:140001) manually as 180 (presubmit successful).