grit: Add fallback_to_default_layout attribute to <output>

grit: Add fallback_to_default_layout attribute to <output>

Introduce the fallback_to_default_layout attribute
in the grit <output> tag. When this attribute is set
to "false", <structure> nodes of type
chrome_scaled_image will not fall back to searching
the default context if the resource is not present
in the context specified in the context attribute.
No resource will be loaded into the output file
for such structures.

BUG=503635
TEST=ChromeScaledImageUnittest.testNoFallbackToDefaultLayout

[tdanderson, 2015-06-19 18:24:35]

tdanderson@chromium.org changed reviewers:
+ flackr@chromium.org

[tdanderson, 2015-06-19 18:24:35]

Rob, what do you think of this approach? See
https://codereview.chromium.org/1197813002 for a WIP CL which uses the new
attribute.

[flackr, 2015-06-19 19:42:13]

Please add tests for both the set of input files referenced and the pak file
generated.

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
File grit/gather/chrome_scaled_image.py (right):

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
grit/gather/chrome_scaled_image.py:104: allow_empty_path = False
I think the evaluation of this could be condensed a bit, simply:
try_default_layout = self.grd_node.GetRoot().fallback_to_default_layout !=
'false';
allow_empty_path is basically !try_default_layout, so I'd omit it altogether.

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
grit/gather/chrome_scaled_image.py:109: if try_default_layout:
No need for else, so merge with above, i.e.:
if 'default' not in layouts and try_default_layout:

https://codereview.chromium.org/1194063002/diff/1/grit/node/io.py
File grit/node/io.py (right):

https://codereview.chromium.org/1194063002/diff/1/grit/node/io.py#newcode105
grit/node/io.py:105: return self.attrs['fallback_to_default_layout']
I'd convert this to a bool at this point to avoid ugly string comparisons
elsewhere (e.g. return self.attrs['fallback_to_default_layout'] == 'true')

[tdanderson, 2015-06-22 23:33:20]

Rob, please take a look at the test added in patch set 2. Note that in order to
make use of _RunBuildTest() in chrome_scaled_image_unittest.py, I needed to add
checks for empty paths in rc.py, include.py, and structure.py. If adding these
checks do not make sense / could have undesirable consequences, let me know and
I can try an alternate approach to testing.

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
File grit/gather/chrome_scaled_image.py (right):

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
grit/gather/chrome_scaled_image.py:104: allow_empty_path = False
On 2015/06/19 19:42:13, flackr wrote:
> I think the evaluation of this could be condensed a bit, simply:
> try_default_layout = self.grd_node.GetRoot().fallback_to_default_layout !=
> 'false';
> allow_empty_path is basically !try_default_layout, so I'd omit it altogether.

Done.

https://codereview.chromium.org/1194063002/diff/1/grit/gather/chrome_scaled_i...
grit/gather/chrome_scaled_image.py:109: if try_default_layout:
On 2015/06/19 19:42:13, flackr wrote:
> No need for else, so merge with above, i.e.:
> if 'default' not in layouts and try_default_layout:

Done.

https://codereview.chromium.org/1194063002/diff/1/grit/node/io.py
File grit/node/io.py (right):

https://codereview.chromium.org/1194063002/diff/1/grit/node/io.py#newcode105
grit/node/io.py:105: return self.attrs['fallback_to_default_layout']
On 2015/06/19 19:42:13, flackr wrote:
> I'd convert this to a bool at this point to avoid ugly string comparisons
> elsewhere (e.g. return self.attrs['fallback_to_default_layout'] == 'true')

Done. Note I added .lower() in next patch set.

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
File grit/gather/chrome_scaled_image_unittest.py (right):

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
grit/gather/chrome_scaled_image_unittest.py:193: layout_fallback='
fallback_to_default_layout="false"')
todo in next patch set in this cl: generate this string similar to
_MakeFallbackAttr() and test that fallback happens as expected if
fallback_to_default_layout is explicitly set to "true"

[flackr, 2015-06-23 13:33:33]

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
File grit/gather/chrome_scaled_image.py (right):

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
grit/gather/chrome_scaled_image.py:103: try_default_layout =
self.grd_node.GetRoot().fallback_to_default_layout
Hmm, can't we do the same thing as for try_low_res? (i.e. just searching the grd
node) I think this would avoid the need to add fallback_to_default_layout to so
many other locations (io.py, misc.py, build.py, grit_info.py).

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
File grit/gather/chrome_scaled_image_unittest.py (right):

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
grit/gather/chrome_scaled_image_unittest.py:191: 'tactile_123_percent':
set([t123a]),
It looks like expected_output only verifies that everything specified is in the
pak, but for this test we want to verify that no extra resources from the base
pak are in the tactile pak.

[tdanderson, 2015-06-23 14:22:55]

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
File grit/gather/chrome_scaled_image.py (right):

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
grit/gather/chrome_scaled_image.py:103: try_default_layout =
self.grd_node.GetRoot().fallback_to_default_layout
On 2015/06/23 13:33:33, flackr wrote:
> Hmm, can't we do the same thing as for try_low_res? (i.e. just searching the
grd
> node) I think this would avoid the need to add fallback_to_default_layout to
so
> many other locations (io.py, misc.py, build.py, grit_info.py).

I considered that, but the corresponding <output> node is not on the path from
self.grd node to the root, so FindBooleanAttribute() won't catch it. I also
could not find a way to access the <output> node directly, so I ended up copying
what was done for output_context.

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
File grit/gather/chrome_scaled_image_unittest.py (right):

https://codereview.chromium.org/1194063002/diff/20001/grit/gather/chrome_scal...
grit/gather/chrome_scaled_image_unittest.py:191: 'tactile_123_percent':
set([t123a]),
On 2015/06/23 13:33:33, flackr wrote:
> It looks like expected_output only verifies that everything specified is in
the
> pak, but for this test we want to verify that no extra resources from the base
> pak are in the tactile pak.

I believe that's what we're doing. The check in _RunBuildTest() at line 112 is
for equality, and the tactile pak should contain only one png (t123a). If
fallback_to_default_layout="false" were not specified, then the tactile pak
would also contain d123b (see testNormalFallback()).

[tdanderson, 2015-06-23 16:26:23]

Rob, patch set 3 should be good to land.

[flackr, 2015-06-23 16:56:41]

LGTM, committed revision 190.