extensions: Remove dependencies on Chrome resources.

extensions: Remove dependencies on Chrome resources.

Remove dependencies on Chrome resources from extensions.gyp.

BUG=397250
Committed: https://crrev.com/1bbe3745194ab91cc90da33d0cfb0be5b53fc4e3
Cr-Commit-Position: refs/heads/master@{#297847}

[Daniel Erat, 2014-10-01 19:22:46]

derat@chromium.org changed reviewers:
+ jamescook@chromium.org, yoz@chromium.org

[Daniel Erat, 2014-10-01 19:22:46]

this seemed way too easy. the app_shell, athena_main, and chrome targets all
build (with chromeos=1, clang=1, and component=shared_library) and run after i
blow away my output directory. should i be doing something else to test this?

[Yoyo Zhou, 2014-10-01 20:56:22]

On 2014/10/01 19:22:46, Daniel Erat wrote:
> this seemed way too easy. the app_shell, athena_main, and chrome targets all
> build (with chromeos=1, clang=1, and component=shared_library) and run after i
> blow away my output directory. should i be doing something else to test this?

It's certainly possible that all the resources have been factored out already,
although missing resources are usually exposed at run time (like when you run
app_shell, or tests) rather than build time.

Perhaps try adding a reference to a now non-included chrome resource and see how
it fails? (I'm not sure exactly where or how easy this would be...)

[James Cook, 2014-10-01 21:11:57]

have to run to a meeting but I would try:

extensions_unittests
app_shell_browsertests

Add a raw reference to getResource(12345) (get the number out of a chrome
generated_resources.h file) and run app_shell in Debug. You should hit a DCHECK
in ResourceBundle.

[Yoyo Zhou, 2014-10-01 21:17:23]

On 2014/10/01 21:11:57, James Cook wrote:
> have to run to a meeting but I would try:
> 
> extensions_unittests
> app_shell_browsertests
> 
> Add a raw reference to getResource(12345) (get the number out of a chrome
> generated_resources.h file) and run app_shell in Debug. You should hit a
DCHECK
> in ResourceBundle.

Ah, yes, good point - you will only crash (DCHECK) in Debug, so the trybots
running Release won't necessarily catch it.

[Daniel Erat, 2014-10-01 21:39:54]

On 2014/10/01 21:17:23, Yoyo Zhou wrote:
> On 2014/10/01 21:11:57, James Cook wrote:
> > have to run to a meeting but I would try:
> > 
> > extensions_unittests
> > app_shell_browsertests
> > 
> > Add a raw reference to getResource(12345) (get the number out of a chrome
> > generated_resources.h file) and run app_shell in Debug. You should hit a
> DCHECK
> > in ResourceBundle.
> 
> Ah, yes, good point - you will only crash (DCHECK) in Debug, so the trybots
> running Release won't necessarily catch it.

i do see a DCHECK() when i do that, but shouldn't we see compilation errors if
we're trying to use resources that aren't present? we're not hardcoding the raw
IDs, so shouldn't it fail to build if we're trying use a #define from a
nonexistent gen/chrome/grit/generated_resources.h file?

[Yoyo Zhou, 2014-10-01 21:43:55]

On 2014/10/01 21:39:54, Daniel Erat wrote:
> On 2014/10/01 21:17:23, Yoyo Zhou wrote:
> > On 2014/10/01 21:11:57, James Cook wrote:
> > > have to run to a meeting but I would try:
> > > 
> > > extensions_unittests
> > > app_shell_browsertests
> > > 
> > > Add a raw reference to getResource(12345) (get the number out of a chrome
> > > generated_resources.h file) and run app_shell in Debug. You should hit a
> > DCHECK
> > > in ResourceBundle.
> > 
> > Ah, yes, good point - you will only crash (DCHECK) in Debug, so the trybots
> > running Release won't necessarily catch it.
> 
> i do see a DCHECK() when i do that, but shouldn't we see compilation errors if
> we're trying to use resources that aren't present? we're not hardcoding the
raw
> IDs, so shouldn't it fail to build if we're trying use a #define from a
> nonexistent gen/chrome/grit/generated_resources.h file?

In my experience, the linker is only this strict on Windows.

[Daniel Erat, 2014-10-01 21:45:50]

On 2014/10/01 21:43:55, Yoyo Zhou wrote:
> On 2014/10/01 21:39:54, Daniel Erat wrote:
> > On 2014/10/01 21:17:23, Yoyo Zhou wrote:
> > > On 2014/10/01 21:11:57, James Cook wrote:
> > > > have to run to a meeting but I would try:
> > > > 
> > > > extensions_unittests
> > > > app_shell_browsertests
> > > > 
> > > > Add a raw reference to getResource(12345) (get the number out of a
chrome
> > > > generated_resources.h file) and run app_shell in Debug. You should hit a
> > > DCHECK
> > > > in ResourceBundle.
> > > 
> > > Ah, yes, good point - you will only crash (DCHECK) in Debug, so the
trybots
> > > running Release won't necessarily catch it.
> > 
> > i do see a DCHECK() when i do that, but shouldn't we see compilation errors
if
> > we're trying to use resources that aren't present? we're not hardcoding the
> raw
> > IDs, so shouldn't it fail to build if we're trying use a #define from a
> > nonexistent gen/chrome/grit/generated_resources.h file?
> 
> In my experience, the linker is only this strict on Windows.

i don't follow. how do you use an identifier that isn't #define-ed?

[Yoyo Zhou, 2014-10-01 21:53:35]

On 2014/10/01 21:45:50, Daniel Erat wrote:
> On 2014/10/01 21:43:55, Yoyo Zhou wrote:
> > On 2014/10/01 21:39:54, Daniel Erat wrote:
> > > On 2014/10/01 21:17:23, Yoyo Zhou wrote:
> > > > On 2014/10/01 21:11:57, James Cook wrote:
> > > > > have to run to a meeting but I would try:
> > > > > 
> > > > > extensions_unittests
> > > > > app_shell_browsertests
> > > > > 
> > > > > Add a raw reference to getResource(12345) (get the number out of a
> chrome
> > > > > generated_resources.h file) and run app_shell in Debug. You should hit
a
> > > > DCHECK
> > > > > in ResourceBundle.
> > > > 
> > > > Ah, yes, good point - you will only crash (DCHECK) in Debug, so the
> trybots
> > > > running Release won't necessarily catch it.
> > > 
> > > i do see a DCHECK() when i do that, but shouldn't we see compilation
errors
> if
> > > we're trying to use resources that aren't present? we're not hardcoding
the
> > raw
> > > IDs, so shouldn't it fail to build if we're trying use a #define from a
> > > nonexistent gen/chrome/grit/generated_resources.h file?
> > 
> > In my experience, the linker is only this strict on Windows.
> 
> i don't follow. how do you use an identifier that isn't #define-ed?

I think the header gets built (because you built chrome?) and linked in, even if
it's not included in the gyp.

[Daniel Erat, 2014-10-01 21:54:43]

On 2014/10/01 21:53:35, Yoyo Zhou wrote:
> On 2014/10/01 21:45:50, Daniel Erat wrote:
> > On 2014/10/01 21:43:55, Yoyo Zhou wrote:
> > > On 2014/10/01 21:39:54, Daniel Erat wrote:
> > > > On 2014/10/01 21:17:23, Yoyo Zhou wrote:
> > > > > On 2014/10/01 21:11:57, James Cook wrote:
> > > > > > have to run to a meeting but I would try:
> > > > > > 
> > > > > > extensions_unittests
> > > > > > app_shell_browsertests
> > > > > > 
> > > > > > Add a raw reference to getResource(12345) (get the number out of a
> > chrome
> > > > > > generated_resources.h file) and run app_shell in Debug. You should
hit
> a
> > > > > DCHECK
> > > > > > in ResourceBundle.
> > > > > 
> > > > > Ah, yes, good point - you will only crash (DCHECK) in Debug, so the
> > trybots
> > > > > running Release won't necessarily catch it.
> > > > 
> > > > i do see a DCHECK() when i do that, but shouldn't we see compilation
> errors
> > if
> > > > we're trying to use resources that aren't present? we're not hardcoding
> the
> > > raw
> > > > IDs, so shouldn't it fail to build if we're trying use a #define from a
> > > > nonexistent gen/chrome/grit/generated_resources.h file?
> > > 
> > > In my experience, the linker is only this strict on Windows.
> > 
> > i don't follow. how do you use an identifier that isn't #define-ed?
> 
> I think the header gets built (because you built chrome?) and linked in, even
if
> it's not included in the gyp.

ah. i didn't build chrome this time -- i deleted the directory again and only
built app_shell; gen/chrome/grit/generated_resources.h doesn't exist.

[James Cook, 2014-10-02 00:38:01]

LGTM assuming extensions_unittests and app_shell_browsertests passed without
DCHECK in debug

On further reflection I think you're safe if it compiles. Our DEPS files used to
allow +grit or +grit/generated_resources.h. They don't anymore, only
+grit/extensions_* is allowed. So you shouldn't be able to reference an IDR_ or
IDS_ from //chrome, only //extensions.

https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp
File extensions/extensions.gyp (left):

https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp#ol...
extensions/extensions.gyp:301: '<(SHARED_INTERMEDIATE_DIR)/chrome',
That it compiles without this bit reassures me.

[Daniel Erat, 2014-10-02 00:39:17]

On 2014/10/02 00:38:01, James Cook wrote:
> LGTM assuming extensions_unittests and app_shell_browsertests passed without
> DCHECK in debug
> 
> On further reflection I think you're safe if it compiles. Our DEPS files used
to
> allow +grit or +grit/generated_resources.h. They don't anymore, only
> +grit/extensions_* is allowed. So you shouldn't be able to reference an IDR_
or
> IDS_ from //chrome, only //extensions.
> 
> https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp
> File extensions/extensions.gyp (left):
> 
>
https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp#ol...
> extensions/extensions.gyp:301: '<(SHARED_INTERMEDIATE_DIR)/chrome',
> That it compiles without this bit reassures me.

(yep, both of those test suites passed in debug mode)

[James Cook, 2014-10-02 00:48:09]

On 2014/10/02 00:39:17, Daniel Erat wrote:
> On 2014/10/02 00:38:01, James Cook wrote:
> > LGTM assuming extensions_unittests and app_shell_browsertests passed without
> > DCHECK in debug
> > 
> > On further reflection I think you're safe if it compiles. Our DEPS files
used
> to
> > allow +grit or +grit/generated_resources.h. They don't anymore, only
> > +grit/extensions_* is allowed. So you shouldn't be able to reference an IDR_
> or
> > IDS_ from //chrome, only //extensions.
> > 
> > https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp
> > File extensions/extensions.gyp (left):
> > 
> >
>
https://codereview.chromium.org/615913003/diff/1/extensions/extensions.gyp#ol...
> > extensions/extensions.gyp:301: '<(SHARED_INTERMEDIATE_DIR)/chrome',
> > That it compiles without this bit reassures me.
> 
> (yep, both of those test suites passed in debug mode)

ship it!

[Daniel Erat, 2014-10-02 00:57:39]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-02 00:59:13]

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

[commit-bot: I haz the power, 2014-10-02 01:09:36]

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

[commit-bot: I haz the power, 2014-10-02 01:09:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2014-10-02 01:09:44]

yoyo, el gee tee em please if you're happy with this

[Yoyo Zhou, 2014-10-02 16:21:49]

On 2014/10/02 01:09:44, Daniel Erat wrote:
> yoyo, el gee tee em please if you're happy with this

LGTM

[Yoyo Zhou, 2014-10-02 16:21:53]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-10-02 16:22:46]

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

[commit-bot: I haz the power, 2014-10-02 16:26:41]

Committed patchset #1 (id:1) as 236b4d79f62332cf4e10f33527b3127a39451d1a

[commit-bot: I haz the power, 2014-10-02 16:27:25]

Patchset 1 (id:??) landed as
https://crrev.com/1bbe3745194ab91cc90da33d0cfb0be5b53fc4e3
Cr-Commit-Position: refs/heads/master@{#297847}