Apply whitelist file to structure elements.

Apply whitelist file to structure elements.

Resources specified with structure elements were being included in
.pak and .h files even if they were not included in a specified
whitelist file.

The .pak file issue is fixed in build.py by including structure nodes
in the collection of node types that are checked against the whitelist.
This seems to be an oversight as the code has a comment like "Apply
the same trick that data_pack.py uses" but the corresponding code in
data_pack.py was updated to include structure noded in [1]. This method
that contained this code is no longer in data_pack.py.

The .h file issue is fixed in rc_header.py by treating structure nodes
in the same manner as message nodes (i.e. only output them if they're
active). The correct fix seems to be to set output_all_resource_defines
to true in all the .grd files, but until then a structure element should
not generate an entry in a header file if it's not in the whitelist.

https://chromiumcodereview.appspot.com/10386189/diff/29002/grit/format/data_pack.py

BUG=None
R=joaodasilva@chromium.org, newt@chromium.org

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

[lliabraa, 2014-10-20 18:53:53]

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

[lliabraa, 2014-10-20 18:53:54]

@joaodasilva - This is the first time I've touched grit, so not sure what
unintended consequences may be lurking here. Please add other reviewer(s) as
needed (though thakis is out until Oct 27 and I'd like to get feedback before
then.

[Joao da Silva, 2014-10-21 09:48:32]

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

[Joao da Silva, 2014-10-21 09:48:33]

I'm more familiar with the code in grit/format/policy_templates but this looks
sane, so lgtm.

I used to ask Jói about these details :-) I think newt@ may know this well too.

[lliabraa, 2014-10-21 11:36:34]

+stuartmorgan FYI

[newt (away), 2014-10-21 15:43:27]

lgtm, though I'm also only somewhat familiar with these files

[lliabraa, 2014-10-22 14:10:59]

On 2014/10/21 15:43:27, newt wrote:
> lgtm, though I'm also only somewhat familiar with these files

@newt - I'm not a grit committer so could you land this for me?

[newt (away), 2014-10-22 16:38:03]

On 2014/10/22 14:10:59, lliabraa wrote:
> On 2014/10/21 15:43:27, newt wrote:
> > lgtm, though I'm also only somewhat familiar with these files
> 
> @newt - I'm not a grit committer so could you land this for me?

I can land this, but could I request one more thing first: a unittest for
whitelisting.

[newt (away), 2014-10-22 16:38:53]

On 2014/10/22 16:38:03, newt wrote:
> On 2014/10/22 14:10:59, lliabraa wrote:
> > On 2014/10/21 15:43:27, newt wrote:
> > > lgtm, though I'm also only somewhat familiar with these files
> > 
> > @newt - I'm not a grit committer so could you land this for me?
> 
> I can land this, but could I request one more thing first: a unittest for
> whitelisting.

FYI, you can run the existing unit tests using: python grit/test_suite_all.py

[newt (away), 2014-10-22 21:18:03]

Hmmm, Chrome for Android doesn't compile with this CL:

../../ui/native_theme/common_theme.cc:137:15: error: use of undeclared
identifier 'IDR_MENU_CHECK_CHECKED'
              IDR_MENU_CHECK_CHECKED).ToImageSkia();
              ^

This happens because IDR_MENU_CHECK_CHECKED is defined inside an <if
expr="toolkit_views"> block in ui_resources.grd, hence with
IDR_MENU_CHECK_CHECKED was not emitted in the .h file.

Do you need to change rc_header.py, or would it be sufficient just to change
build.py so that pak files don't include structures that aren't whitelisted?

[lliabraa, 2014-10-23 11:49:13]

On 2014/10/22 21:18:03, newt wrote:
> Hmmm, Chrome for Android doesn't compile with this CL:
> 
> ../../ui/native_theme/common_theme.cc:137:15: error: use of undeclared
> identifier 'IDR_MENU_CHECK_CHECKED'
>               IDR_MENU_CHECK_CHECKED).ToImageSkia();
>               ^
> 
> This happens because IDR_MENU_CHECK_CHECKED is defined inside an <if
> expr="toolkit_views"> block in ui_resources.grd, hence with
> IDR_MENU_CHECK_CHECKED was not emitted in the .h file.
> 
> Do you need to change rc_header.py, or would it be sufficient just to change
> build.py so that pak files don't include structures that aren't whitelisted?

