Change grit whitelist to a string parameter

Change grit whitelist to a string parameter

This CL changes the grit_whitelist parameter to be a string instead of a
part of grit_defines.
This allow user to override the parameter with another file.

The goal of this is to allow creating bundles with their own whitelists.

At the moment, if you want to create a small executable (i.e. an ios extension)
that only uses 3 strings, you are required to include all the strings
whitelisted by the main chrome bundle.
By allowing to override the whitelist argument (instead of only appending the
argument array, it will allow to specify a specific whitelist for a specific
target.

BUG=456837
Committed: https://crrev.com/2310fc2b85b31504de87ba22d9d14fc406ed3147
Cr-Commit-Position: refs/heads/master@{#315324}

[Olivier, 2015-01-21 14:04:29]

olivierrobin@chromium.org changed reviewers:
+ lliabra@chromium.org, sdefresne@chromium.org

[sdefresne, 2015-01-23 21:15:25]

Did you forget to send email asking for review? LGTM

[Olivier, 2015-02-05 16:52:36]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2015-02-05 16:52:48]

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

[Olivier, 2015-02-05 16:53:14]

The CQ bit was unchecked by olivierrobin@chromium.org

[Olivier, 2015-02-05 17:19:08]

+ thakis for owner

[Olivier, 2015-02-09 09:01:23]

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

[Nico, 2015-02-09 15:13:50]

Neither cl description nor bug link (not present) explain why one would want to
override this (there's even a "TODO: remove this whitelist" in the code)

[Olivier, 2015-02-09 15:20:47]

On 2015/02/09 15:13:50, Nico wrote:
> Neither cl description nor bug link (not present) explain why one would want
to
> override this (there's even a "TODO: remove this whitelist" in the code)

The goal of the CL is to allow creating bundles with their own whitelists.

At the moment, if you want to create a small executable (i.e. an ios extension)
that only uses 3 strings, you are required to include all the strings
whitelisted by the main chrome bundle.
By allowing to override the whitelist argument (instead of only appending the
argument array, it will allow to specify a specific whitelist for a specific
target.

The TODO: remove this whitelist was present before the CL

[Nico, 2015-02-09 15:44:00]

On 2015/02/09 15:20:47, Olivier Robin wrote:
> On 2015/02/09 15:13:50, Nico wrote:
> > Neither cl description nor bug link (not present) explain why one would want
> to
> > override this (there's even a "TODO: remove this whitelist" in the code)
> 
> The goal of the CL is to allow creating bundles with their own whitelists.
> 
> At the moment, if you want to create a small executable (i.e. an ios
extension)
> that only uses 3 strings, you are required to include all the strings
> whitelisted by the main chrome bundle.
> By allowing to override the whitelist argument (instead of only appending the
> argument array, it will allow to specify a specific whitelist for a specific
> target.

Right, but things like this should be in cl descriptions and / or tracking bugs.

It sounds like the problem is that build/common.gypi adds `'-w',
'<(DEPTH)/build/ios/grit_whitelist.txt',` unconditionally for all targets. Maybe
it should stop doing that? Things in build/common.gypi are for global defaults
that are correct for every target; sounds like that's no longer true for the
whitelist?

[Olivier, 2015-02-09 16:07:04]

On 2015/02/09 15:44:00, Nico wrote:
> On 2015/02/09 15:20:47, Olivier Robin wrote:
> > On 2015/02/09 15:13:50, Nico wrote:
> > > Neither cl description nor bug link (not present) explain why one would
want
> > to
> > > override this (there's even a "TODO: remove this whitelist" in the code)
> > 
> > The goal of the CL is to allow creating bundles with their own whitelists.
> > 
> > At the moment, if you want to create a small executable (i.e. an ios
> extension)
> > that only uses 3 strings, you are required to include all the strings
> > whitelisted by the main chrome bundle.
> > By allowing to override the whitelist argument (instead of only appending
the
> > argument array, it will allow to specify a specific whitelist for a specific
> > target.
> 
> Right, but things like this should be in cl descriptions and / or tracking
bugs.
> 
> It sounds like the problem is that build/common.gypi adds `'-w',
> '<(DEPTH)/build/ios/grit_whitelist.txt',` unconditionally for all targets.
Maybe
> it should stop doing that? Things in build/common.gypi are for global defaults
> that are correct for every target; sounds like that's no longer true for the
> whitelist?

Added the bug.

If we consider '<(DEPTH)/build/ios/grit_whitelist.txt'  as a global default
value and not an unconditional value, it still make sense to define it in
common.gypi as it must be included in all bundles (application and tests) except
if it is specifically overridden.

That was also the case of grit_defines which was overridden in
ios_chrome_resources.gyp (but adding a whitelist instead of replacing)

[Nico, 2015-02-09 16:43:59]

On 2015/02/09 16:07:04, Olivier Robin wrote:
> On 2015/02/09 15:44:00, Nico wrote:
> > On 2015/02/09 15:20:47, Olivier Robin wrote:
> > > On 2015/02/09 15:13:50, Nico wrote:
> > > > Neither cl description nor bug link (not present) explain why one would
> want
> > > to
> > > > override this (there's even a "TODO: remove this whitelist" in the code)
> > > 
> > > The goal of the CL is to allow creating bundles with their own whitelists.
> > > 
> > > At the moment, if you want to create a small executable (i.e. an ios
> > extension)
> > > that only uses 3 strings, you are required to include all the strings
> > > whitelisted by the main chrome bundle.
> > > By allowing to override the whitelist argument (instead of only appending
> the
> > > argument array, it will allow to specify a specific whitelist for a
specific
> > > target.
> > 
> > Right, but things like this should be in cl descriptions and / or tracking
> bugs.
> > 
> > It sounds like the problem is that build/common.gypi adds `'-w',
> > '<(DEPTH)/build/ios/grit_whitelist.txt',` unconditionally for all targets.
> Maybe
> > it should stop doing that? Things in build/common.gypi are for global
defaults
> > that are correct for every target; sounds like that's no longer true for the
> > whitelist?
> 
> Added the bug.

That's a private bug. Will the extension you mention eventually be in the public
repo? We try to not add stuff for internal-only projects to public code (from
any company).

> If we consider '<(DEPTH)/build/ios/grit_whitelist.txt'  as a global default
> value and not an unconditional value, it still make sense to define it in
> common.gypi as it must be included in all bundles (application and tests)
except
> if it is specifically overridden.

Ok, that makes sense. lgtm (independent of the question above as this is a
fairly small change)

> That was also the case of grit_defines which was overridden in
> ios_chrome_resources.gyp (but adding a whitelist instead of replacing)

[Olivier, 2015-02-09 16:57:50]

On 2015/02/09 16:43:59, Nico wrote:
> On 2015/02/09 16:07:04, Olivier Robin wrote:
> > On 2015/02/09 15:44:00, Nico wrote:
> > > On 2015/02/09 15:20:47, Olivier Robin wrote:
> > > > On 2015/02/09 15:13:50, Nico wrote:
> > > > > Neither cl description nor bug link (not present) explain why one
would
> > want
> > > > to
> > > > > override this (there's even a "TODO: remove this whitelist" in the
code)
> > > > 
> > > > The goal of the CL is to allow creating bundles with their own
whitelists.
> > > > 
> > > > At the moment, if you want to create a small executable (i.e. an ios
> > > extension)
> > > > that only uses 3 strings, you are required to include all the strings
> > > > whitelisted by the main chrome bundle.
> > > > By allowing to override the whitelist argument (instead of only
appending
> > the
> > > > argument array, it will allow to specify a specific whitelist for a
> specific
> > > > target.
> > > 
> > > Right, but things like this should be in cl descriptions and / or tracking
> > bugs.
> > > 
> > > It sounds like the problem is that build/common.gypi adds `'-w',
> > > '<(DEPTH)/build/ios/grit_whitelist.txt',` unconditionally for all targets.
> > Maybe
> > > it should stop doing that? Things in build/common.gypi are for global
> defaults
> > > that are correct for every target; sounds like that's no longer true for
the
> > > whitelist?
> > 
> > Added the bug.
> 
> That's a private bug. Will the extension you mention eventually be in the
public
> repo? We try to not add stuff for internal-only projects to public code (from
> any company).
> 
> > If we consider '<(DEPTH)/build/ios/grit_whitelist.txt'  as a global default
> > value and not an unconditional value, it still make sense to define it in
> > common.gypi as it must be included in all bundles (application and tests)
> except
> > if it is specifically overridden.
> 
> Ok, that makes sense. lgtm (independent of the question above as this is a
> fairly small change)
> 
> > That was also the case of grit_defines which was overridden in
> > ios_chrome_resources.gyp (but adding a whitelist instead of replacing)

Well, that's part of the problem.
The extension will not be in the public repo (at least not in short term), but
this change was obviously upstream.
That is why the details were rather small in this CL.

Thanks for the review.

[Olivier, 2015-02-09 16:58:23]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2015-02-09 17:00:47]

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

[commit-bot: I haz the power, 2015-02-09 17:05:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-09 17:05:34]

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