Also, we should try to get code in extensions/ whitelisted for gn check. Not sure if there's a related bug.

Yeah, we've run into this before with api directories (I think some, though not all, of the conversation is captured in https://codereview.chromium.org/2812013003/).  The problem is that the subdirectories in extensions don't really represent independent build targets, but just a grouping of related files.  While having a separate BUILD.gn file for each subdirectory avoids having giant targets, it's not really correct - the files in subdirectories and at top-level //extensions/browser are inherently interrelated.

I think overall the consensus has been that, really, there should just be one extensions/browser target for all of these.

Yeah a monolithic build target should work. There may be better solutions as well like e/b/api depending on e/b with e/b prohibiting any includes to e/b/api. But these all would require extensive refactoring.

Devlin: Does it sound good if I move targets in e/b/guest_view to e/b:browser_sources. However it will still have a cyclic dependency to e/b/api (due to its use of ExtensionsAPIClient).

But this can be removed by migrating guest view related functions from ExtensionsAPIClient to ExtensionsBrowserClient (these functions don't depend on e/b/api). 

Nevermind, there will still be other dependencies. A single build target seems to be the thing to do.

I tried to fix this a few months ago and realized that there is an inherent issue with the generation of the bindings files and the dependencies that we have on that code.

Unfortunately this was long enough ago that I no longer recall any details. Adding Michael in case he has done any investigation here.

The biggest thing is that most of our subdirectories in extensions are only there for a logical grouping of files, rather than any kind of dependency chain.  There are a few exceptions, but for the most part, most stuff is inter-dependent, and should all be in the same build target.

If we wanted to institute logical dependencies, we'd probably want a dependency from apis -> core system (the APIs know about the core system, but the core system doesn't know about the APIs).  We could probably do this in a few places, but a) there are still exceptions - e.g. we call directly into the runtime API a few places - and b) this is actually the opposite of what we currently have (where, according to the build files, the core system depends on the APIs). :)

There would be some benefit to cleaning all of this up, but for now I think we'd settle for a build that passes gn checks - which largely just means grouping everything together.

Actually stuff in e/b/api does depend on e/b:browser_sources => Hence I think the APIs depend on the core system today, which seems logical.

Ah, so it does.  I'm either misremembering or someone helped clean this up a bit at some point.  Either way, yay!  We're not as far off as I thought.

Yeah, I haven't looked much into stuff in e/b/api, but it may just be a case of adding missing dependencies to GN files, for the most part. However, again I am not really sure about where stuff in e/b/guest_view belongs with the way targets are currently organized, and even conceptually.

I created the //extensions/browser:browser_sources target a while ago, and got gn check working for //extensions/browser: https://codereview.chromium.org/2684393002

I thought that improved the circular dependency issue. Lumping all of //e/b/api/* into //e/b seems like a regression (and that has caused me to break the tree in the past).

Classes in //e/b/api are always going to depend on ExtensionsAPIClient and I think keeping that in //e/b/api makes sense -- meanwhile core extensions stuff outside of //e/b/api probably shouldn't depend on it.

The ExtensionsAPIClient functions used by //e/b/guest_view seem to be guest-view-specific. Can we just move them to ExtensionsBrowserClient?

>> The ExtensionsAPIClient functions used by //e/b/guest_view seem to be guest-view-specific. Can we just move them to ExtensionsBrowserClient?

Yeah I also alluded to this in c#4. But there are also some other e/b/api dependencies in e/b/guest_view:

app_view/app_view_guest.cc:#include "extensions/browser/api/app_runtime/app_runtime_api.h"
mime_handler_view/mime_handler_view_guest.cc:#include "extensions/browser/api/mime_handler_private/mime_handler_private.h"
web_view/web_view_apitest.cc:#include "extensions/browser/api/test/test_api.h"
web_view/web_view_find_helper.cc:#include "extensions/browser/api/guest_view/web_view/web_view_internal_api.h"
web_view/web_view_guest.cc:#include "extensions/browser/api/declarative/rules_registry_service.h"
web_view/web_view_guest.cc:#include "extensions/browser/api/guest_view/web_view/web_view_internal_api.h"
web_view/web_view_guest.cc:#include "extensions/browser/api/web_request/web_request_api.h"

It seems to me that most of these can be solved by moving code from e/b/api to e/b. However, e/b/guest_view/web_view/ seems a bit tricky.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05ba7bc22678febb0eff54435aa1acca3b06878b

commit 05ba7bc22678febb0eff54435aa1acca3b06878b
Author: karandeepb <karandeepb@chromium.org>
Date: Wed Jun 14 00:49:45 2017

Extensions: Move targets in extensions/browser/guest_view to extensions/browser:browser_sources.

Currently the dependency chain is as follows: extensions/browser ->
extensions/browser/api -> extensions/browser:browser_sources ->
extensions/browser/guest_view. However extensions/browser/guest_view also has
cyclic dependencies on extensions/browser/api and
extensions/browser:browser_sources. This can be problematic since
extensions/browser/guest_view won't get any configs it needs for its dependency
on extensions/browser:browser_sources and extensions/browser/api.

This CL moves the files in extensions/browser/guest_view, one layer up to
extensions/browser:browser_sources, hence removing the cyclic dependency between
extensions/browser/guest_view and extensions/browser:browser_sources. However,
the cyclic dependency to extensions/browser/api is still not resolved.

BUG=730220

Review-Url: https://codereview.chromium.org/2932643004
Cr-Commit-Position: refs/heads/master@{#479231}

[modify] https://crrev.com/05ba7bc22678febb0eff54435aa1acca3b06878b/extensions/browser/BUILD.gn
[modify] https://crrev.com/05ba7bc22678febb0eff54435aa1acca3b06878b/extensions/browser/api/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/app_view/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/extension_options/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/extension_view/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/extension_view/whitelist/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/mime_handler_view/BUILD.gn
[delete] https://crrev.com/adeae69afea91b503de336275bad316a43080ee4/extensions/browser/guest_view/web_view/BUILD.gn

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

[Extensions Triage] Not a high priority as of now. Marking as Available.