resources: Prevent including the same resource in multiple pack files.

resources: Prevent including the same resource in multiple pack files.

Add code to cause failure at runtime if a resource-pack includes a
resource that is already present in another resource-pack.

BUG=471609
Committed: https://crrev.com/618bc16b18999137337aaed4ad87f99e8563093c
Cr-Commit-Position: refs/heads/master@{#329646}

[sadrul, 2015-05-01 15:22:45]

sadrul@chromium.org changed reviewers:
+ thakis@chromium.org

[sadrul, 2015-05-01 15:22:45]

Hi. I reworked https://codereview.chromium.org/1056993003/ to make it a runtime
check turned on all platforms (when DCHECK_IS_ON), instead of a build-time check
for android. This helped find some issues in the current code, so this CL
includes fixes for those as well. Do you think this would be useful to have? I
was a bit worried that perhaps this would affect runtime in the bots, but
doesn't seem to (although I am not sure exactly how to interpret the buildbot UI
when it comes to runtime for individual tests anymore).

[Nico, 2015-05-02 00:50:48]

I like the idea of this CL a lot. I'm a bit surprised about loading the ui test
pak in a different test. I saw that other code does this too, I asked about that
in https://codereview.chromium.org/477923004/ .

Are the keyboard changes intentionally part of this CL? They don't look
obviously related to me.

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
File ash/test/ash_test_helper.cc (left):

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
ash/test/ash_test_helper.cc:129: keyboard::ResetKeyboardForTesting();
?

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
File ui/base/resource/data_pack.cc (right):

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:239: for (size_t i = 0; i < resource_count_ + 1;
++i) {
why + 1?

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:249: });
nit: Writing this as

    for (const ResourceHandle* handle : packs) {
      DCHECK(GetScaleForScaleFactor(handle->GetScaleFactor()) !=
                 resource_scale ||
             !handle->HasResource(resource_id));
    }

is both shorter and more readable

[sadrul, 2015-05-02 04:17:09]

On 2015/05/02 00:50:48, Nico wrote:
> I like the idea of this CL a lot. I'm a bit surprised about loading the ui
test
> pak in a different test. I saw that other code does this too, I asked about
that
> in https://codereview.chromium.org/477923004/ .
> 
> Are the keyboard changes intentionally part of this CL? They don't look
> obviously related to me.

The keyboard changes are indeed needed for this change (otherwise the new check
triggers a crash (see ash_unittests failures in patchset 1)). But they can also
land separately before the check in DataPack, if you would prefer that.

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
File ash/test/ash_test_helper.cc (left):

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
ash/test/ash_test_helper.cc:129: keyboard::ResetKeyboardForTesting();
On 2015/05/02 00:50:48, Nico wrote:
> ?