I hit this for iOS as well. On iOS, we don't actually need native_theme so I
have a CL out to remove it from the iOS build
(https://chromiumcodereview.appspot.com/667223003/).

Seems like the correct solution is to not have these platforms compiling code
that refers to resources that the platform doesn't support. Defining the IDR in
a header is misleading if the asset isn't actually in the .pak file; I'd prefer
a compile-time failure if we're referring to an asset that isn't supported.

Since I can't run try jobs against this CL I was thinking I'd land it in grit
and run try jobs against a CL that rolls DEPS for grit. That should flush out
issues like the one you've found, which I would fix before landing this CL. This
will block rolling DEPS for grit until the chromium builds are fixed, but it
doesn't look like grit's whitelisting functionality is used very much.

In any case, I'll work on some unit tests for whitelisting. I found a separate
case with this CL when grit generates resource maps: grit is adding a
non-whiltelisted resource to foo_resource_map.cc which causes a compile error
because the resource is not defined in foo_resource.h. Probably just another if
check that's not accounting for StrucutreNode.

[newt (away), 2014-10-23 16:19:41]

On 2014/10/23 11:49:13, lliabraa wrote:
> On 2014/10/22 21:18:03, newt wrote:
> > Hmmm, Chrome for Android doesn't compile with this CL:
> > 
> > ../../ui/native_theme/common_theme.cc:137:15: error: use of undeclared
> > identifier 'IDR_MENU_CHECK_CHECKED'
> >               IDR_MENU_CHECK_CHECKED).ToImageSkia();
> >               ^
> > 
> > This happens because IDR_MENU_CHECK_CHECKED is defined inside an <if
> > expr="toolkit_views"> block in ui_resources.grd, hence with
> > IDR_MENU_CHECK_CHECKED was not emitted in the .h file.
> > 
> > Do you need to change rc_header.py, or would it be sufficient just to change
> > build.py so that pak files don't include structures that aren't whitelisted?
> 
> I hit this for iOS as well. On iOS, we don't actually need native_theme so I
> have a CL out to remove it from the iOS build
> (https://chromiumcodereview.appspot.com/667223003/).
> 
> Seems like the correct solution is to not have these platforms compiling code
> that refers to resources that the platform doesn't support. Defining the IDR
in
> a header is misleading if the asset isn't actually in the .pak file; I'd
prefer
> a compile-time failure if we're referring to an asset that isn't supported.

Agreed. I would like the compile-time safety here too. I'm not sure how much
work
it'll be to fix all the compile errors, though.

> Since I can't run try jobs against this CL I was thinking I'd land it in grit
> and run try jobs against a CL that rolls DEPS for grit. That should flush out
> issues like the one you've found, which I would fix before landing this CL.
This
> will block rolling DEPS for grit until the chromium builds are fixed, but it
> doesn't look like grit's whitelisting functionality is used very much.

This change affects all platforms, even where a whitelist isn't used, since
node.ActiveDescendants() excludes nodes that appear inside <if> tags that
evaluate
to false. Incidentally, I think iOS is the only platform where a whitelist is
used
during normal compile. Chrome for Android uses an automatically generated
whitelist during official release builds, but nowhere else.

> In any case, I'll work on some unit tests for whitelisting. I found a separate
> case with this CL when grit generates resource maps: grit is adding a
> non-whiltelisted resource to foo_resource_map.cc which causes a compile error
> because the resource is not defined in foo_resource.h. Probably just another
if
> check that's not accounting for StrucutreNode.

Sounds good. Thanks.

[newt (away), 2014-10-23 16:24:39]

cc: Aurimas, who worked on excluding unused resources on Android (using lots of
<if> tags) and the automatic whitelist generation script.

Aurimas: do you have a feel for how much work it would be to ensure that no code
is compiled that references resource IDs (e.g. IDS_BLAH) that have <if>'d out in
the grd file for Android? Is this an attainable goal?

[newt (away), 2014-10-24 01:21:13]

On 2014/10/23 16:24:39, newt wrote:
> cc: Aurimas, who worked on excluding unused resources on Android (using lots
of
> <if> tags) and the automatic whitelist generation script.
> 
> Aurimas: do you have a feel for how much work it would be to ensure that no
code
> is compiled that references resource IDs (e.g. IDS_BLAH) that have <if>'d out
in
> the grd file for Android? Is this an attainable goal?

Aurimas and I had an idea for measuring how many occurrences there are of
IDS_FOO being compiled even though the resource isn't actually included in the
pak file:
 1. Use the whitelist generation script that Aurimas wrote to print out a list
of all resource IDs that are referenced in compiled code
 2. Use this CL (or similar grit magic) to generate a list of all resources that
are actually included in the pak file
 3. Take the difference of these two lists to find the resources that are
referenced during compilation but that aren't actually in the pak file.

Hopefully the final list isn't too large.

[newt (away), 2014-10-24 01:24:42]

Side note: What's the motivation for this CL? Is it that the whitelist used on
iOS keeps getting out of date, but this isn't discovered until a runtime crash
occurs? If so, you should consider using Aurimas's automatic whitelist
generation script. Aurimas can provide more details if that sounds interesting.

[lliabraa, 2014-10-29 15:22:07]

On 2014/10/24 01:24:42, newt wrote:
> Side note: What's the motivation for this CL? Is it that the whitelist used on
> iOS keeps getting out of date, but this isn't discovered until a runtime crash
> occurs? If so, you should consider using Aurimas's automatic whitelist
> generation script. Aurimas can provide more details if that sounds
interesting.

The motivation is mostly that the current code is wrong and causes a bunch of
unused assets to appear in the .pak file and to have references in header files
that shouldn't be there.

Also, the simplest fix for Android would be to include any missing resource IDs
in the whitelist (due to this grit bug, they're already being included anyway),
then cull out the references to them later.

[newt (away), 2014-10-30 03:22:12]

On 2014/10/29 15:22:07, lliabraa wrote:
> On 2014/10/24 01:24:42, newt wrote:
> > Side note: What's the motivation for this CL? Is it that the whitelist used
on
> > iOS keeps getting out of date, but this isn't discovered until a runtime
crash
> > occurs? If so, you should consider using Aurimas's automatic whitelist
> > generation script. Aurimas can provide more details if that sounds
> interesting.
> 
> The motivation is mostly that the current code is wrong and causes a bunch of
> unused assets to appear in the .pak file and to have references in header
files
> that shouldn't be there.

I agree this should be fixed.

> Also, the simplest fix for Android would be to include any missing resource
IDs
> in the whitelist (due to this grit bug, they're already being included
anyway),
> then cull out the references to them later.

Android doesn't use a whitelist (except on release official bots), so I'm not
sure how this would work.

I looked into the specific compile failures on Android. There are two issues:
  1. Several one-off cases where we're compiling files that reference resources
that aren't included in the PAK file. Fixing these should be pretty easy.
  2. grit's resource_map.py generates resource map files (e.g.
out/Debug/gen/ui/resources/grit/ui_resources_map.cc) which map resource names to
resource IDs. These include all resource IDS (because
output_all_resource_defines is true), hence these generated files fail to
compile. To fix this, we need either to set output_all_resource_defines to false
(if that's doable at this point), or to modify resource_map.py to ignore
output_all_resource_defines, as this CL does to rc_header.py.

[newt (away), 2014-10-30 03:32:14]

I've uploaded a CL showing what fixes are needed to get Android to compile with
the changes from this CL:

https://codereview.chromium.org/692733003/

I've added output_all_resource_defines="false" to a few grd files. That's
hopefully legitimate. (Seems like we should add
output_all_resource_defines="false" to all Chrome's grd files, or just make that
the default in GRIT.) I also moved several resources out of <if> clauses. This
isn't a legitimate fix, but it shows which resource IDs will need fixing.

[lliabraa, 2014-10-31 11:35:19]

I've removed the changes to header_rc so this CL should only affect what
resources are included in pak files and not what ends up in header files or
resource maps. Does it work with the android build now?

[newt (away), 2014-10-31 15:58:16]

lgtm

Chrome for Android builds with this patch.

https://chromiumcodereview.appspot.com/669763002/diff/60001/grit/testdata/whi...
File grit/testdata/whitelist_resources.grd (right):

https://chromiumcodereview.appspot.com/669763002/diff/60001/grit/testdata/whi...
grit/testdata/whitelist_resources.grd:18: <release seq="1">
nit: indent

[lliabraa, 2014-10-31 16:05:53]

Thanks for you help on this. Can you land it for me?

https://codereview.chromium.org/669763002/diff/60001/grit/testdata/whitelist_...
File grit/testdata/whitelist_resources.grd (right):

https://codereview.chromium.org/669763002/diff/60001/grit/testdata/whitelist_...
grit/testdata/whitelist_resources.grd:18: <release seq="1">
On 2014/10/31 15:58:15, newt wrote:
> nit: indent

Done.

[newt (away), 2014-10-31 16:24:55]

Committed patchset #5 (id:80001) manually as 179 (presubmit successful).