Use output_all_resource_defines=false in some chrome/ grd files.

Use output_all_resource_defines=false in some chrome/ grd files.

BUG=333201
Committed: https://crrev.com/06a903776919712f01ce7ffce0847409d54e5343
Cr-Commit-Position: refs/heads/master@{#406116}

[Lei Zhang, 2016-07-11 17:42:20]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2016-07-12 18:22:10]

is this a relevant error?
[3524:1476:0710/000542:ERROR:memory_mapped_file.cc(52)] Couldn't open
c:\users\chrome~1\appdata\local\temp\isolated_runlqc_ij\out\Release\chrome_200_percent.pak

[Lei Zhang, 2016-07-12 18:54:03]

On 2016/07/12 18:22:10, Evan Stade wrote:
> is this a relevant error?
> [3524:1476:0710/000542:ERROR:memory_mapped_file.cc(52)] Couldn't open
>
c:\users\chrome~1\appdata\local\temp\isolated_runlqc_ij\out\Release\chrome_200_percent.pak

Probably not. I looked at another run on the same bot, 252714, and it has the
same message.

[Lei Zhang, 2016-07-12 18:55:55]

So this CL is one where we have to change the C++ code to match the .grd files.

[Evan Stade, 2016-07-12 19:10:09]

I'm not really a huge fan of these c++ changes which takes us back to my
question on the other CL --- what's the purported benefit of this change? "It
forces the C++ side to match the <if> conditions in .grd files." That doesn't
seem like a win in this case.

[Lei Zhang, 2016-07-12 19:14:25]

On 2016/07/12 19:10:09, Evan Stade wrote:
> I'm not really a huge fan of these c++ changes which takes us back to my
> question on the other CL --- what's the purported benefit of this change? "It
> forces the C++ side to match the <if> conditions in .grd files." That doesn't
> seem like a win in this case.

Taking the shell_integration_linux.* change for example, why are we building
AppList code when AppList isn't enabled? Isn't it a win to not build code that
would just fail at runtime?

[Evan Stade, 2016-07-12 19:22:16]

On 2016/07/12 19:14:25, Lei Zhang wrote:
> On 2016/07/12 19:10:09, Evan Stade wrote:
> > I'm not really a huge fan of these c++ changes which takes us back to my
> > question on the other CL --- what's the purported benefit of this change?
"It
> > forces the C++ side to match the <if> conditions in .grd files." That
doesn't
> > seem like a win in this case.
> 
> Taking the shell_integration_linux.* change for example, why are we building
> AppList code when AppList isn't enabled? Isn't it a win to not build code that
> would just fail at runtime?

I guess that's a matter for some debate, as I've tried removing dead code but
been blocked because I used too many ifdefs. Sometimes I think people have an
unreasonable aversion to ifdefs.

That said, AppList example doesn't seem bad to me. I dislike the
download_commands.cc change. To follow the AppList example we should remove more
dead code (pretty sure GetCommandIcon is never called outside of cros).

[Lei Zhang, 2016-07-12 20:41:25]

On 2016/07/12 19:22:16, Evan Stade wrote:
> That said, AppList example doesn't seem bad to me. I dislike the
> download_commands.cc change. To follow the AppList example we should remove
more
> dead code (pretty sure GetCommandIcon is never called outside of cros).

I reverted the downloads changes since they aren't needed for this CL as of
patch set 3. There's other problems with theme_resources.grd, so I'll punt on
downloads to later.

[Evan Stade, 2016-07-15 18:29:32]

lgtm

https://codereview.chromium.org/2129413002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/component_extensions_whitelist/whitelist.cc
(right):

https://codereview.chromium.org/2129413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc:126: for
(size_t i = 0; i < arraysize(allowed); ++i) {
nit: why doesn't this function employ a switch/case block?

[Lei Zhang, 2016-07-16 00:35:50]

https://codereview.chromium.org/2129413002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/component_extensions_whitelist/whitelist.cc
(right):

https://codereview.chromium.org/2129413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc:126: for
(size_t i = 0; i < arraysize(allowed); ++i) {
On 2016/07/15 18:29:31, Evan Stade wrote:
> nit: why doesn't this function employ a switch/case block?

Probably was a copy + paste from IsComponentExtensionWhitelisted() above, which
cannot use a switch block. Using a switch now.

[Lei Zhang, 2016-07-16 00:52:36]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-07-16 00:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2129413002/#ps100001
(title: "rebase, address comment, more whitelist.cc cleanup")

[commit-bot: I haz the power, 2016-07-16 00:52:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-16 01:02:00]

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

[commit-bot: I haz the power, 2016-07-16 01:02:01]

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

[Lei Zhang, 2016-07-16 01:12:27]

thestig@chromium.org changed reviewers:
+ brettw@chromium.org

[Lei Zhang, 2016-07-16 01:12:28]

brettw: PTAL at whitelist.cc. It's just refactoring. No new component extension
has been added.

[brettw, 2016-07-18 19:47:16]

lgtm

[Lei Zhang, 2016-07-18 20:15:09]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-07-18 20:15:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 22:04:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-18 22:08:03]

Description was changed from

==========
Use output_all_resource_defines=false in some chrome/ grd files.

BUG=333201
==========

to

==========
Use output_all_resource_defines=false in some chrome/ grd files.

BUG=333201

Committed: https://crrev.com/06a903776919712f01ce7ffce0847409d54e5343
Cr-Commit-Position: refs/heads/master@{#406116}
==========

[commit-bot: I haz the power, 2016-07-18 22:08:04]

Patchset 6 (id:??) landed as
https://crrev.com/06a903776919712f01ce7ffce0847409d54e5343
Cr-Commit-Position: refs/heads/master@{#406116}