A new instance of ash::Shell gets created for each test, and it initializes the
virtual keyboard upon creation. The virtual keyboard installs the resource-pak
to the resource-bundle when it is initialized. However, the resource-bundle is
initialized only once for the entire test-suite. This trips the new check, since
the same resource-pak is attempted to load multiple times. So for the ash tests,
the keyboard is reset with the resource-bundle, so that the virtual keyboard
initialization does not attempt to load the resource-pak for each test.

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
File ui/base/resource/data_pack.cc (right):

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:239: for (size_t i = 0; i < resource_count_ + 1;
++i) {
On 2015/05/02 00:50:48, Nico wrote:
> why + 1?

I am following the code from DataPack::LoadImpl() above (lines 157+). I
considered introducing a
DataPack::ForEachDataPackEntry(void(*func)(DataPackEntry*)), and use that from
both places, since this looks a bit subtle, but decided against it. WDYT?

https://codereview.chromium.org/1115033003/diff/150001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:249: });
On 2015/05/02 00:50:48, Nico wrote:
> nit: Writing this as
> 
>     for (const ResourceHandle* handle : packs) {
>       DCHECK(GetScaleForScaleFactor(handle->GetScaleFactor()) !=
>                  resource_scale ||
>              !handle->HasResource(resource_id));
>     }
> 
> is both shorter and more readable

Heh, yeah. Totally. Done.

[sadrul, 2015-05-05 02:38:13]

On 2015/05/02 00:50:48, Nico wrote:
> ... I'm a bit surprised about loading the ui test pak in a different test. ...

This change removes the resources that are already in ui-test pak from
components_tests_resources pak. This is necessary since components_browsertests
loads content_shell.pak (which also includes the ui test resources), and so
trying to load both content_shell.pak and components_tests_resources.pak trips
the check in the CL. That is why it is necessary to load the ui-test pak for
components_unittests (which also loads components_tests_resources.pak). An
alternate fix for this would be to create separate test-resource paks for
components_unittests and components_browsertests.

[Nico, 2015-05-06 01:06:35]

Thanks for explaining. Basically lgtm, but someone familiar with the keyboard
code should look at the keyboard change (either in this review or you can pull
it out to its own review, up to you)

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
File ash/test/ash_test_helper.cc (left):

https://codereview.chromium.org/1115033003/diff/150001/ash/test/ash_test_help...
ash/test/ash_test_helper.cc:129: keyboard::ResetKeyboardForTesting();
On 2015/05/02 04:17:09, sadrul wrote:
> On 2015/05/02 00:50:48, Nico wrote:
> > ?
> 
> A new instance of ash::Shell gets created for each test, and it initializes
the
> virtual keyboard upon creation. The virtual keyboard installs the resource-pak
> to the resource-bundle when it is initialized. However, the resource-bundle is
> initialized only once for the entire test-suite. This trips the new check,
since
> the same resource-pak is attempted to load multiple times. So for the ash
tests,
> the keyboard is reset with the resource-bundle, so that the virtual keyboard
> initialization does not attempt to load the resource-pak for each test.

I would've guessed that the intention of this is that the virtual keyboard is
dismissed after each test if it's open. With this change, it's only dismissed
once at the end of all tests.

Maybe it's good to land this separately and have someone familiar with the
virtual keyboard review it.

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
tools/gritsettings/resource_ids:219: "includes": [28120],
why this change?

https://codereview.chromium.org/1115033003/diff/190001/ui/base/resource/data_...
File ui/base/resource/data_pack.cc (right):

https://codereview.chromium.org/1115033003/diff/190001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:245: !handle->HasResource(resource_id));
(nit: if you want I think you can use dcheck_implies for this)

[sadrul, 2015-05-06 17:29:24]

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
tools/gritsettings/resource_ids:219: "includes": [28120],
On 2015/05/06 01:06:35, Nico wrote:
> why this change?

keyboard_resources.grd has more than 50 resources. So some of the resources in
keyboard_resources.grd have conflicting resource-ids as some of the resources in
file_manager_resources.grd (and helpfully trips the new DCHECK). This was a bit
surprising to me. Are there other existing tools/checks that are supposed to
have caught this?

https://codereview.chromium.org/1115033003/diff/190001/ui/base/resource/data_...
File ui/base/resource/data_pack.cc (right):

https://codereview.chromium.org/1115033003/diff/190001/ui/base/resource/data_...
ui/base/resource/data_pack.cc:245: !handle->HasResource(resource_id));
On 2015/05/06 01:06:35, Nico wrote:
> (nit: if you want I think you can use dcheck_implies for this)

Done.

[sadrul, 2015-05-06 17:32:32]

sadrul@chromium.org changed reviewers:
+ blundell@chromium.org

[sadrul, 2015-05-06 17:32:33]

+blundell@: Please review changes in components/

[ I moved off the ash/keyboard changes in
https://codereview.chromium.org/1130753004 ]

[Nico, 2015-05-06 17:34:59]

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
tools/gritsettings/resource_ids:219: "includes": [28120],
On 2015/05/06 17:29:24, sadrul wrote:
> On 2015/05/06 01:06:35, Nico wrote:
> > why this change?
> 
> keyboard_resources.grd has more than 50 resources. So some of the resources in
> keyboard_resources.grd have conflicting resource-ids as some of the resources
in
> file_manager_resources.grd (and helpfully trips the new DCHECK). This was a
bit
> surprising to me. Are there other existing tools/checks that are supposed to
> have caught this?

I thought we had something that checks that grd file ids don't overlap, but I'm
not sure. Maybe thestig knows?

[sadrul, 2015-05-06 18:28:25]

On 2015/05/06 17:34:59, Nico wrote:
>
https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
> File tools/gritsettings/resource_ids (right):
> 
>
https://codereview.chromium.org/1115033003/diff/190001/tools/gritsettings/res...
> tools/gritsettings/resource_ids:219: "includes": [28120],
> On 2015/05/06 17:29:24, sadrul wrote:
> > On 2015/05/06 01:06:35, Nico wrote:
> > > why this change?
> > 
> > keyboard_resources.grd has more than 50 resources. So some of the resources
in
> > keyboard_resources.grd have conflicting resource-ids as some of the
resources
> in
> > file_manager_resources.grd (and helpfully trips the new DCHECK). This was a
> bit
> > surprising to me. Are there other existing tools/checks that are supposed to
> > have caught this?
> 
> I thought we had something that checks that grd file ids don't overlap, but
I'm
> not sure. Maybe thestig knows?

It looks like the repack step has some check: it fails when resources with the
same resource-id is repacked into the same pak file. However, in this case, the
keyboard resources are not packed with the files-app resources, and so that
check doesn't catch this.

[Lei Zhang, 2015-05-06 21:11:24]

On 2015/05/06 17:34:59, Nico wrote:
> I thought we had something that checks that grd file ids don't overlap, but
I'm
> not sure. Maybe thestig knows?

I don't remember such a thing, but if I did write it and you find it, I would
love to know. :)

