Fix deps in //brave/browser/themes #6887
Conversation
e3fdd0e to
6dacb96
Compare
6dacb96 to
f92c220
Compare
| ] | ||
|
|
||
| if (!is_android) { | ||
| sources += [ | ||
| "brave_dark_mode_utils.cc", | ||
| "brave_dark_mode_utils_internal.cc", |
There was a problem hiding this comment.
I think whole sources/deps can be moved to sources.gni instead of spliting. WDYT?
There was a problem hiding this comment.
yes, I tried to do it first. But, //brave/browser has a dependency //brave/browser/themes. So, it needs to be kept.
There was a problem hiding this comment.
I think we can deprecate themes target.
There was a problem hiding this comment.
Do we use this splitting pattern commonly? If so, I'm fine.
There was a problem hiding this comment.
Below //brave/browser files have been including //brave/browser/themes/foo files. So, I'm not fully sure whether it's fine to remove the target.
- //brave/browser/brave_browser_process_impl.cc,
- //brave/browser/brave_local_state_pref.cc,
- //brave/browser/ui/views/inforbars/brave_wayback_machine_inforbar_contents_view.cc
...
And also, some brave modules have been using both BUILD.gn and sources.gni and it will be used more to fix the dependency violation.
- //brave/renderer/sources.gni, BUILD.gn
- //brave/browser/brave_wallet/sources.gni, BUILD.gn
- //brave/browser/sources.gni, BUILD.gn
...
Even, Brian also approved //brave/browser/importer - #6664
brave_theme_helper_win.cc/h were moved out into sources.gni by #6887
Resolves brave/brave-browser#10656.
Submitter Checklist:
npm run lint,npm run gn_check)git rebase master(if needed).git rebase -ito squash commits (if needed).QA/YesorQA/No) to the associated issuerelease-notes/includeorrelease-notes/exclude) to the associated issueTest Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.