Decompose //extensions/browser/BUILD.gn

Decompose //extensions/browser/BUILD.gn

This CL changes the extensions build to have BUILD.gn's in (almost) all
leaf directories. This accomplishes several things,

a.) Makes it very clear of what files belong to which component/API. For
example, to see which files exist in the audio API, one needs to parse
the massive single //extensions/browsers/BUILD.gn to find the main API
source list, then look if any of the exception conditions (is_chromeos,
is_linux, etc) also contain any of the audio API files. This is now all
in one //extensions/browser/api/audio/BUILD.gn, making it easier to parse
and understand the build of each component.

b.) It makes dependencies clearer. Currently we have no idea which
dependency in the huge dependency list in //extensions/browser/BUILD.gn is
needed by which API. With individual build files, we can now specify
dependencies for just the component that needs them. This CL tries to
do this but some dependencies are used by enough components that we've just
included them for all (//content/browser/public, //skia). This is not ideal
but at worst, it is still much better than the current build.

Since we're now moving APIs over from //chrome/browser/extensions over to
//extensions/browser, this becomes even more important. All APIs that get
moved over now can use this new format for their build files.

jamescook@, rockot@, xiyuan@ - FYI

R=rdevlin.cronin@chromium.org
BUG=641155
Committed: https://crrev.com/a932263a75195ad6cfe3e47dce49cc8845ded05e
Cr-Commit-Position: refs/heads/master@{#418729}

[Rahul Chaturvedi, 2016-09-12 23:25:27]

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

[commit-bot: I haz the power, 2016-09-12 23:25:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rahul Chaturvedi, 2016-09-12 23:28:49]

Most of this was automatically generated so the dependencies will not be
optimal. The purpose is to,
(a) ensure that the BUILD.gn is decomposed,
(b) the build is correct and doesn't break anything
(c) if possible, make the code better by including the minimal dependencies we
can in an automatic process.

[commit-bot: I haz the power, 2016-09-13 00:22:49]

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

[commit-bot: I haz the power, 2016-09-13 00:22:50]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2016-09-13 00:27:53]

On 2016/09/12 at 23:28:49, steel wrote:
> Most of this was automatically generated so the dependencies will not be
optimal. The purpose is to,
> (a) ensure that the BUILD.gn is decomposed,
> (b) the build is correct and doesn't break anything
> (c) if possible, make the code better by including the minimal dependencies we
can in an automatic process.

I'm happy to see this.

I do wonder what it would take to be a little more precise with the
dependencies.

Apparently it builds, so there may not be any major practical issues at the
moment, but one example where I've seen real trouble is that //skia exposes a
public_config for its include path. If your target exposes a header which itself
includes skia headers, then your dependents also must inherit this config. If
you specify the //skia dependency in deps rather than public_deps where it
belongs, your dependents can experience great sadness (i.e. compile errors.)

In general people seem to be lazy about getting the deps/public_deps split
right, and really about getting dependencies right in general. I've seen it
cause enough confusing headaches for other developers that I try to encourage
everyone to get it right whenever I have the opportunity to do so. So consider
this an instance of me doing that. :)

[Rahul Chaturvedi, 2016-09-13 01:58:46]

I was surprised how often //skia was needed in this code. I tried weeding
through a few manually but the number seemed large enough that it didn't seem
like enough ROI to do it manually.

Just trying to compile doesn't reliably fail if we remove a dependency we
actually need. The only way to know for sure is to completely wipe the out/
directory. This becomes super time consuming.

The worst case is that we are as bad as the previous build in this one
particular case (since currently //skia is a dependency for every single API).
Given the effort required to really do this correctly, it seemed just easier to
add these two dependencies everywhere.

[Rahul Chaturvedi, 2016-09-13 02:04:38]

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

[commit-bot: I haz the power, 2016-09-13 02:05:00]

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-09-13 03:06:17]

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

[commit-bot: I haz the power, 2016-09-13 03:06:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2016-09-13 17:10:49]