[blundell, 2015-05-07 08:25:43]

https://codereview.chromium.org/1115033003/diff/250001/components/test/run_al...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1115033003/diff/250001/components/test/run_al...
components/test/run_all_unittests.cc:65: #if defined(OS_IOS) ||
defined(OS_ANDROID)
Why is the loading of the ui test pak conditional on the OS here? That doesn't
seem to match up with how it was working before, unless I'm missing something in
the build files.

[sadrul, 2015-05-07 08:46:14]

https://codereview.chromium.org/1115033003/diff/250001/components/test/run_al...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1115033003/diff/250001/components/test/run_al...
components/test/run_all_unittests.cc:65: #if defined(OS_IOS) ||
defined(OS_ANDROID)
On 2015/05/07 08:25:43, blundell wrote:
> Why is the loading of the ui test pak conditional on the OS here? That doesn't
> seem to match up with how it was working before, unless I'm missing something
in
> the build files.

I tried unconditionally loading ui test pak in patchset 5, but could not grok
the failures on ios/android trybots. I have sent out another set of tries with
this removed (patchset 15), but the components_unittests failure on ios still
isn't particularly helpful
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...).
I can dig into this a bit more if there's some way to symbolize the backtraces
from the crashes?

[sadrul, 2015-05-07 19:05:06]

Patchset #17 (id:310001) has been deleted

[sadrul, 2015-05-07 22:26:35]

Finally figured the failures on ios and android:

 . In the case of android: the PathService was returning the incorrect path for
UI_TEST_PAK. The fix for this is in ui_base_paths.cc.
 . In the case of ios: The ui_test.pak was not being copied correctly for
components_unittests. The fix for this is in components_tests.gyp (the change
regarding mac_bundle_resources).

PTAL.

[Nico, 2015-05-07 22:46:46]

thakis@chromium.org changed reviewers:
+ benm@chromium.org, rohitrao@chromium.org

[Nico, 2015-05-07 22:46:47]

benm, can you look at the ui/base/ui_base_paths.cc change?

rohitrao: Can you spot-check the component_tests.gyp change? (Looks fine to me,
but maybe ios is weird)

[rohitrao (ping after 24h), 2015-05-07 22:58:15]

components_tests.gyp LGTM.  Adding the .pak to mac_bundle_resources should be
enough to ensure that it gets included in the .app.

[blundell, 2015-05-11 07:37:28]

//components lgtm

Thanks!

[sadrul, 2015-05-11 15:12:34]

sadrul@chromium.org changed reviewers:
+ torne@chromium.org

[sadrul, 2015-05-11 15:13:15]

On 2015/05/07 22:46:47, Nico wrote:
> benm, can you look at the ui/base/ui_base_paths.cc change?

benm@ is '(inactive)'. So +torne@

[Torne, 2015-05-13 11:55:26]

Android pak directory change LGTM.

[sadrul, 2015-05-13 12:48:57]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-13 12:48:58]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1115033003/#ps370001
(title: ".")

[commit-bot: I haz the power, 2015-05-13 12:49:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115033003/370001

[commit-bot: I haz the power, 2015-05-13 13:27:00]

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

[commit-bot: I haz the power, 2015-05-13 13:27:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2015-05-13 14:59:45]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-13 14:59:45]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
blundell@chromium.org, torne@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1115033003/#ps390001
(title: "tot-merge")

[commit-bot: I haz the power, 2015-05-13 15:00:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115033003/390001

[commit-bot: I haz the power, 2015-05-13 16:10:06]

Committed patchset #20 (id:390001)

[commit-bot: I haz the power, 2015-05-13 16:10:56]

Patchset 20 (id:??) landed as
https://crrev.com/618bc16b18999137337aaed4ad87f99e8563093c
Cr-Commit-Position: refs/heads/master@{#329646}