Build public cast_shell on x86 with GN.

This change contains several new targets required to successfully build the public cast_shell for x86. This code also contributes to other public build variants, but these have not been tested.

Keeping these changes current to TOT will keep the BUILD.gn files current as the code base changes. There are several known issues noted in the TODOs, some of which are listed below.

* //chromecast:chromecast_locales_pak needs to be built.

* //chromecast/app:cast_shell_unittests is not building.

* logic pieces for internal builds are not tested.

BUG=516899
(internal b/17615224)

Committed: https://crrev.com/0c9838bcf5d06900aded810bfc7548afd90297f5
Cr-Commit-Position: refs/heads/master@{#341928}

[slan, 2015-08-03 23:49:08]

slan@chromium.org changed reviewers:
+ alokp@chromium.org, gunsch@chromium.org, kmackay@chromium.org,
mbjorge@chromium.org

[slan, 2015-08-03 23:52:29]

All,

This change is ready for a look. There are some holes noted in each of the
BUILD.gn files, but I wanted to get these changes submitted so that they
continue to track the code. My plan is to continue to fix these in future CLs.

However, do not be shy about calling out something if you feel it is not ready
for submit.

[slan, 2015-08-04 00:04:58]

Note: I missed the hack in render_fram_impl.cc. 

I think we need to explicitly disable enable_pepper_cdms in features.gni. I am
exploring a solution.

[gunsch, 2015-08-04 00:43:10]

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.gn
File chromecast/app/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.g...
chromecast/app/BUILD.gn:8: source_set("main_delegate") {
Should this simply be the "app" source_set, and include "cast_crash_client"?

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.g...
chromecast/app/BUILD.gn:83: # TODO(slan): These outputs should be required. Why
is this building...?
These are "required" but nothing enforces them at build-time. I'm not sure we'd
notice they're missing until trying to cycle through all the home screen UIs
with different locales (Byungchul has a script that does this).

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
File chromecast/browser/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:35: # TODO(slan): Resolve co-dependency and create
"devtools" target.
IDK if this is entirely necessary (here and below). I don't think it's the worst
thing to have a larger "browser" target that encompasses everything. I doesn't
seem to me that we would gain a lot by taking the effort to decouple components
in subfolders from their parents, particularly ones that still have a fairly
heavy-weight dependency on the conceptual "chromecast/browser".

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:57: "service/cast_service_android.cc",
does GYP do the same magic w/r/t automatically hiding android files from the
linux target?

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args....
chromecast/build/args.gn:8: # in turn builds
//content/browser/render_host/render_process_host_impl.h's
why is this detail here? This flag isn't an arbitrary setting to allow building.
We quite rely on browser-based CDMs.

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args....
chromecast/build/args.gn:28: # See the corresponding TODO(gyp) comment in
//build/config/features.gni.
Unclear to me if this TODO will remain, since Brett has expressed not wanting us
to introduce a global "is_chromecast".

https://codereview.chromium.org/1213333003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1213333003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:177: // TODO(slan): Figure out why line
4989 doesn't vibe.
did you figure it out?

[mbjorge, 2015-08-04 01:02:05]

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
File chromecast/browser/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:57: "service/cast_service_android.cc",
On 2015/08/04 00:43:10, gunsch wrote:
> does GYP do the same magic w/r/t automatically hiding android files from the
> linux target?

Yep (at least, Chromium .gyp files are set up to):
https://chromium.googlesource.com/external/gyp/+/master/docs/UserDocumentatio...

[mbjorge, 2015-08-04 18:10:54]

https://codereview.chromium.org/1213333003/diff/120001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/build/args....
chromecast/build/args.gn:12: # TODO(slan): Add note about this variable.
What needs to be noted about this variable?
Is it just documentation (e.g. "# Set to true to compile with MSE support for
MPEG2 TS", like in common.gypi), or something else?

https://codereview.chromium.org/1213333003/diff/120001/chromecast/common/BUIL...
File chromecast/common/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/common/BUIL...
chromecast/common/BUILD.gn:32: source_set("media") {
Why not put this in chromecast/common/media/BUILD.gn?

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:48: source_set("media") {
Maybe put this in chromecast/renderer/media/BUILD.gn ?

[slan, 2015-08-04 21:22:05]

Still working to get solution for resource targets.

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.gn
File chromecast/app/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.g...
chromecast/app/BUILD.gn:8: source_set("main_delegate") {
On 2015/08/04 00:43:10, gunsch wrote:
> Should this simply be the "app" source_set, and include "cast_crash_client"?

Done.

https://codereview.chromium.org/1213333003/diff/100001/chromecast/app/BUILD.g...
chromecast/app/BUILD.gn:83: # TODO(slan): These outputs should be required. Why
is this building...?
On 2015/08/04 00:43:10, gunsch wrote:
> These are "required" but nothing enforces them at build-time.

Please see the documentation here: 
https://code.google.com/p/chromium/codesearch#chromium/src/build/secondary/to...

I am investigating and will update this comment thread when I resolve this.

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
File chromecast/browser/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:35: # TODO(slan): Resolve co-dependency and create
"devtools" target.
On 2015/08/04 00:43:10, gunsch wrote:
> IDK if this is entirely necessary (here and below). I don't think it's the
worst
> thing to have a larger "browser" target that encompasses everything. I doesn't
> seem to me that we would gain a lot by taking the effort to decouple
components
> in subfolders from their parents, particularly ones that still have a fairly
> heavy-weight dependency on the conceptual "chromecast/browser".

My main concerns here are:
1) The design seems to be pretty heavily coupled. 
2) None of this is tested at a granular level.

Modularizing this would help both of these goals. However, I admit most of this
source is pretty mechanical, and chromecast_browser_test may be enough coverage.
Thoughts?

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args....
chromecast/build/args.gn:8: # in turn builds
//content/browser/render_host/render_process_host_impl.h's
On 2015/08/04 00:43:10, gunsch wrote:
> why is this detail here? This flag isn't an arbitrary setting to allow
building.
> We quite rely on browser-based CDMs.

Simplified.

https://codereview.chromium.org/1213333003/diff/100001/chromecast/build/args....
chromecast/build/args.gn:28: # See the corresponding TODO(gyp) comment in
//build/config/features.gni.
On 2015/08/04 00:43:10, gunsch wrote:
> Unclear to me if this TODO will remain, since Brett has expressed not wanting
us
> to introduce a global "is_chromecast".

This comment is confusing anyway. We can move forward without |is_chromecast|

https://codereview.chromium.org/1213333003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1213333003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:177: // TODO(slan): Figure out why line
4989 doesn't vibe.
On 2015/08/04 00:43:10, gunsch wrote:
> did you figure it out?

Resolved:
https://codereview.chromium.org/1266693005/

https://codereview.chromium.org/1213333003/diff/120001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/build/args....
chromecast/build/args.gn:12: # TODO(slan): Add note about this variable.
On 2015/08/04 18:10:54, mbjorge wrote:
> What needs to be noted about this variable?
> Is it just documentation (e.g. "# Set to true to compile with MSE support for
> MPEG2 TS", like in common.gypi), or something else?

Comment added. 

These are just to provide developers with starting points to find out how these
args impact the code.

https://codereview.chromium.org/1213333003/diff/120001/chromecast/common/BUIL...
File chromecast/common/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/common/BUIL...
chromecast/common/BUILD.gn:32: source_set("media") {
On 2015/08/04 18:10:54, mbjorge wrote:
> Why not put this in chromecast/common/media/BUILD.gn?

See my response to the other comment. It does not seem to stand alone enough in
my mind as a target, but it is really pretty arbitrary. If you feel that we
should move it, or see that the SG says one way or another, let's move it.

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:48: source_set("media") {
On 2015/08/04 18:10:54, mbjorge wrote:
> Maybe put this in chromecast/renderer/media/BUILD.gn ?

This was my initial thought. It is really a style choice. IMO, it is not
sufficiently complex to merit its own target. I think we should keep it here for
now, but I will add a TODO if we find that it makes more sense to move it down.
Perhaps if we add unit testing, for example.

https://codereview.chromium.org/1213333003/diff/120001/ui/ozone/demo/BUILD.gn
File ui/ozone/demo/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/ui/ozone/demo/BUILD.gn...
ui/ozone/demo/BUILD.gn:33: # "//ui/ozone/gpu",
This is fixed here: https://codereview.chromium.org/1262623009/

[mbjorge, 2015-08-04 22:09:14]

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:48: source_set("media") {
On 2015/08/04 21:22:05, slan wrote:
> On 2015/08/04 18:10:54, mbjorge wrote:
> > Maybe put this in chromecast/renderer/media/BUILD.gn ?
> 
> This was my initial thought. It is really a style choice. IMO, it is not
> sufficiently complex to merit its own target. I think we should keep it here
for
> now, but I will add a TODO if we find that it makes more sense to move it
down.
> Perhaps if we add unit testing, for example.

I feel like complexity isn't a good metric to base targets on.
Chromium has lots of targets that are very simple, e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/device/core/BUILD.gn
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/BUILD.gn
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/util/BUILD.gn

The SG only says: "It usually makes sense to have more build files closer to the
code than fewer ones at the toplevel (this is in contrast with what we did with
GYP). This makes things easier to find and owners reviews easier since changes
are more focused." So there doesn't seem to be a firm ruling.

However, I think (as you mentioned in your other comment), that if the target
doesn't stand alone, then it's reasonable to keep it here (or in general, keep
it with its parent).

Still, my vote would be to put it in media/BUILD.gn, since it's arbitrary and I
personally prefer having smaller targets very close to the corresponding source
files. 

But it's really a minor detail, and I think, grand scheme, it's fine to leave it
as is and add a TODO as you said (and perhaps do the same for browser:media and
common:media?)

[slan, 2015-08-04 23:55:48]

The most recent patch cleans up some of the TODO's I have been waffling on. Most
notably:

* chromecast_settings is working as expected, the failure only occurs if it is
configured incorrectly and the output dir is empty. No problems there.

* media targets have been moved closer to the source for common, renderer, and
browser. 

* Decoupling the browser target should not be a priority of ours. (testing it,
however, perhaps should be). Those TODO's are gone.

* chromecast_locales_pak will go in as a separate change. It is currently
blocking this CL unecessarily.

This CL is ready for more review. I think I have cleaned up everything I want to
before submit.

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/120001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:48: source_set("media") {
On 2015/08/04 22:09:14, mbjorge wrote:
> On 2015/08/04 21:22:05, slan wrote:
> > On 2015/08/04 18:10:54, mbjorge wrote:
> > > Maybe put this in chromecast/renderer/media/BUILD.gn ?
> > 
> > This was my initial thought. It is really a style choice. IMO, it is not
> > sufficiently complex to merit its own target. I think we should keep it here
> for
> > now, but I will add a TODO if we find that it makes more sense to move it
> down.
> > Perhaps if we add unit testing, for example.
> 
> I feel like complexity isn't a good metric to base targets on.
> Chromium has lots of targets that are very simple, e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/core/BUILD.gn
> https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/ipc/BUILD.gn
> https://code.google.com/p/chromium/codesearch#chromium/src/mojo/util/BUILD.gn
> 
> The SG only says: "It usually makes sense to have more build files closer to
the
> code than fewer ones at the toplevel (this is in contrast with what we did
with
> GYP). This makes things easier to find and owners reviews easier since changes
> are more focused." So there doesn't seem to be a firm ruling.
> 
> However, I think (as you mentioned in your other comment), that if the target
> doesn't stand alone, then it's reasonable to keep it here (or in general, keep
> it with its parent).
> 
> Still, my vote would be to put it in media/BUILD.gn, since it's arbitrary and
I
> personally prefer having smaller targets very close to the corresponding
source
> files. 
> 
> But it's really a minor detail, and I think, grand scheme, it's fine to leave
it
> as is and add a TODO as you said (and perhaps do the same for browser:media
and
> common:media?)

Thanks for the in-depth comment here. I think that moving the targets into
*/media is a good choice for the reasons you mentioned. After some thought,
there is little sense in putting TODOs in here which might never be addressed
and which clutter the code.

[bcf_google, 2015-08-05 00:15:13]

bcf@google.com changed reviewers:
+ bcf@google.com

[bcf_google, 2015-08-05 00:15:13]

https://codereview.chromium.org/1213333003/diff/160001/chromecast/common/medi...
File chromecast/common/media/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/160001/chromecast/common/medi...
chromecast/common/media/BUILD.gn:14: "cma_messages.h",
Is this supposed to be duplicated?

[slan, 2015-08-05 00:19:25]

https://codereview.chromium.org/1213333003/diff/160001/chromecast/common/medi...
File chromecast/common/media/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/160001/chromecast/common/medi...
chromecast/common/media/BUILD.gn:14: "cma_messages.h",
On 2015/08/05 00:15:13, bcf wrote:
> Is this supposed to be duplicated?

Nice catch!

[mbjorge, 2015-08-05 00:26:27]

lgtm

[slan, 2015-08-05 01:56:35]

FYI, internal instructions for building this are here:

https://sites.google.com/a/google.com/dongle/engineering/development-instruct...

I will link this page to the parent page when this CL lands.

[gunsch, 2015-08-05 01:58:01]

Commit message note: I realize it's a little lateish now, but do you have a
public crbug tracking our GN work? Since we're not picking these back to our M43
fork, these commits don't end up getting automatically picked up by any bug
tracker.

[gunsch, 2015-08-05 02:04:11]

Few more notes but overall this looks nice.

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args....
chromecast/build/args.gn:31: # See //build/config/ui.gni. Notice that use_x11 =
is_linux && !use_ozone.
Are you saying we don't want use_x11 for our build?

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:18: # TODO(slan): Can these be added into
"//chromecast/renderer/media"?
I don't understand, why can't they already? ISTM like a one-way dependency of
chromecast/renderer on chromecast/renderer/media

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:28: "//chromecast/renderer/media",
Note: this and other media deps should only be here if (linux)---both this file,
chromecast/browser, and chromecast/common. This should correspond to the
"cast_shell_media" target in GYP, which will not build on Android.

As a side note, one potential benefit to the Mojo path is that it should (?)
build on Android. Our CMA pipeline actually fails at link time due to duplicate
definitions, since we have to create the same "IPC traits" classes for an
IPC-based media pipeline that Android does, and they're internal to content/ so
we can't include the declarations. :(

[slan, 2015-08-05 14:34:58]

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args....
chromecast/build/args.gn:31: # See //build/config/ui.gni. Notice that use_x11 =
is_linux && !use_ozone.
On 2015/08/05 02:04:10, gunsch wrote:
> Are you saying we don't want use_x11 for our build?

Correct, we are using ozone, with the egl_platform_x11 implementation:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

I will make sure that halliwell@ sees this before submit for visibility.

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
File chromecast/renderer/BUILD.gn (right):

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:18: # TODO(slan): Can these be added into
"//chromecast/renderer/media"?
On 2015/08/05 02:04:11, gunsch wrote:
> I don't understand, why can't they already? ISTM like a one-way dependency of
> chromecast/renderer on chromecast/renderer/media

Huh. You're correct. Moved.

https://codereview.chromium.org/1213333003/diff/180001/chromecast/renderer/BU...
chromecast/renderer/BUILD.gn:28: "//chromecast/renderer/media",
On 2015/08/05 02:04:10, gunsch wrote:
> Note: this and other media deps should only be here if (linux)---both this
file,
> chromecast/browser, and chromecast/common. This should correspond to the
> "cast_shell_media" target in GYP, which will not build on Android.
> 
Noted. I've added a (!is_android) conditional. I will keep an eye on this for
when we start the Android build.

> As a side note, one potential benefit to the Mojo path is that it should (?)
> build on Android. Our CMA pipeline actually fails at link time due to
duplicate
> definitions, since we have to create the same "IPC traits" classes for an
> IPC-based media pipeline that Android does, and they're internal to content/
so
> we can't include the declarations. :(

Wholesale unification of code, especially of IPC, would be awesome.

[slan, 2015-08-05 17:22:28]

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args.gn
File chromecast/build/args.gn (right):

https://codereview.chromium.org/1213333003/diff/180001/chromecast/build/args....
chromecast/build/args.gn:31: # See //build/config/ui.gni. Notice that use_x11 =
is_linux && !use_ozone.
On 2015/08/05 14:34:58, slan wrote:
> On 2015/08/05 02:04:10, gunsch wrote:
> > Are you saying we don't want use_x11 for our build?
> 
> Correct, we are using ozone, with the egl_platform_x11 implementation:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...
> 
> I will make sure that halliwell@ sees this before submit for visibility.

lgtm from halliwell@ here.

[slan, 2015-08-05 18:02:08]

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

[commit-bot: I haz the power, 2015-08-05 18:03:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213333003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1213333003/200001

[commit-bot: I haz the power, 2015-08-05 18:03:58]

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

[commit-bot: I haz the power, 2015-08-05 18:03:59]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gunsch, 2015-08-05 18:05:04]

lgtm

[slan, 2015-08-05 18:05:37]

The CQ bit was checked by slan@chromium.org

[slan, 2015-08-05 18:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mbjorge@chromium.org
Link to the patchset: https://codereview.chromium.org/1213333003/#ps200001
(title: "Address linux-specific deps.")

[commit-bot: I haz the power, 2015-08-05 18:07:07]

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

[commit-bot: I haz the power, 2015-08-05 18:37:50]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-05 18:38:56]

Patchset 11 (id:??) landed as
https://crrev.com/0c9838bcf5d06900aded810bfc7548afd90297f5
Cr-Commit-Position: refs/heads/master@{#341928}