New issue
Advanced search Search tips

Issue 883570 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Extensions/Apps APIs, GN Deps, and GN check

Project Member Reported by rdevlin....@chromium.org, Sep 13

Issue description

From an architectural standpoint, most Extension and App APIs should be leaf node dependencies.  Core chrome (or even core extensions system) code should not be bothered with these APIs.  Instead, the APIs should typically rely on observer interfaces to monitor events, and use the constructs available to them to change behavior.  There shouldn't be common reasons for the core chrome code to call into an API, and most cases where this happens is likely not ideal layering (though for practicality reasons, we may allow some exceptions).

From this standpoint, it would be very nice to have a dependency graph like:
API -> [ apps|extensions, browser, profiles, ui, etc ]

However, the API also needs to be bundled in chrome itself - thus, something, somewhere, must depend upon it.  Currently, this is in the extensions or apps targets (i.e., chrome/browser/extensions, chrome/browser/apps/platform_apps), which results in a dependency graph like:
browser -> apps|extensions -> APIs -> [ apps|extensions, browser, profiles, ui, etc ]

This causes a circular dependency between 'browser' and the API target, which is resolved by adding "allow_circular_includes_from" in the BUILD.gn file.  However, this circular dependency isn't strictly necessary, since the browser doesn't often depend on these APIs; they just need to be included somewhere.  Instead of being depended on by the browser target, they could be included elsewhere, breaking the circular dependency and removing the need for "allow_circular_includes_from".

chrome:browser_dependencies [1] is perhaps a conceptually-appropriate place for these: it is something that would cause them to be built, but that they themselves should not rely upon.  Then, the dependency graphs would be:

chrome:browser_dependencies -> [ browser, profiles, ui, apps, extensions, APIs, etc ]
API -> [ apps|extensions, browser, profiles, ui, etc ]

Here, there are no dependency cycles or circular includes - yay!  APIs are mostly-correctly represented as leaf nodes, with only the chrome binary (ish) depending upon them - which will always be necessary to ensure they are built.

However, there's at least one hiccup: many APIs require the instantiation of a profile-keyed service - often just used as a glorified singleton - that needs to be instantiated in order to register necessary observers.  The registration of all profile keyed services currently happens in ChromeBrowserMainExtraPartsProfiles::EnsureBrowserContextKeyedServiceFactoriesBuilt() [2], in chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc.  This means that the profiles code has to depend upon anything and everything that has a keyed service.

There's a rather appropriate TODO above the method:
// TODO(erg): This needs to be something else. I don't think putting every
// FooServiceFactory here will scale or is desirable long term.

In order to properly break the dependency graph, we basically have to resolve that TODO.

I think, conceptually, the right thing to do here would be to pull factory registration outside chrome/browser (and profile) code, up to a similar level of chrome:browser_dependencies.  That target could then depend on the "anything and everything with a keyed service", but would mean that chrome/browser need not have knowledge of each individual piece.  I reckon this would actually break a number of similar dependency graphs, where things need keyed services and depend upon browser concepts, but the browser need not know about them.

I'm curious for input and ideas here, so adding some folks that may have thoughts.  Any opinions?  Simpler approaches?  Thoughts on whether such rearchitecture is worth the investment, rather than having a monolithic browser target?  Please chime in, and add other folks!

[1] https://chromium.googlesource.com/chromium/src/+/d10f17a0805b69b6795d691bf11c342afd217685/chrome/BUILD.gn#1428
[2] https://chromium.googlesource.com/chromium/src/+/a6d369fa8ac4fb96b99f3cffd1841cdd15fca490/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc#202
 
Cc: -dominickn@google.com -dpranke@google.com -thestig@google.com dominickn@chromium.org dpranke@chromium.org thestig@chromium.org
Thanks, thestig@ - no idea why crbug added everyone's @google by default :(  Definitely not my intent.

Sign in to add a comment