Update deps in extensions/browser/BUILD.gn to pass gn check.

Update deps in extensions/browser/BUILD.gn to pass gn check.

I think this change improves things... if we care about `gn check`.

Creates a couple new source_sets solely for the purpose of avoiding
circular dependencies between //extensions/browser and
//extensions/browser/api.

Alternatively, we could move browser_context_keyed_service_factories.*
down into api/, but it's not clear they belong there.

BUG=none

Review-Url: https://codereview.chromium.org/2684393002
Cr-Commit-Position: refs/heads/master@{#450168}
Committed: https://chromium.googlesource.com/chromium/src/+/09c1c7e0c3dff51be0c613a8163ab31f19f4a7a6

[michaelpg, 2017-02-10 03:38:57]

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

[michaelpg, 2017-02-10 03:38:58]

brettw: I broke the build by forgetting to include a dep in an API, but when I
tried `gn check` I got a bunch of unrelated errors.

This fixes `gn check` for extensions/browser:* and extensions/browser/api:*. I
had to make "browser" a group to remove the circular dependency between
"browser" and "browser/api" (without breaking everything else depending on
"browser"). Is that OK?

[brettw, 2017-02-11 00:29:13]

Fixing "gn check" is good and I'm OK with this type of thing, but I feel like
you should get an extensions person to review the change since the main issue
seems to be where this file belongs. I'm not familiar enough with this to have a
good opinion.

[michaelpg, 2017-02-11 00:37:36]

On 2017/02/11 00:29:13, brettw (plz ping after 24h) wrote:
> Fixing "gn check" is good and I'm OK with this type of thing, but I feel like
> you should get an extensions person to review the change since the main issue
> seems to be where this file belongs. I'm not familiar enough with this to have
a
> good opinion.

I definitely will -- just wanted to check first that this is "correct" (apart
from subjective points like where those files go). Thanks!

[michaelpg, 2017-02-11 02:09:22]

Description was changed from

==========
Update deps in extensions/browser/BUILD.gn to pass gn check.

I think this change improves things... if we care about `gn check`.

Moves browser_context_keyed_service_factories.* down into api/. This might not
be a good thing, but the alternative to dealing with the circular dependency
from extensions/browser/api => extensions/browser => extensions/browser/api was
to create a new source set for those two files and group them into
extensions/browser which seemed like overkill.

BUG=none
==========

to

==========
Update deps in extensions/browser/BUILD.gn to pass gn check.

I think this change improves things... if we care about `gn check`.

Creates a couple new source_sets solely for the purpose of avoiding
circular dependencies between //extensions/browser and
//extensions/browser/api.

Alternatively, we could move browser_context_keyed_service_factories.*
down into api/, but it's not clear they belong there.

BUG=none
==========

[michaelpg, 2017-02-11 02:09:22]

michaelpg@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[michaelpg, 2017-02-11 02:27:25]

PTAL. This attempt creates separate source_sets for internal targets and
restricts their visibility.

[Devlin, 2017-02-13 16:07:21]

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

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:15: if (enable_extensions) {
what includes extensions/* when enable_extensions is false?  We have a good
number of asserts to try and make sure this doesn't happen, e.g.
https://chromium.googlesource.com/chromium/src/+/28ff32abc83ae8939f56702151b3....
 Where does this fail?

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:25: # Isolate the browser's dependency on APIs to
the class that instantiates them.
Nit: This isn't a class.  Also, not all of these are API-related.  Maybe just
// Isolate the instantiation of BrowserContextKeyedServices.
?

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
File extensions/browser/api/BUILD.gn (right):

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
extensions/browser/api/BUILD.gn:82: "//device/base",
multiple entries for //device/base, //device/hid, //device/usb (here and in the
block below)

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
extensions/browser/api/BUILD.gn:113: deps += [ "//chromeos" ]
Do we need to depend on chromeos in non-chromeos builds?

[michaelpg, 2017-02-13 21:08:43]

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

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:15: if (enable_extensions) {
On 2017/02/13 16:07:21, Devlin wrote:
> what includes extensions/* when enable_extensions is false?  We have a good
> number of asserts to try and make sure this doesn't happen, e.g.
>
https://chromium.googlesource.com/chromium/src/+/28ff32abc83ae8939f56702151b3....
>  Where does this fail?

Dunno, maybe nowhere. But I don't want to remove this existing check (moved from
below) in combination with this re-grouping. 

In fact there's already a TODO for this (also moved from below). It may already
be toDONE, but there are other configs I can't check right now (Win/Mac).

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:25: # Isolate the browser's dependency on APIs to
the class that instantiates them.
On 2017/02/13 16:07:21, Devlin wrote:
> Nit: This isn't a class.  Also, not all of these are API-related.  Maybe just
> // Isolate the instantiation of BrowserContextKeyedServices.
> ?

Done.

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
File extensions/browser/api/BUILD.gn (right):

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
extensions/browser/api/BUILD.gn:82: "//device/base",
On 2017/02/13 16:07:21, Devlin wrote:
> multiple entries for //device/base, //device/hid, //device/usb (here and in
the
> block below)

oops, removed

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
extensions/browser/api/BUILD.gn:113: deps += [ "//chromeos" ]
On 2017/02/13 16:07:21, Devlin wrote:
> Do we need to depend on chromeos in non-chromeos builds?

I don't think so, am I missing something? (This is inside the is_chromeos "if".)

[Devlin, 2017-02-13 22:15:12]

lgtm

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

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:15: if (enable_extensions) {
On 2017/02/13 21:08:42, michaelpg wrote:
> On 2017/02/13 16:07:21, Devlin wrote:
> > what includes extensions/* when enable_extensions is false?  We have a good
> > number of asserts to try and make sure this doesn't happen, e.g.
> >
>
https://chromium.googlesource.com/chromium/src/+/28ff32abc83ae8939f56702151b3....
> >  Where does this fail?
> 
> Dunno, maybe nowhere. But I don't want to remove this existing check (moved
from
> below) in combination with this re-grouping. 
> 
> In fact there's already a TODO for this (also moved from below). It may
already
> be toDONE, but there are other configs I can't check right now (Win/Mac).

Hmm... okay.  But let's look into this soon.

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
File extensions/browser/api/BUILD.gn (right):

https://codereview.chromium.org/2684393002/diff/40001/extensions/browser/api/...
extensions/browser/api/BUILD.gn:113: deps += [ "//chromeos" ]
On 2017/02/13 21:08:42, michaelpg wrote:
> On 2017/02/13 16:07:21, Devlin wrote:
> > Do we need to depend on chromeos in non-chromeos builds?
> 
> I don't think so, am I missing something? (This is inside the is_chromeos
"if".)

Whoops; my block-vision was off.  Nvm.

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

https://codereview.chromium.org/2684393002/diff/80001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:36: "//extensions/browser/api:api",
nit: I'm pretty sure that in gn,
//extensions/browser/api
===
//extensions/browser/api:api

In which case, we should be consistent.  And line 21 omits the :api.

[michaelpg, 2017-02-13 22:21:03]

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

https://codereview.chromium.org/2684393002/diff/80001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:36: "//extensions/browser/api:api",
On 2017/02/13 22:15:12, Devlin wrote:
> nit: I'm pretty sure that in gn,
> //extensions/browser/api
> ===
> //extensions/browser/api:api
> 
> In which case, we should be consistent.  And line 21 omits the :api.

Yes, it does, I tried to do that everywhere. Thanks.

[michaelpg, 2017-02-13 22:21:13]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-02-13 22:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2684393002/#ps100001
(title: "shorthand")

[commit-bot: I haz the power, 2017-02-13 22:21:59]

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, 2017-02-14 00:22:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487024473315520,
"parent_rev": "f9bf372ebfcea5043ba08a15320e8ddace508f19", "commit_rev":
"09c1c7e0c3dff51be0c613a8163ab31f19f4a7a6"}

[commit-bot: I haz the power, 2017-02-14 00:23:03]

Description was changed from

==========
Update deps in extensions/browser/BUILD.gn to pass gn check.

I think this change improves things... if we care about `gn check`.

Creates a couple new source_sets solely for the purpose of avoiding
circular dependencies between //extensions/browser and
//extensions/browser/api.

Alternatively, we could move browser_context_keyed_service_factories.*
down into api/, but it's not clear they belong there.

BUG=none
==========

to

==========
Update deps in extensions/browser/BUILD.gn to pass gn check.

I think this change improves things... if we care about `gn check`.

Creates a couple new source_sets solely for the purpose of avoiding
circular dependencies between //extensions/browser and
//extensions/browser/api.

Alternatively, we could move browser_context_keyed_service_factories.*
down into api/, but it's not clear they belong there.

BUG=none

Review-Url: https://codereview.chromium.org/2684393002
Cr-Commit-Position: refs/heads/master@{#450168}
Committed:
https://chromium.googlesource.com/chromium/src/+/09c1c7e0c3dff51be0c613a8163a...
==========

[commit-bot: I haz the power, 2017-02-14 00:23:04]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/09c1c7e0c3dff51be0c613a8163a...