From 60474db16fe1f61604b06eb24ead3fa2422c6204 Mon Sep 17 00:00:00 2001 From: Brian Johnson <34129+bridiver@users.noreply.github.com> Date: Wed, 13 Nov 2024 08:34:30 -0700 Subject: [PATCH] Modularize brave news internals (#26445) Modularize brave_news_internals --- .github/CODEOWNERS | 3 +++ browser/extensions/BUILD.gn | 1 - browser/sources.gni | 6 +++++ browser/ui/BUILD.gn | 19 ++++++++------- browser/ui/config.gni | 10 ++++++++ .../ui/webui/brave_news_internals/BUILD.gn | 18 ++++++++++++++ .../brave_news_internals_ui.cc | 24 +++++++------------ .../brave_news_internals_ui.h | 10 ++++++-- .../webui/brave_web_ui_controller_factory.cc | 5 +++- ...hrome-browser-safe_browsing-BUILD.gn.patch | 12 ---------- patches/chrome-browser-ui-BUILD.gn.patch | 12 ++++++++-- 11 files changed, 78 insertions(+), 42 deletions(-) create mode 100644 browser/ui/config.gni create mode 100644 browser/ui/webui/brave_news_internals/BUILD.gn delete mode 100644 patches/chrome-browser-safe_browsing-BUILD.gn.patch diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 25932600d476..23905f92095b 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -10,6 +10,9 @@ DEPS @brave/deps-reviewers script/deps.py @brave/deps-reviewers package*.json @brave/js-deps-reviewers +# brave/browser/ui/BUILD.gn should not have new files added to it +browser/ui/BUILD.gn @bridiver + # BraveBrowserProcess public interface browser/brave_browser_process.h @bridiver diff --git a/browser/extensions/BUILD.gn b/browser/extensions/BUILD.gn index d30a5bb2ec12..c5bc55eb1a47 100644 --- a/browser/extensions/BUILD.gn +++ b/browser/extensions/BUILD.gn @@ -133,7 +133,6 @@ source_set("extensions") { "//brave/browser/extensions/api:generated_api_registration", "//brave/browser/themes", "//brave/browser/ui:brave_tab_prefs", - "//brave/browser/ui:ui_public_dependencies", "//brave/common", "//brave/common/extensions/api", "//brave/components/ai_chat/core/common", diff --git a/browser/sources.gni b/browser/sources.gni index d2d559b38ffc..bfc253936934 100644 --- a/browser/sources.gni +++ b/browser/sources.gni @@ -38,6 +38,7 @@ import("//brave/browser/search_engines/sources.gni") import("//brave/browser/speedreader/sources.gni") import("//brave/browser/sync/sources.gni") import("//brave/browser/themes/sources.gni") +import("//brave/browser/ui/config.gni") import("//brave/browser/url_sanitizer/sources.gni") import("//brave/browser/web_discovery/sources.gni") import("//brave/browser/webcompat_reporter/sources.gni") @@ -285,6 +286,7 @@ if (!is_android) { brave_chrome_browser_deps += [ "//brave/browser/ui/ai_chat", + "//brave/browser/ui/webui/brave_news_internals", "//components/feed:feature_list", "//components/feed/core/v2:feed_core_v2", "//components/feed/mojom:mojo_bindings", @@ -575,6 +577,10 @@ brave_chrome_browser_deps += brave_browser_webcompat_reporter_deps brave_chrome_browser_deps += brave_chromium_src_chrome_browser_deps brave_chrome_browser_deps += brave_chromium_src_chrome_browser_prefs_deps +brave_chrome_browser_ui_allow_circular_includes_from = [ "//brave/browser/ui" ] +brave_chrome_browser_ui_allow_circular_includes_from += + brave_ui_allow_circular_includes_from + brave_chrome_browser_allow_circular_includes_from = [ "//brave/browser/ui", "//brave/browser/brave_ads:impl", diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 4f2e3c2b89f3..ba4aeb1fd94c 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -5,6 +5,7 @@ import("//brave/browser/ethereum_remote_client/buildflags/buildflags.gni") import("//brave/browser/shell_integrations/buildflags/buildflags.gni") +import("//brave/browser/ui/config.gni") import("//brave/build/config.gni") import("//brave/components/ai_rewriter/common/buildflags/buildflags.gni") import("//brave/components/brave_vpn/common/buildflags/buildflags.gni") @@ -23,11 +24,12 @@ import("//mojo/public/tools/bindings/mojom.gni") import("//printing/buildflags/buildflags.gni") import("//third_party/widevine/cdm/widevine.gni") -# Some targets have allow_circular_includes_from for //chrome/browser/ui so they -# are missing the dependency that would otherwise be required for it. This is a -# workaround to add the public deps from //brave/browser/ui that would be added -# automatically if the //chrome/browser/ui dependency was present. +# See //chrome/browser/ui:ui_public_depdendencies. group("ui_public_dependencies") { + visibility = [ + "//brave/browser/ui", + "//chrome/browser/ui:ui_public_dependencies", + ] public_deps = [ ":brave_rewards_source", ":brave_tab_features", @@ -57,6 +59,9 @@ source_set("ui") { "//chrome/browser/ui:browser_navigator_params_headers", ] + allow_circular_includes_from = brave_ui_allow_circular_includes_from + deps += brave_ui_allow_circular_includes_from + public_deps = [ ":ui_public_dependencies" ] sources = [ @@ -771,6 +776,7 @@ source_set("ui") { "//brave/components/brave_ads/core", "//brave/components/brave_federated", "//brave/components/brave_federated/public/interfaces", + "//brave/components/brave_news/browser", "//brave/components/brave_news/common", "//brave/components/brave_perf_predictor/common", "//brave/components/brave_rewards/browser", @@ -1046,7 +1052,6 @@ source_set("ui") { "//brave/common/importer", "//brave/components/brave_new_tab_ui:generated_resources", "//brave/components/brave_new_tab_ui:mojom", - "//brave/components/brave_news/browser", "//brave/components/brave_private_new_tab_ui/common", "//brave/components/brave_private_new_tab_ui/common:mojom", "//brave/components/brave_private_new_tab_ui/resources/page:generated_resources", @@ -1245,8 +1250,6 @@ source_set("ui") { "views/wallet_bubble_focus_observer.h", "wallet_bubble_manager_delegate_impl.cc", "wallet_bubble_manager_delegate_impl.h", - "webui/brave_news_internals/brave_news_internals_ui.cc", - "webui/brave_news_internals/brave_news_internals_ui.h", "webui/brave_wallet/ledger/ledger_ui.cc", "webui/brave_wallet/ledger/ledger_ui.h", "webui/brave_wallet/page_handler/wallet_page_handler.cc", @@ -1267,8 +1270,6 @@ source_set("ui") { "//brave/browser/resources/settings/shortcuts_page:generated_resources", "//brave/browser/ui/brave_wallet", "//brave/browser/ui/webui/brave_wallet/common_handler", - "//brave/components/brave_news/browser", - "//brave/components/brave_news/browser/resources:generated_resources", "//brave/components/brave_wallet/browser:permission_utils", "//brave/components/brave_wallet/common", "//brave/components/brave_wallet/common:mojom", diff --git a/browser/ui/config.gni b/browser/ui/config.gni new file mode 100644 index 000000000000..62a9a4940769 --- /dev/null +++ b/browser/ui/config.gni @@ -0,0 +1,10 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +brave_ui_allow_circular_includes_from = [] +if (!is_android) { + brave_ui_allow_circular_includes_from += + [ "//brave/browser/ui/webui/brave_news_internals" ] +} diff --git a/browser/ui/webui/brave_news_internals/BUILD.gn b/browser/ui/webui/brave_news_internals/BUILD.gn new file mode 100644 index 000000000000..b5cca7a5a0e8 --- /dev/null +++ b/browser/ui/webui/brave_news_internals/BUILD.gn @@ -0,0 +1,18 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +source_set("brave_news_internals") { + sources = [ + "brave_news_internals_ui.cc", + "brave_news_internals_ui.h", + ] + + deps = [ + "//brave/components/brave_news/browser", + "//brave/components/brave_news/browser/resources:generated_resources", + "//brave/components/brave_news/common:mojom", + "//chrome/browser:browser_public_dependencies", + ] +} diff --git a/browser/ui/webui/brave_news_internals/brave_news_internals_ui.cc b/browser/ui/webui/brave_news_internals/brave_news_internals_ui.cc index 8790302edb73..d6f04847c546 100644 --- a/browser/ui/webui/brave_news_internals/brave_news_internals_ui.cc +++ b/browser/ui/webui/brave_news_internals/brave_news_internals_ui.cc @@ -8,17 +8,17 @@ #include #include -#include "brave/browser/brave_news/brave_news_controller_factory.h" #include "brave/browser/ui/webui/brave_webui_source.h" #include "brave/components/brave_news/browser/brave_news_controller.h" #include "brave/components/brave_news/browser/resources/grit/brave_news_internals_generated_map.h" #include "brave/components/brave_news/common/brave_news.mojom.h" -#include "chrome/browser/profiles/profile.h" #include "components/grit/brave_components_resources.h" -BraveNewsInternalsUI::BraveNewsInternalsUI(content::WebUI* web_ui, - const std::string& host) - : content::WebUIController(web_ui) { +BraveNewsInternalsUI::BraveNewsInternalsUI( + content::WebUI* web_ui, + const std::string& host, + brave_news::BraveNewsController* controller) + : content::WebUIController(web_ui), controller_(controller) { auto* source = CreateAndAddWebUIDataSource( web_ui, host, kBraveNewsInternalsGenerated, kBraveNewsInternalsGeneratedSize, IDR_BRAVE_NEWS_INTERNALS_HTML); @@ -30,24 +30,18 @@ WEB_UI_CONTROLLER_TYPE_IMPL(BraveNewsInternalsUI) void BraveNewsInternalsUI::BindInterface( mojo::PendingReceiver receiver) { - auto* profile = Profile::FromWebUI(web_ui()); - auto* controller = - brave_news::BraveNewsControllerFactory::GetForBrowserContext(profile); - if (!controller) { + if (!controller_) { return; } - controller->Bind(std::move(receiver)); + controller_->Bind(std::move(receiver)); } void BraveNewsInternalsUI::BindInterface( mojo::PendingReceiver receiver) { - auto* profile = Profile::FromWebUI(web_ui()); - auto* controller = - brave_news::BraveNewsControllerFactory::GetForBrowserContext(profile); - if (!controller) { + if (!controller_) { return; } - controller->Bind(std::move(receiver)); + controller_->Bind(std::move(receiver)); } diff --git a/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h b/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h index 78775610c9a6..0faeb8b3caa2 100644 --- a/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h +++ b/browser/ui/webui/brave_news_internals/brave_news_internals_ui.h @@ -6,17 +6,22 @@ #ifndef BRAVE_BROWSER_UI_WEBUI_BRAVE_NEWS_INTERNALS_BRAVE_NEWS_INTERNALS_UI_H_ #define BRAVE_BROWSER_UI_WEBUI_BRAVE_NEWS_INTERNALS_BRAVE_NEWS_INTERNALS_UI_H_ -#include #include +#include "base/memory/raw_ptr.h" #include "brave/components/brave_news/common/brave_news.mojom-forward.h" #include "content/public/browser/web_ui_controller.h" #include "mojo/public/cpp/bindings/pending_receiver.h" +namespace brave_news { +class BraveNewsController; +} + class BraveNewsInternalsUI : public content::WebUIController { public: explicit BraveNewsInternalsUI(content::WebUI* web_ui, - const std::string& host); + const std::string& host, + brave_news::BraveNewsController* controller); BraveNewsInternalsUI(const BraveNewsInternalsUI&) = delete; BraveNewsInternalsUI& operator=(const BraveNewsInternalsUI&) = delete; ~BraveNewsInternalsUI() override; @@ -27,6 +32,7 @@ class BraveNewsInternalsUI : public content::WebUIController { mojo::PendingReceiver receiver); private: + raw_ptr controller_ = nullptr; WEB_UI_CONTROLLER_TYPE_DECL(); }; diff --git a/browser/ui/webui/brave_web_ui_controller_factory.cc b/browser/ui/webui/brave_web_ui_controller_factory.cc index 6538b57eb32f..6db0b56d592d 100644 --- a/browser/ui/webui/brave_web_ui_controller_factory.cc +++ b/browser/ui/webui/brave_web_ui_controller_factory.cc @@ -11,6 +11,7 @@ #include "base/feature_list.h" #include "base/memory/ptr_util.h" #include "base/no_destructor.h" +#include "brave/browser/brave_news/brave_news_controller_factory.h" #include "brave/browser/brave_rewards/rewards_util.h" #include "brave/browser/ethereum_remote_client/buildflags/buildflags.h" #include "brave/browser/ui/webui/brave_rewards/rewards_page_ui.h" @@ -128,7 +129,9 @@ WebUIController* NewWebUI(WebUI* web_ui, const GURL& url) { } else if (base::FeatureList::IsEnabled( brave_news::features::kBraveNewsFeedUpdate) && host == kBraveNewsInternalsHost) { - return new BraveNewsInternalsUI(web_ui, url.host()); + return new BraveNewsInternalsUI( + web_ui, url.host(), + brave_news::BraveNewsControllerFactory::GetForBrowserContext(profile)); } else if (host == kWelcomeHost && !profile->IsGuestSession()) { return new BraveWelcomeUI(web_ui, url.host()); } else if (host == chrome::kChromeUINewTabHost) { diff --git a/patches/chrome-browser-safe_browsing-BUILD.gn.patch b/patches/chrome-browser-safe_browsing-BUILD.gn.patch deleted file mode 100644 index 40fa983f917b..000000000000 --- a/patches/chrome-browser-safe_browsing-BUILD.gn.patch +++ /dev/null @@ -1,12 +0,0 @@ -diff --git a/chrome/browser/safe_browsing/BUILD.gn b/chrome/browser/safe_browsing/BUILD.gn -index 19cc1b47ecf4a970f930f7c0b7e802d2739d5802..d37aff1fb03b7383ee5a33ddabc9ac1a13e292a6 100644 ---- a/chrome/browser/safe_browsing/BUILD.gn -+++ b/chrome/browser/safe_browsing/BUILD.gn -@@ -485,6 +485,7 @@ static_library("safe_browsing") { - "//components/safe_browsing/android:safe_browsing_mobile", - ] - } -+ deps += [ "//brave/browser/ui:ui_public_dependencies" ] # safe_browsing should depend on //chrome/browser/ui but it would cause a circular dependency - } - } - diff --git a/patches/chrome-browser-ui-BUILD.gn.patch b/patches/chrome-browser-ui-BUILD.gn.patch index c2324a7ac2d0..7365deea5dbf 100644 --- a/patches/chrome-browser-ui-BUILD.gn.patch +++ b/patches/chrome-browser-ui-BUILD.gn.patch @@ -1,12 +1,12 @@ diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn -index 4c9e41d73cedd5eafeb65832c1a8e172484a84cd..e5897fcb367c20c7235d56c11e6f35b51a69f554 100644 +index 4c9e41d73cedd5eafeb65832c1a8e172484a84cd..2a59f5e6a414f61dbc39a6e305109db366539352 100644 --- a/chrome/browser/ui/BUILD.gn +++ b/chrome/browser/ui/BUILD.gn @@ -741,6 +741,7 @@ static_library("ui") { "//chrome/browser/content_settings:impl", "//chrome/browser/content_settings:content_settings_util_impl", ] -+ public_deps += [ "//brave/browser/ui" ] allow_circular_includes_from += [ "//brave/browser/ui" ] ++ import("//brave/browser/sources.gni") public_deps += [ "//brave/browser/ui" ] deps += brave_chrome_browser_ui_allow_circular_includes_from allow_circular_includes_from += brave_chrome_browser_ui_allow_circular_includes_from if (enable_vr && is_win) { deps += [ "//chrome/browser/vr:vr_base" ] @@ -22,3 +22,11 @@ index 4c9e41d73cedd5eafeb65832c1a8e172484a84cd..e5897fcb367c20c7235d56c11e6f35b5 } } +@@ -5709,6 +5713,7 @@ static_library("ui_public_dependencies") { + "//content/public/browser", + "//mojo/public/cpp/bindings", + ] ++ public_deps += [ "//brave/browser/ui:ui_public_dependencies" ] + if (!is_android) { + public_deps += [ + "//build:branding_buildflags",