Fold //brave/browser:browser_process GN target into //chrome/browser #8538
Conversation
browser/sources.gni
Outdated
| "//brave/components/cosmetic_filters/browser", | ||
| "//brave/components/cosmetic_filters/common:mojom", | ||
| "//brave/components/crypto_dot_com/browser/buildflags", | ||
| "//brave/components/crypto_dot_com/browser", |
There was a problem hiding this comment.
shouldn't we add this only if the buildflag is enabled? also, this and similar entries are injected below with `brave_chrome_browser_deps += brave_browser_crypto_dot_com_deps``
There was a problem hiding this comment.
Hmm... I think you're right, looks like I need to make a few other adjustments here, will adapt + fix other platforms (ios / Mac) and will ping you back. Thanks for the review!
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
2 times, most recently
from
20193c1 to
43861a7
Compare
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
4 times, most recently
from
4a4ba34 to
2dd065f
Compare
browser/autocomplete/sources.gni
Outdated
| brave_browser_autocomplete_deps = [ | ||
| "//base", | ||
| "//brave/common", | ||
| "//brave/components/brave_webtorrent/browser", |
There was a problem hiding this comment.
this one and "//brave/components/ipfs" should be added conditionally, depending on the buildflags
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
from
2dd065f to
e0d2abe
Compare
| ] | ||
|
|
||
| brave_browser_autocomplete_deps = [ | ||
| "//base", |
There was a problem hiding this comment.
some of sources.gni has this dependency, while others are not - I'm guessing we can actually omit it
There was a problem hiding this comment.
In this particular case it's because of the #include "base/strings/string_util.h" include from brave/browser/autocomplete/brave_autocomplete_scheme_classifier.cc.
As for the inconsistency with other files, I think I might have made a mistake and didn't notice because the gn_check step didn't catch it now that this has been assimilated into the main //chrome/browser target. I'll do another pass over all these new sources.gni file and will add what's missing (vs removing them, as I think it's more interesting, see my comment in #8538 (comment))
There was a problem hiding this comment.
Done another pass and fix a few missing deps (sorry not having noticed earlier), put the difference as fixup commits on top of this PR to ease review (will squash them before merge).
| "//brave/common", | ||
| "//brave/components/brave_webtorrent/browser/buildflags", | ||
| "//brave/components/ipfs/buildflags", | ||
| "//chrome/browser/profiles:profile", |
There was a problem hiding this comment.
Wondering whether we actually need this one and similar chrome dependencies, given that we inject our sources into the target that has them
There was a problem hiding this comment.
If it's ok to you, I think it's better to keep them because that way it's clearer what exactly a given subset of sources depend on, which IMHO makes it harder for people to forget about adding new deps as needed when adding new files to these sources.gni in the future. WDYT?
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
2 times, most recently
from
4df80d1 to
7845c76
Compare
|
@tmancey since this has already been reviewed by @simonhong and @iefremov, I'm adding @bridiver to the reviewers list in case he can cover the files you own, so that we can land this asap (as other PRs conflict with it). Hope that's ok! |
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
2 times, most recently
from
ee3ddf1 to
aacf937
Compare
This is the very first step towards a clearer way to organize and state dependencies between targets: for now we just remove this target and fold everything into //chrome/browser via a massive dump into browser/sources.gni with no further changes, plus the necessary adaptations wherever the old target was referenced. Once this is in place, next steps will extract different subsets of the sources dumped in there into separate .gni files, so that it can be more clearly stated which dependencies are actually related to which subset of source files, instead of having the giant merge of dependencies we had until now in a single target. Once those separate .gni files are in place, and each of them with their own clear set of dependencies, we should be in a much better position to reorganize things and see what can be moved around into independent GN targets instead of being part of the //chrome/browser main target. Finally, this change also makes the //brave/browser target a group instead of a source_set and adjust visibility rules in a more precise way, so that we can still reference //brave/browser from different places in Brave (vs having to reference //chrome/browser).
List the source files from browser/sources.gni related to //brave/browser/brave_rewards in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/ipfs in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/binance in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/greaselion in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/speedreader in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/decentralized_dns in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/crypto_dot_com in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/gemini in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/autocomplete in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/brave_shields in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/component_updater in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
List the source files from browser/sources.gni related to //brave/browser/search_engines in a separate sources.gni file, being explicit on the actual list of dependencies required by this subset of source files.
Remove imports that are not actually needed and update the list of dependencies for the sources included directly from this .gni file so that we're only including what it's needed, and also adding those dependencies that were missing before.
mariospr
force-pushed
the
fix-gn-deps-brave_browser_process
branch
from
aacf937 to
71900a5
Compare
|
Errors are unrelated, so merging. Thank you all for the reviews! |
This implements first step that will make it easier to fix GN deps by allowing
us to get rid of the //brave/browser:browser_process and moving its contents
into //chrome/browser, so that we can more easily work on breaking cycles on
other //brave/browser/* targets once this is in place.
More specifically, this patch implements the following sequence of ideas:
Move contents from //brave/browser:browser_process into //chrome/browser.
Identify logical subsets of sources from Brave's browser_process target and
move them into separate .gni files with proper dependencies for each of them.
Cleanup the result //brave/browser/sources.gni after that, making sure that
the dependencies there are the one actually needed by the remaining sources.
Resolves brave/brave-browser#12623
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on