Really include *.content_browsertests.filter in data dependencies of Android.

Really include *.content_browsertests.filter in data dependencies of Android.

r426336 intended to add a data dependency for
site-per-process.content_browsertests.filter on Android builds.  The CL
missed the fact that the modified line (where "|| is_android" was added)
is nested in a long if that says "if (!is_android)".  Doh...
This CL tries to ensure that the dependency is *really* there.

The CL also refactors away nested ifs and ifs with negative conditions.
Hopefully this will help avoid similar confusion in the future.

BUG=654569
Committed: https://crrev.com/ff56030a9b90a903622b1563dd7e56998d58e30a
Cr-Commit-Position: refs/heads/master@{#426817}

[Łukasz Anforowicz, 2016-10-20 17:05:49]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 17:06:35]

Dry run: 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-10-20 18:39:57]

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

[commit-bot: I haz the power, 2016-10-20 18:39:58]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-20 18:45:00]

lukasza@chromium.org changed reviewers:
+ sky@chromium.org

[Łukasz Anforowicz, 2016-10-20 18:45:00]

sky@, could you please take a look?

I haven't been able to test this CL well locally :-/

- I've tried building content_browsertests and they build fine.

- I don't know how to build an "isolate" and verify that the filter file is
present.

- I also don't know how to verify presence of the dependency (AFAICT only
*runtime* dependencies are returned by: gn desc out/gn
//content/test:content_browsertests deps --all | grep title1.html).

OTOH, I think that since this is just for the Site Isolation Android bot, I
think the most efficient way to go forward is to just try landing this CL
(assuming it looks okay in the review;  I see that the trybots/CQ-dry-run are
happy).

[sky, 2016-10-20 20:36:48]

https://codereview.chromium.org/2434983003/diff/1/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/1/content/test/BUILD.gn#newco...
content/test/BUILD.gn:644: if (is_android || is_linux || is_mac || is_win) {
Thanks for trying to clean things up. These conditionals are away from other
conditionals, so it's hard to see we already have conditionals for some of these
cases. Please merge these conditionals with the others below.

[Łukasz Anforowicz, 2016-10-20 21:40:54]

+brettw@ in case he wants to comment on the question below -
https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...

https://codereview.chromium.org/2434983003/diff/1/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/1/content/test/BUILD.gn#newco...
content/test/BUILD.gn:644: if (is_android || is_linux || is_mac || is_win) {
On 2016/10/20 20:36:48, sky wrote:
> Thanks for trying to clean things up. These conditionals are away from other
> conditionals, so it's hard to see we already have conditionals for some of
these
> cases. Please merge these conditionals with the others below.

Done.

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:738: ]
I am not sure why these data dependencies aren't just unconditional.  FWIW this
comes from https://crrev.com/355500 by brettw@.  Maybe we should consider making
this unconditional? (in a separate CL?)  This would also make it more obviously
safe to use "=" here but "+=" below.

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:753: data += [
"//testing/buildbot/filters/site-per-process.content_browsertests.filter" ]
Maybe this should be moved to the generic section (i.e. next to "data/" and next
to current (is_android || is_linux || is_mac || is_win) condition.  Skipping
this filter file is not a huge performance win for the testing infrastructure
and it surely adds a little bit of complexity to the BUILD.gn file.  WDYT?  Do
it in a separate CL?  Not at all?  Just do this in this CL?

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:776: data += [
"//testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
]
Same as above - the filter file can just go into a generic section.

BTW: I haven't been able to say something like (pseudo-code):
data += all_files_matching("//testing/buildbot/filters/*content_browsertests*")

[brettw, 2016-10-20 21:56:11]

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

[brettw, 2016-10-20 21:56:12]

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:738: ]
This kind of thing would have normally just been copied from GYP, so I have no
idea.

[sky, 2016-10-20 22:53:21]

LGTM

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:753: data += [
"//testing/buildbot/filters/site-per-process.content_browsertests.filter" ]
On 2016/10/20 21:40:54, Łukasz Anforowicz wrote:
> Maybe this should be moved to the generic section (i.e. next to "data/" and
next
> to current (is_android || is_linux || is_mac || is_win) condition.  Skipping
> this filter file is not a huge performance win for the testing infrastructure
> and it surely adds a little bit of complexity to the BUILD.gn file.  WDYT?  Do
> it in a separate CL?  Not at all?  Just do this in this CL?

Not sure what you are proposing. I like what you have here.

[Łukasz Anforowicz, 2016-10-20 22:58:33]

Thank you for the review.

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/20001/content/test/BUILD.gn#n...
content/test/BUILD.gn:753: data += [
"//testing/buildbot/filters/site-per-process.content_browsertests.filter" ]
On 2016/10/20 22:53:21, sky wrote:
> On 2016/10/20 21:40:54, Łukasz Anforowicz wrote:
> > Maybe this should be moved to the generic section (i.e. next to "data/" and
> next
> > to current (is_android || is_linux || is_mac || is_win) condition.  Skipping
> > this filter file is not a huge performance win for the testing
infrastructure
> > and it surely adds a little bit of complexity to the BUILD.gn file.  WDYT? 
Do
> > it in a separate CL?  Not at all?  Just do this in this CL?
> 
> Not sure what you are proposing. I like what you have here.

Okay, thanks.  To clarify, I'll put together a separate CL and share with you.

[Łukasz Anforowicz, 2016-10-20 22:58:40]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-20 22:59:25]

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-10-20 23:06:19]

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

[commit-bot: I haz the power, 2016-10-20 23:06:20]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-10-20 23:13:29]

sky@, it turned out that my previous changes didn't work on Mac, because the
OS-related conditionals are *after* attempting to append to |data| in |if
(enable_plugins)| case.  I've tried fixing this in the latest patchset - could
you please take one more look?

https://codereview.chromium.org/2434983003/diff/40001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2434983003/diff/40001/content/test/BUILD.gn#n...
content/test/BUILD.gn:731: data += [ "$root_out_dir/ppapi_tests.plugin/" ]
This is the line that was failing on Mac... :-/

[sky, 2016-10-20 23:36:57]

LGTM

[Łukasz Anforowicz, 2016-10-21 15:42:33]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-21 15:42:50]

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-10-21 16:50:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 17:05:56]

Description was changed from

==========
Really include *.content_browsertests.filter in data dependencies of Android.

r426336 intended to add a data dependency for
site-per-process.content_browsertests.filter on Android builds.  The CL
missed the fact that the modified line (where "|| is_android" was added)
is nested in a long if that says "if (!is_android)".  Doh...
This CL tries to ensure that the dependency is *really* there.

The CL also refactors away nested ifs and ifs with negative conditions.
Hopefully this will help avoid similar confusion in the future.

BUG=654569
==========

to

==========
Really include *.content_browsertests.filter in data dependencies of Android.

r426336 intended to add a data dependency for
site-per-process.content_browsertests.filter on Android builds.  The CL
missed the fact that the modified line (where "|| is_android" was added)
is nested in a long if that says "if (!is_android)".  Doh...
This CL tries to ensure that the dependency is *really* there.

The CL also refactors away nested ifs and ifs with negative conditions.
Hopefully this will help avoid similar confusion in the future.

BUG=654569

Committed: https://crrev.com/ff56030a9b90a903622b1563dd7e56998d58e30a
Cr-Commit-Position: refs/heads/master@{#426817}
==========

[commit-bot: I haz the power, 2016-10-21 17:05:57]

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