I took a brief look at some of these.  One thing that I'd like to accomplish is
to only have to add a new entry once, if it's used in e.g. extensions/browser. 
There's no reason that I should need to include a new API in extensions/browser,
chrome/browser/extensions/, extensions/browser/api, etc - I should add it to
extensions/browser/api, which extensions/browser depends on, which
chrome/browser/extensions depends on.  I'd love this to be a cleaner, simpler
world than we had with gyp - but if we have to add new entries in three
different places, I'm worried it won't be.

I'd also like to add deps of content and skia a little more judiciously.  I
understand that it makes no difference in the linking of the monolithic targets,
but the goal here is to clean up deps. :)  Additionally, though the end result
is the same, I'm sure it takes some toll on GN's dependency parsing.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
We already depend on :browser, why do we need to add all of these dependencies
separately?

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/activity_log/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/activity_log/BUILD.gn:11: "//content/public/browser",
?

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/activity_log/BUILD.gn:12: "//skia",
ditto

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/alarms/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/alarms/BUILD.gn:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
not 2015

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/bluetooth_socket/bluetooth_socket_api.cc (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth_socket/bluetooth_socket_api.cc:609:
socket->Send(io_buffer_, static_cast<int>(io_buffer_size_),
?  This seems orthogonal to this CL.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/dns/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/dns/BUILD.gn:4: source_set("dns") {
newlines after license

[Rahul Chaturvedi, 2016-09-13 20:33:50]

So this took some manual work, but I removed all the implicit dependencies my
script added and instead wrote another script to parse the error output from the
build to add deps += ["//skia"] only to those BUILD.gn files that wouldn't
compile without it.

In the latest patch, you should now be seeing only the exact dependencies that
the source sets need to compile.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
On 2016/09/13 17:10:49, Devlin wrote:
> We already depend on :browser, why do we need to add all of these dependencies
> separately?

GN requires us to explicitly depend on what we're using. If we don't, the gn gen
outdir --check command run in the analyze step of the bots fails.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/activity_log/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/activity_log/BUILD.gn:11: "//content/public/browser",
On 2016/09/13 17:10:49, Devlin wrote:
> ?

Done.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/activity_log/BUILD.gn:12: "//skia",
On 2016/09/13 17:10:49, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/alarms/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/alarms/BUILD.gn:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/09/13 17:10:49, Devlin wrote:
> not 2015

Done.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/bluetooth_socket/bluetooth_socket_api.cc (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth_socket/bluetooth_socket_api.cc:609:
socket->Send(io_buffer_, static_cast<int>(io_buffer_size_),
On 2016/09/13 17:10:49, Devlin wrote:
> ?  This seems orthogonal to this CL.

So these are actually real build errors - which for some reason were not showing
up earlier. Doing this build change seems to have, fixed?, the compiler on Win64
and these start breaking.

There are several of these changes over the code, fixing implicit conversions to
int from size_t.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
File extensions/browser/api/dns/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/api/...
extensions/browser/api/dns/BUILD.gn:4: source_set("dns") {
On 2016/09/13 17:10:49, Devlin wrote:
> newlines after license

Done.

[Rahul Chaturvedi, 2016-09-13 20:49:58]

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

[commit-bot: I haz the power, 2016-09-13 20:50: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-09-13 21:39:59]

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

[commit-bot: I haz the power, 2016-09-13 21:40:00]

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

[Rahul Chaturvedi, 2016-09-13 22:31:22]

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

[Rahul Chaturvedi, 2016-09-13 22:31:23]

Adding brettw@ for
extensions/browser/guest_view/extension_view/whitelist/BUILD.gn OWNERs review.

[Rahul Chaturvedi, 2016-09-13 22:34:16]

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

[commit-bot: I haz the power, 2016-09-13 22:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rahul Chaturvedi, 2016-09-13 22:48:52]

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/extension_view/whitelist/BUILD.gn (left):

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/whitelist/BUILD.gn:9:
"catalog.mojom",
I have no idea where this came from. Rietveld fail.

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/extension_view/whitelist/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/whitelist/BUILD.gn:5:
source_set("whitelist") {
This is a newly added file. Not sure why the code review tool is showing
differently.

[Rahul Chaturvedi, 2016-09-13 22:55:01]

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

[commit-bot: I haz the power, 2016-09-13 22:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-13 23:02:30]

+cc dpranke to keep me honest here.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
On 2016/09/13 20:33:49, Rahul Chaturvedi wrote:
> On 2016/09/13 17:10:49, Devlin wrote:
> > We already depend on :browser, why do we need to add all of these
dependencies
> > separately?
> 
> GN requires us to explicitly depend on what we're using. If we don't, the gn
gen
> outdir --check command run in the analyze step of the bots fails.

I've talked with dpranke about this, and he said we should be able to include
transitive dependencies with gn.  I think that would make this a lot more
readable, and is one of the biggest wins in moving to GN, IMO.

[brettw, 2016-09-13 23:13:09]

The whitelist BUILD file LGTM.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
If you convert //extensions/browser/api:api to list all of the APIs in its
public_deps instead of deps, then just depending on //extensions/browser/api
from here will hame "gn check" happy. That's what I would do.

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/extension_view/whitelist/BUILD.gn (left):

https://codereview.chromium.org/2336843002/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/whitelist/BUILD.gn:9:
"catalog.mojom",
On 2016/09/13 22:48:52, Rahul Chaturvedi wrote:
> I have no idea where this came from. Rietveld fail.

This is git trying to track moved+modified files.

[Rahul Chaturvedi, 2016-09-13 23:21:44]

Discussed it offline with Devlin.

There is some value in having explicit dependencies - but having to add an API
in 3 different places is pretty painful.

This does have the side effect of unit tests and browser tests depending on APIs
that they don't really need to depend on. It does help with
//chrome/browser/extensions though, since we don't have to specify every single
individual API that the target depends on.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
On 2016/09/13 23:02:30, Devlin wrote:
> On 2016/09/13 20:33:49, Rahul Chaturvedi wrote:
> > On 2016/09/13 17:10:49, Devlin wrote:
> > > We already depend on :browser, why do we need to add all of these
> dependencies
> > > separately?
> > 
> > GN requires us to explicitly depend on what we're using. If we don't, the gn
> gen
> > outdir --check command run in the analyze step of the bots fails.
> 
> I've talked with dpranke about this, and he said we should be able to include
> transitive dependencies with gn.  I think that would make this a lot more
> readable, and is one of the biggest wins in moving to GN, IMO.

Done.

https://codereview.chromium.org/2336843002/diff/20001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:328: "//extensions/browser/api",
On 2016/09/13 23:13:09, brettw (ping on IM after 24h) wrote:
> If you convert //extensions/browser/api:api to list all of the APIs in its
> public_deps instead of deps, then just depending on //extensions/browser/api
> from here will hame "gn check" happy. That's what I would do.

Done.

[Rahul Chaturvedi, 2016-09-13 23:48:25]

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

[commit-bot: I haz the power, 2016-09-13 23:48:52]

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-09-13 23:53:15]

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

[commit-bot: I haz the power, 2016-09-13 23:53:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Rahul Chaturvedi, 2016-09-14 00:03:10]

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

[commit-bot: I haz the power, 2016-09-14 00:04:07]

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-09-14 00:50:32]

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

[commit-bot: I haz the power, 2016-09-14 00:50:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Rahul Chaturvedi, 2016-09-14 18:15:32]

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

[commit-bot: I haz the power, 2016-09-14 18:16:20]

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-09-14 19:01:20]

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

[commit-bot: I haz the power, 2016-09-14 19:01:21]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Rahul Chaturvedi, 2016-09-14 20:49:38]

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

[commit-bot: I haz the power, 2016-09-14 20:50:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-14 21:06:52]

Looking better!  Glad to see us get rid of the modifications to chrome/ build
files. :)

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:253: "//extensions/browser/guest_view/app_view",
What's the motivation for not having guest_view/ encompass the rest of these?

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:441: "//extensions/browser/api",
browser depends on this, right?  So do we still need this?

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
File extensions/browser/updater/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
extensions/browser/updater/BUILD.gn:35: "//skia",
Does this need skia?

https://codereview.chromium.org/2336843002/diff/160001/extensions/shell/BUILD.gn
File extensions/shell/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/shell/BUILD...
extensions/shell/BUILD.gn:327: "//extensions/browser/app_window",
doesn't extensions/browser include app_window?

[Rahul Chaturvedi, 2016-09-14 21:40:17]

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:253: "//extensions/browser/guest_view/app_view",
On 2016/09/14 21:06:52, Devlin wrote:
> What's the motivation for not having guest_view/ encompass the rest of these?

Mostly because this is auto-generated code. I moved this manually.

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:441: "//extensions/browser/api",
On 2016/09/14 21:06:52, Devlin wrote:
> browser depends on this, right?  So do we still need this?

No, browser depends on //extension/browser now. We need to publicly depend on
//e/b/api so //c/b/e can depend on the API code.

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
File extensions/browser/updater/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
extensions/browser/updater/BUILD.gn:35: "//skia",
On 2016/09/14 21:06:52, Devlin wrote:
> Does this need skia?

Yep.
//extensions/browser/updater/safe_manifest_parser.cc includes,
//extensions/common/extension_utility_messages.h which includes,
//third_party/skia/include/core/SkBitmap.h

In fact, we don't really need //skia in the main //e/b/BUILD.gn, so removed it
from there.

https://codereview.chromium.org/2336843002/diff/160001/extensions/shell/BUILD.gn
File extensions/shell/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/shell/BUILD...
extensions/shell/BUILD.gn:327: "//extensions/browser/app_window",
On 2016/09/14 21:06:52, Devlin wrote:
> doesn't extensions/browser include app_window?

Ah, didn't remove this after I made the deps public.
Done.

[Rahul Chaturvedi, 2016-09-14 21:40:32]

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

[commit-bot: I haz the power, 2016-09-14 21:41:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2016-09-14 21:42:22]

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
File extensions/browser/updater/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
extensions/browser/updater/BUILD.gn:35: "//skia",
If extensions/common uses skia in its public headers, then it should provide
skia as a public_deps. This way we can remove it from here.

[Devlin, 2016-09-14 21:45:06]

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:441: "//extensions/browser/api",
On 2016/09/14 21:40:17, Rahul Chaturvedi wrote:
> On 2016/09/14 21:06:52, Devlin wrote:
> > browser depends on this, right?  So do we still need this?
> 
> No, browser depends on //extension/browser now. We need to publicly depend on
> //e/b/api so //c/b/e can depend on the API code.

This target depends on browser (line 425), which depends on
extensions/browser/api (line 250).  What am I missing?

[Rahul Chaturvedi, 2016-09-14 22:23:41]

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/BUI...
extensions/browser/BUILD.gn:441: "//extensions/browser/api",
On 2016/09/14 21:45:05, Devlin wrote:
> On 2016/09/14 21:40:17, Rahul Chaturvedi wrote:
> > On 2016/09/14 21:06:52, Devlin wrote:
> > > browser depends on this, right?  So do we still need this?
> > 
> > No, browser depends on //extension/browser now. We need to publicly depend
on
> > //e/b/api so //c/b/e can depend on the API code.
> 
> This target depends on browser (line 425), which depends on
> extensions/browser/api (line 250).  What am I missing?

Ugh right, completely missed the :browser, I was searching for
"//extension/browser".
Fixed.

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
File extensions/browser/updater/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/160001/extensions/browser/upd...
extensions/browser/updater/BUILD.gn:35: "//skia",
On 2016/09/14 21:42:21, brettw (ping on IM after 24h) wrote:
> If extensions/common uses skia in its public headers, then it should provide
> skia as a public_deps. This way we can remove it from here.

I was hoping to keep this change contained to //extensions/browser. Might as
well fix this now.

Also removed all extra //skia dependencies.

Done.

[Rahul Chaturvedi, 2016-09-14 22:34:21]

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

[commit-bot: I haz the power, 2016-09-14 22:35:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-14 22:50:30]

Nice!  This looks great.  Just a few more places where I think we can polish
this up.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/BUI...
extensions/browser/BUILD.gn:323: "//extensions/browser/api",
ditto here re deps

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/diagnostics/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/diagnostics/BUILD.gn:9: "diagnostics_api_chromeos.cc",
can we add an assert platform is chromeos here?

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/document_scan/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/document_scan/BUILD.gn:11:
"document_scan_interface_chromeos.cc",
this seems broken - why do we include chromeos when line 14 implies that
nonchromeos is possible here?

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/messaging/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/messaging/BUILD.gn:7: "native_message_host.cc",
what about the .h? (I think this was probably a bug in the gypi)

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/networking_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/networking_private/BUILD.gn:11:
"networking_private_chromeos.cc",
here too - chromeos, but line 28 implies not chromeos is possible

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/system_cpu/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/system_cpu/BUILD.gn:10: "cpu_info_provider_mac.cc",
?

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/webcam_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/webcam_private/BUILD.gn:14:
"webcam_private_api_chromeos.cc",
assert chromeos (it'd actually be great to do this in all api BUILD files that
are supposed to be cros-only)

[Rahul Chaturvedi, 2016-09-14 23:10:25]

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/BUI...
extensions/browser/BUILD.gn:323: "//extensions/browser/api",
On 2016/09/14 22:50:29, Devlin wrote:
> ditto here re deps

Done.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/diagnostics/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/diagnostics/BUILD.gn:9: "diagnostics_api_chromeos.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> can we add an assert platform is chromeos here?

Done.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/document_scan/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/document_scan/BUILD.gn:11:
"document_scan_interface_chromeos.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> this seems broken - why do we include chromeos when line 14 implies that
> nonchromeos is possible here?

_chromeos files will only be compiled on Chrome OS. This is also how the
original build was setup, my parser just moved it. This could 'maybe' be done
better, but is definitely not in the scope of this CL.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/messaging/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/messaging/BUILD.gn:7: "native_message_host.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> what about the .h? (I think this was probably a bug in the gypi)

Yep, was broken in the original BUILD.gn. Fixed and added the other missing
header.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/networking_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/networking_private/BUILD.gn:11:
"networking_private_chromeos.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> here too - chromeos, but line 28 implies not chromeos is possible

Same reasoning.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/system_cpu/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/system_cpu/BUILD.gn:10: "cpu_info_provider_mac.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> ?

_platform files are compiled only for that platform.

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/webcam_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/webcam_private/BUILD.gn:14:
"webcam_private_api_chromeos.cc",
On 2016/09/14 22:50:29, Devlin wrote:
> assert chromeos (it'd actually be great to do this in all api BUILD files that
> are supposed to be cros-only)

Done.

[Rahul Chaturvedi, 2016-09-14 23:25:14]

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

[commit-bot: I haz the power, 2016-09-14 23:25:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-15 00:08:04]

lgtm, thanks for bearing with me through the iterations on this!

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
File extensions/browser/api/document_scan/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/200001/extensions/browser/api...
extensions/browser/api/document_scan/BUILD.gn:11:
"document_scan_interface_chromeos.cc",
On 2016/09/14 23:10:24, Rahul Chaturvedi wrote:
> On 2016/09/14 22:50:29, Devlin wrote:
> > this seems broken - why do we include chromeos when line 14 implies that
> > nonchromeos is possible here?
> 
> _chromeos files will only be compiled on Chrome OS. This is also how the
> original build was setup, my parser just moved it. This could 'maybe' be done
> better, but is definitely not in the scope of this CL.

Ah, didn't realize we had that rule.  Interesting!

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
File extensions/browser/api/webcam_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
extensions/browser/api/webcam_private/BUILD.gn:5: assert(is_chromeos)
nit: do we have to import anything to use is_chromeos?

[Rahul Chaturvedi, 2016-09-15 00:14:57]

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
File extensions/browser/api/webcam_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
extensions/browser/api/webcam_private/BUILD.gn:5: assert(is_chromeos)
On 2016/09/15 00:08:04, Devlin wrote:
> nit: do we have to import anything to use is_chromeos?

Nope. It is available without any imports, in fact, all is_platform variables
seem to be used without imports everywhere.

[Rahul Chaturvedi, 2016-09-15 00:14:58]

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
File extensions/browser/api/webcam_private/BUILD.gn (right):

https://codereview.chromium.org/2336843002/diff/220001/extensions/browser/api...
extensions/browser/api/webcam_private/BUILD.gn:5: assert(is_chromeos)
On 2016/09/15 00:08:04, Devlin wrote:
> nit: do we have to import anything to use is_chromeos?

Nope. It is available without any imports, in fact, all is_platform variables
seem to be used without imports everywhere.

[Rahul Chaturvedi, 2016-09-15 00:15:20]

The CQ bit was unchecked by steel@chromium.org

[Rahul Chaturvedi, 2016-09-15 00:15:28]

The CQ bit was checked by steel@chromium.org

[Rahul Chaturvedi, 2016-09-15 00:15:29]

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

[commit-bot: I haz the power, 2016-09-15 00:16:15]

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-09-15 00:44:32]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-15 00:46:54]

