[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org

Committed: https://crrev.com/90112aa640570a8b497b15b69d02c047f384ad40
Cr-Commit-Position: refs/heads/master@{#391283}

[Robert Sesek, 2016-04-29 17:13:14]

Description was changed from

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org
==========

to

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org
==========

[Robert Sesek, 2016-04-29 17:13:14]

rsesek@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2016-04-29 18:23:19]

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:24: (defined(invoker.info_plist) ||
defined(invoker.info_plist_target)) &&
Can't this be expressed more simply by "defined(invoker.info_plist) !=
defined(invoker.info_plist_target)"?
If neither is defined, or both, then the expression is false, if only one of
them is, then it is true. Or do I miss something?

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:126: #         string, path to the Info.plist file
that will be used for the bundle.
nit: info_plist is now also optional, isn't it?

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:194: #         string, path to the Info.plist file
that will be used for the bundle.
ditto

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:205: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
Shouldn't it be the same mutually exclusive test than before?

[Robert Sesek, 2016-05-02 16:53:54]

Thanks for the review!

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:24: (defined(invoker.info_plist) ||
defined(invoker.info_plist_target)) &&
On 2016/04/29 18:23:18, sdefresne wrote:
> Can't this be expressed more simply by "defined(invoker.info_plist) !=
> defined(invoker.info_plist_target)"?
> If neither is defined, or both, then the expression is false, if only one of
> them is, then it is true. Or do I miss something?

Ah, yes, that's better.

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:126: #         string, path to the Info.plist file
that will be used for the bundle.
On 2016/04/29 18:23:18, sdefresne wrote:
> nit: info_plist is now also optional, isn't it?

Done.

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:194: #         string, path to the Info.plist file
that will be used for the bundle.
On 2016/04/29 18:23:18, sdefresne wrote:
> ditto

Done.

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:205: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
On 2016/04/29 18:23:18, sdefresne wrote:
> Shouldn't it be the same mutually exclusive test than before?

Does it make sense to duplicate that check? The mac_info_plist template, when
instantiated below, will perform that.

[sdefresne, 2016-05-03 07:12:18]

lgtm

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:205: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
On 2016/05/02 at 16:53:54, Robert Sesek wrote:
> On 2016/04/29 18:23:18, sdefresne wrote:
> > Shouldn't it be the same mutually exclusive test than before?
> 
> Does it make sense to duplicate that check? The mac_info_plist template, when
instantiated below, will perform that.

I wanted to say that we should either do the same check or no check, but not a
different check. Seems strange to have this template say "you can define any of
info_plist or info_plist_target, or none", and then fail if neither is set
because it use a more strict template. Either do no check and let the underlying
template fail, or duplicate the test.

https://codereview.chromium.org/1925383003/diff/20001/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1925383003/diff/20001/build/config/mac/rules....
build/config/mac/rules.gni:147: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
Remove and let helper template warn about incorrect usage of
info_plist/info_plist_target.

https://codereview.chromium.org/1925383003/diff/20001/build/config/mac/rules....
build/config/mac/rules.gni:205: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
Remove and let helper template warn about incorrect usage of
info_plist/info_plist_target.

[Robert Sesek, 2016-05-03 13:44:27]

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni
File build/config/mac/rules.gni (right):

https://codereview.chromium.org/1925383003/diff/1/build/config/mac/rules.gni#...
build/config/mac/rules.gni:205: assert(defined(invoker.info_plist) ||
defined(invoker.info_plist_target),
On 2016/05/03 07:12:17, sdefresne wrote:
> On 2016/05/02 at 16:53:54, Robert Sesek wrote:
> > On 2016/04/29 18:23:18, sdefresne wrote:
> > > Shouldn't it be the same mutually exclusive test than before?
> > 
> > Does it make sense to duplicate that check? The mac_info_plist template,
when
> instantiated below, will perform that.
> 
> I wanted to say that we should either do the same check or no check, but not a
> different check. Seems strange to have this template say "you can define any
of
> info_plist or info_plist_target, or none", and then fail if neither is set
> because it use a more strict template. Either do no check and let the
underlying
> template fail, or duplicate the test.

Agreed on your reasoning. Removed the checks in favor of just having the
template do them. Duplication seems bad, since it means changing the check in
three places if it does need to change.

[Robert Sesek, 2016-05-03 13:44:54]

rsesek@chromium.org changed reviewers:
+ avi@chromium.org

[Robert Sesek, 2016-05-03 13:44:55]

thakis: //build/mac OWNERS
avi: //content OWNERS

[Nico, 2016-05-03 13:47:52]

lgtm

[Avi (use Gerrit), 2016-05-03 16:32:24]

lgtm

stampity stamp

[Robert Sesek, 2016-05-03 16:33:01]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2016-05-03 16:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1925383003/#ps40001
(title: "Remove extra checks")

[commit-bot: I haz the power, 2016-05-03 16:33:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925383003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925383003/40001

[commit-bot: I haz the power, 2016-05-03 17:33:45]

Description was changed from

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org
==========

to

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org
==========

[commit-bot: I haz the power, 2016-05-03 17:33:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 17:35:35]

Description was changed from

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org
==========

to

==========
[Mac/GN] Port tweak_info_plist.py and run it on //content targets.

BUG=431177
R=thakis@chromium.org,sdefresne@chromium.org

Committed: https://crrev.com/90112aa640570a8b497b15b69d02c047f384ad40
Cr-Commit-Position: refs/heads/master@{#391283}
==========

[commit-bot: I haz the power, 2016-05-03 17:35:36]

Patchset 3 (id:??) landed as
https://crrev.com/90112aa640570a8b497b15b69d02c047f384ad40
Cr-Commit-Position: refs/heads/master@{#391283}