Media Router GYP/GN cleanup

Media Router GYP/GN cleanup:

- Refactor GYP and GN files to share source lists.
- Extract a proper test_support target.
- Fix upstream files with new target structure.

BUG=461815, 464199, 464205
Committed: https://crrev.com/9c5e5781517823c18144b0bb5b66b66a1afd6733
Cr-Commit-Position: refs/heads/master@{#330002}

[mark a. foltz, 2015-05-07 21:17:21]

mfoltz@chromium.org changed reviewers:
+ brettw@chromium.org, kmarshall@chromium.org

[mark a. foltz, 2015-05-07 21:17:21]

brettw@: Can you please review the GN/GYP files outside of browser/media/router
incl .gn?
kmarshall@: Everything

Thanks.

[Kevin M, 2015-05-07 22:10:58]

lgtm

[brettw, 2015-05-08 16:52:57]

Is the file list frequently changing? For this kind of thing I would typically
not share source file lists as it tends to make both builds more difficult to
understand, it slows down GN a lot (about half the GN runtime on Windows is
forking Python to read the existing ones), and we'll have to clean this all up
again when we remove GYP.

For larger and frequently changing file lists, this overhead is worth the
benefit of maintaining two lists. In just glancing at this, I would not bother
to share a smaller target like this, but it would depend on what you expect to
happen in the future to this component.

[mark a. foltz, 2015-05-08 17:48:29]

On 2015/05/08 16:52:57, brettw wrote:
> Is the file list frequently changing? For this kind of thing I would typically
> not share source file lists as it tends to make both builds more difficult to
> understand, it slows down GN a lot (about half the GN runtime on Windows is
> forking Python to read the existing ones), and we'll have to clean this all up
> again when we remove GYP.
> 
> For larger and frequently changing file lists, this overhead is worth the
> benefit of maintaining two lists. In just glancing at this, I would not bother
> to share a smaller target like this, but it would depend on what you expect to
> happen in the future to this component.

These lists are under a lot of churn at the moment as we are finishing up the
Media Router implementation.  And they were diverging already resulting in some
variations between the two builds.

After MR lands they should stabilize and we can re-split the lists without too
much trouble.

[brettw, 2015-05-08 20:38:27]

Is there any reason to separate out the gyp files? It will be almost twice as
fast in GN if there is only one gypi file to load with both file lists in it,
and the separation doesn't seem to be of much benefit.

[mark a. foltz, 2015-05-11 20:07:12]

On 2015/05/08 20:38:27, brettw wrote:
> Is there any reason to separate out the gyp files? It will be almost twice as
> fast in GN if there is only one gypi file to load with both file lists in it,
> and the separation doesn't seem to be of much benefit.

You're right. I merged the two .gypi files, and also brought the other unit
tests from chrome_tests_unit.gypi into media_router.gypi.

PTAL

[brettw, 2015-05-11 21:53:37]

https://codereview.chromium.org/1136623003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1136623003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:22: defines = [ "ENABLE_MEDIA_ROUTER=1" ]
You add this define here, twice (incorrectly) in //chrome/common, in
//chrome/browser, in //chrome/browser/extensions, and in chrome/browser/ui. And
these are controlled by a global build flag.

This isn't a good state to be in, and it's also different than the GYP build.

While I would normally want this flag to be scoped non-globally, the fact that
the define controls some grit flags means you had top make it globa. And since
the build flag is global, I think the define should be as well (in
//build/config:feature_flags config). This will match the GYP build and you can
delete all of these custom defines in various targets.

[mark a. foltz, 2015-05-11 23:08:26]

https://codereview.chromium.org/1136623003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1136623003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:22: defines = [ "ENABLE_MEDIA_ROUTER=1" ]
On 2015/05/11 21:53:37, brettw wrote:
> You add this define here, twice (incorrectly) in //chrome/common, in
> //chrome/browser, in //chrome/browser/extensions, and in chrome/browser/ui.
And
> these are controlled by a global build flag.
> 
> This isn't a good state to be in, and it's also different than the GYP build.
> 
> While I would normally want this flag to be scoped non-globally, the fact that
> the define controls some grit flags means you had top make it globa. And since
> the build flag is global, I think the define should be as well (in
> //build/config:feature_flags config). This will match the GYP build and you
can
> delete all of these custom defines in various targets.

Okay, done.

This current situation was done in response to
https://codereview.chromium.org/1069063002/diff/20001/build/config/BUILD.gn#n....
 Does the comment in build/config/BUILD.gn need to be updated?

[mark a. foltz, 2015-05-12 16:36:45]

brettw@: Friendly ping :)

[brettw, 2015-05-12 16:58:16]

LGTM

[mark a. foltz, 2015-05-12 19:11:25]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-05-12 19:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1136623003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-12 19:12:55]

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

[commit-bot: I haz the power, 2015-05-12 20:48:23]

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

[commit-bot: I haz the power, 2015-05-12 20:48:25]

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

[mark a. foltz, 2015-05-15 00:41:43]

After several brave attempts to get PS#11 to pass the trybots, I gave up,
because moving our unit tests into their own target is not possible at present,
as tests are a rat's nest of dependencies.

I simplified the change considerably by merging our test sources into the
existing unit_tests and browser_tests targets.  This seems more maintainable
than the original patch anyway, so I'm going to queue it for commit.

[mark a. foltz, 2015-05-15 00:42:17]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-05-15 00:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1136623003/#ps260001
(title: "Merge file lists into chrome_tests*.gypi")

[commit-bot: I haz the power, 2015-05-15 00:42:38]

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

[commit-bot: I haz the power, 2015-05-15 01:01:58]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-05-15 01:02:49]

Patchset 14 (id:??) landed as
https://crrev.com/9c5e5781517823c18144b0bb5b66b66a1afd6733
Cr-Commit-Position: refs/heads/master@{#330002}

[jam, 2015-05-15 01:17:57]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1140183002/ by jam@chromium.org.

The reason for reverting is: Broke Linux bots on Chromium waterfall, see

http://build.chromium.org/p/chromium/builders/Linux/builds/62091
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=3231

The error in gclient runhooks is:

gyp: Key 'dependencies' repeated at level 3 with key path 'targets.3' while
reading
/b/build/slave/Linux_x64/build/src/chrome/browser/media/router/media_router.gyp

Not sure why this didn't show up on other bots or trybots..