Description was changed from

==========
Decompose //extensions/browser/BUILD.gn

This CL changes the extensions build to have BUILD.gn's in (almost) all
leaf directories. This accomplishes several things,

a.) Makes it very clear of what files belong to which component/API. For
example, to see which files exist in the audio API, one needs to parse
the massive single //extensions/browsers/BUILD.gn to find the main API
source list, then look if any of the exception conditions (is_chromeos,
is_linux, etc) also contain any of the audio API files. This is now all
in one //extensions/browser/api/audio/BUILD.gn, making it easier to parse
and understand the build of each component.

b.) It makes dependencies clearer. Currently we have no idea which
dependency in the huge dependency list in //extensions/browser/BUILD.gn is
needed by which API. With individual build files, we can now specify
dependencies for just the component that needs them. This CL tries to
do this but some dependencies are used by enough components that we've just
included them for all (//content/browser/public, //skia). This is not ideal
but at worst, it is still much better than the current build.

Since we're now moving APIs over from //chrome/browser/extensions over to
//extensions/browser, this becomes even more important. All APIs that get
moved over now can use this new format for their build files.


jamescook@, rockot@, xiyuan@ - FYI

R=rdevlin.cronin@chromium.org
BUG=641155
==========

to

==========
Decompose //extensions/browser/BUILD.gn

This CL changes the extensions build to have BUILD.gn's in (almost) all
leaf directories. This accomplishes several things,

a.) Makes it very clear of what files belong to which component/API. For
example, to see which files exist in the audio API, one needs to parse
the massive single //extensions/browsers/BUILD.gn to find the main API
source list, then look if any of the exception conditions (is_chromeos,
is_linux, etc) also contain any of the audio API files. This is now all
in one //extensions/browser/api/audio/BUILD.gn, making it easier to parse
and understand the build of each component.

b.) It makes dependencies clearer. Currently we have no idea which
dependency in the huge dependency list in //extensions/browser/BUILD.gn is
needed by which API. With individual build files, we can now specify
dependencies for just the component that needs them. This CL tries to
do this but some dependencies are used by enough components that we've just
included them for all (//content/browser/public, //skia). This is not ideal
but at worst, it is still much better than the current build.

Since we're now moving APIs over from //chrome/browser/extensions over to
//extensions/browser, this becomes even more important. All APIs that get
moved over now can use this new format for their build files.

jamescook@, rockot@, xiyuan@ - FYI

R=rdevlin.cronin@chromium.org
BUG=641155

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

[commit-bot: I haz the power, 2016-09-15 00:46:56]

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