From 5e79e1818c90a8d70d4b18bfd76e3163e6eae908 Mon Sep 17 00:00:00 2001 From: Vadym Struts Date: Wed, 11 Dec 2024 21:35:48 +0100 Subject: [PATCH] review changes Signed-off-by: Vadym Struts --- .../org/chromium/base/BraveFeatureList.java | 2 +- .../chrome/browser/app/BraveActivity.java | 5 -- .../BraveCosmeticFiltersUtils.java | 11 ---- .../browser/shields/BraveShieldsHandler.java | 2 +- browser/about_flags.cc | 3 +- .../cosmetic_filters_utils.cc | 5 -- .../cosmetic_filters/cosmetic_filters_utils.h | 1 - .../cosmetic_filters_tab_helper.cc | 16 ++---- .../cosmetic_filters_tab_helper.h | 1 - .../flags/android/chrome_feature_list.cc | 2 +- .../render_view_context_menu.cc | 2 +- components/brave_shields/core/common/BUILD.gn | 5 +- .../core/common/buildflags/BUILD.gn | 12 ----- .../core/common/buildflags/buildflags.gni | 12 ----- .../brave_shields/core/common/features.cc | 11 ++-- .../brave_shields/core/common/features.h | 2 +- .../common/cosmetic_filters.mojom | 7 --- .../renderer/cosmetic_filters_js_handler.cc | 51 ++++--------------- .../renderer/cosmetic_filters_js_handler.h | 7 +-- .../resources/data/element_picker.ts | 10 ++-- components/definitions/chromel.d.ts | 2 +- 21 files changed, 34 insertions(+), 135 deletions(-) delete mode 100644 components/brave_shields/core/common/buildflags/BUILD.gn delete mode 100644 components/brave_shields/core/common/buildflags/buildflags.gni diff --git a/android/java/org/chromium/base/BraveFeatureList.java b/android/java/org/chromium/base/BraveFeatureList.java index 8d5845df420a..69e4e9aebedb 100644 --- a/android/java/org/chromium/base/BraveFeatureList.java +++ b/android/java/org/chromium/base/BraveFeatureList.java @@ -35,5 +35,5 @@ public abstract class BraveFeatureList { public static final String BRAVE_DAY_ZERO_EXPERIMENT = "BraveDayZeroExperiment"; public static final String BRAVE_FALLBACK_DOH_PROVIDER = "BraveFallbackDoHProvider"; public static final String BRAVE_BLOCK_ALL_COOKIES_TOGGLE = "BlockAllCookiesToggle"; - public static final String BRAVE_BLOCK_ELEMENTS_FEATURE = "BlockElementFeature"; + public static final String BRAVE_SHIELDS_ELEMENT_PICKER = "BraveShieldsElementPicker"; } diff --git a/android/java/org/chromium/chrome/browser/app/BraveActivity.java b/android/java/org/chromium/chrome/browser/app/BraveActivity.java index a943dca9236c..b70dfb0460d3 100644 --- a/android/java/org/chromium/chrome/browser/app/BraveActivity.java +++ b/android/java/org/chromium/chrome/browser/app/BraveActivity.java @@ -142,7 +142,6 @@ import org.chromium.chrome.browser.multiwindow.BraveMultiWindowUtils; import org.chromium.chrome.browser.multiwindow.MultiInstanceManager; import org.chromium.chrome.browser.multiwindow.MultiWindowUtils; -import org.chromium.chrome.browser.night_mode.GlobalNightModeStateProviderHolder; import org.chromium.chrome.browser.notifications.permissions.NotificationPermissionController; import org.chromium.chrome.browser.notifications.retention.RetentionNotificationUtil; import org.chromium.chrome.browser.ntp.NewTabPageManager; @@ -1624,10 +1623,6 @@ public void openBraveContentFilteringSettings() { settingsLauncher.startSettings(this, ContentFilteringFragment.class); } - public boolean isNightModeEnabled() { - return GlobalNightModeStateProviderHolder.getInstance().isInNightMode(); - } - public int getBraveThemeBackgroundColor() { return ContextUtils.getApplicationContext() .getColor(R.color.toolbar_background_color_for_ntp); diff --git a/android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java b/android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java index 7e31670880d2..aa5c6a13eea5 100644 --- a/android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java +++ b/android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java @@ -41,17 +41,6 @@ public static int getThemeBackgroundColor() { return backgroundColor; } - @CalledByNative - public static boolean isNightModeEnabled() { - boolean isDarkModeEnabled = false; - try { - isDarkModeEnabled = BraveActivity.getBraveActivity().isNightModeEnabled(); - } catch (BraveActivity.BraveActivityNotFoundException e) { - Log.e(TAG, "Get nightly mode status" + e); - } - return isDarkModeEnabled; - } - @NativeMethods interface Natives { boolean launchContentPickerForWebContent(Tab tab); diff --git a/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java b/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java index cdd41fce3883..b93420fe9fb5 100644 --- a/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java +++ b/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java @@ -749,7 +749,7 @@ private void setUpSwitchLayouts() { TextView blockElementsText = mSecondaryLayout.findViewById(R.id.brave_shields_block_element_text); blockElementsText.setVisibility( - ChromeFeatureList.isEnabled(BraveFeatureList.BRAVE_BLOCK_ELEMENTS_FEATURE) + ChromeFeatureList.isEnabled(BraveFeatureList.BRAVE_SHIELDS_ELEMENT_PICKER) ? View.VISIBLE : View.GONE); blockElementsText.setOnClickListener( diff --git a/browser/about_flags.cc b/browser/about_flags.cc index 262806810f2b..83dba169c756 100644 --- a/browser/about_flags.cc +++ b/browser/about_flags.cc @@ -693,7 +693,8 @@ "Allows to block sekected HTML element on the page" \ "marked as a known threat.", \ kOsAll, \ - FEATURE_VALUE_TYPE(brave_shields::features::kBlockElementFeature), \ + FEATURE_VALUE_TYPE( \ + brave_shields::features::kBraveShieldsElementPicker), \ }, \ { \ "brave-super-referral", \ diff --git a/browser/android/cosmetic_filters/cosmetic_filters_utils.cc b/browser/android/cosmetic_filters/cosmetic_filters_utils.cc index 4e8143ae02dc..e850457af0da 100644 --- a/browser/android/cosmetic_filters/cosmetic_filters_utils.cc +++ b/browser/android/cosmetic_filters/cosmetic_filters_utils.cc @@ -35,9 +35,4 @@ int32_t GetThemeBackgroundColor() { return Java_BraveCosmeticFiltersUtils_getThemeBackgroundColor(env); } -bool IsDarkModeEnabled() { - JNIEnv* env = base::android::AttachCurrentThread(); - return Java_BraveCosmeticFiltersUtils_isNightModeEnabled(env); -} - } // namespace cosmetic_filters diff --git a/browser/android/cosmetic_filters/cosmetic_filters_utils.h b/browser/android/cosmetic_filters/cosmetic_filters_utils.h index c16417b795f4..1b86ab9a051d 100644 --- a/browser/android/cosmetic_filters/cosmetic_filters_utils.h +++ b/browser/android/cosmetic_filters/cosmetic_filters_utils.h @@ -12,7 +12,6 @@ namespace cosmetic_filters { void ShowCustomFilterSettings(); int GetThemeBackgroundColor(); -bool IsDarkModeEnabled(); } // namespace cosmetic_filters diff --git a/browser/cosmetic_filters/cosmetic_filters_tab_helper.cc b/browser/cosmetic_filters/cosmetic_filters_tab_helper.cc index c325500a6c07..a43df64272a6 100644 --- a/browser/cosmetic_filters/cosmetic_filters_tab_helper.cc +++ b/browser/cosmetic_filters/cosmetic_filters_tab_helper.cc @@ -24,6 +24,7 @@ #include "ui/color/color_provider.h" #else #include "brave/browser/android/cosmetic_filters/cosmetic_filters_utils.h" +#include "chrome/browser/flags/android/chrome_session_state.h" #endif // !BUILDFLAG(IS_ANDROID) namespace cosmetic_filters { @@ -105,21 +106,12 @@ void CosmeticFiltersTabHelper::GetElementPickerThemeInfo( GetWebContents().GetColorMode() == ui::ColorProviderKey::ColorMode::kDark, color_provider.GetColor(kColorSidePanelBadgeBackground)); #else // !BUILDFLAG(IS_ANDROID) - std::move(callback).Run(IsDarkModeEnabled(), GetThemeBackgroundColor()); + std::move(callback).Run(chrome::android::GetDarkModeState() == + chrome::android::DarkModeState::kDarkModeSystem, + GetThemeBackgroundColor()); #endif // !BUILDFLAG(IS_ANDROID) } -void CosmeticFiltersTabHelper::InitElementPicker( - InitElementPickerCallback callback) { - std::move(callback).Run( -#if !BUILDFLAG(IS_ANDROID) - mojom::RunningPlatform::kDesktop -#else // !BUILDFLAG(IS_ANDROID) - mojom::RunningPlatform::kAndroid -#endif // !BUILDFLAG(IS_ANDROID) - ); -} - CosmeticFiltersTabHelper::CosmeticFiltersTabHelper( content::WebContents* web_contents) : content::WebContentsUserData(*web_contents), diff --git a/browser/cosmetic_filters/cosmetic_filters_tab_helper.h b/browser/cosmetic_filters/cosmetic_filters_tab_helper.h index 5ac97f52d845..9b5e91d34881 100644 --- a/browser/cosmetic_filters/cosmetic_filters_tab_helper.h +++ b/browser/cosmetic_filters/cosmetic_filters_tab_helper.h @@ -34,7 +34,6 @@ class CosmeticFiltersTabHelper void ManageCustomFilters() override; void GetElementPickerThemeInfo( GetElementPickerThemeInfoCallback callback) override; - void InitElementPicker(InitElementPickerCallback callback) override; friend class content::WebContentsUserData; diff --git a/chromium_src/chrome/browser/flags/android/chrome_feature_list.cc b/chromium_src/chrome/browser/flags/android/chrome_feature_list.cc index 41dc80b8c58e..09b2d9be3680 100644 --- a/chromium_src/chrome/browser/flags/android/chrome_feature_list.cc +++ b/chromium_src/chrome/browser/flags/android/chrome_feature_list.cc @@ -46,7 +46,7 @@ &brave_shields::features::kBraveShowStrictFingerprintingMode, \ &brave_shields::features::kBraveLocalhostAccessPermission, \ &brave_shields::features::kBlockAllCookiesToggle, \ - &brave_shields::features::kBlockElementFeature + &brave_shields::features::kBraveShieldsElementPicker // clang-format on diff --git a/chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc b/chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc index e8b40a670b17..cd52c7117673 100644 --- a/chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc +++ b/chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc @@ -717,7 +717,7 @@ void BraveRenderViewContextMenu::AppendDeveloperItems() { const auto page_url = source_web_contents_->GetLastCommittedURL(); add_block_elements &= page_url.SchemeIsHTTPOrHTTPS(); add_block_elements &= base::FeatureList::IsEnabled( - brave_shields::features::kBlockElementFeature); + brave_shields::features::kBraveShieldsElementPicker); if (add_block_elements) { std::optional inspect_index = menu_model_.GetIndexOfCommandId(IDC_CONTENT_CONTEXT_INSPECTELEMENT); diff --git a/components/brave_shields/core/common/BUILD.gn b/components/brave_shields/core/common/BUILD.gn index 125a78da44f3..12bfd585dfdc 100644 --- a/components/brave_shields/core/common/BUILD.gn +++ b/components/brave_shields/core/common/BUILD.gn @@ -19,10 +19,7 @@ static_library("common") { sources += [ "brave_shield_localized_strings.h" ] } - public_deps = [ - "//brave/components/brave_shields/core/common/buildflags", - "//brave/components/resources:strings", - ] + public_deps = [ "//brave/components/resources:strings" ] deps = [ "//base", diff --git a/components/brave_shields/core/common/buildflags/BUILD.gn b/components/brave_shields/core/common/buildflags/BUILD.gn deleted file mode 100644 index c3fbf9179842..000000000000 --- a/components/brave_shields/core/common/buildflags/BUILD.gn +++ /dev/null @@ -1,12 +0,0 @@ -# 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/. - -import("//brave/components/brave_shields/core/common/buildflags/buildflags.gni") -import("//build/buildflag_header.gni") - -buildflag_header("buildflags") { - header = "buildflags.h" - flags = [ "ENABLE_BLOCK_ELEMENT_FEATURE=$enable_block_elements_feature" ] -} diff --git a/components/brave_shields/core/common/buildflags/buildflags.gni b/components/brave_shields/core/common/buildflags/buildflags.gni deleted file mode 100644 index 6e70421f9730..000000000000 --- a/components/brave_shields/core/common/buildflags/buildflags.gni +++ /dev/null @@ -1,12 +0,0 @@ -# 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/. - -import("//brave/build/config.gni") - -declare_args() { - enable_block_elements_feature = - (brave_channel == "development" || brave_channel == "nightly" || - brave_channel == "beta" || (is_official_build && !is_android)) && !is_ios -} diff --git a/components/brave_shields/core/common/features.cc b/components/brave_shields/core/common/features.cc index f5cdcb733df1..b8e73812d2ca 100644 --- a/components/brave_shields/core/common/features.cc +++ b/components/brave_shields/core/common/features.cc @@ -6,7 +6,6 @@ #include "brave/components/brave_shields/core/common/features.h" #include "base/feature_list.h" -#include "brave/components/brave_shields/core/common/buildflags/buildflags.h" namespace brave_shields::features { @@ -135,12 +134,12 @@ BASE_FEATURE(kBlockAllCookiesToggle, "BlockAllCookiesToggle", base::FEATURE_DISABLED_BY_DEFAULT); // when enabled, allow to select and block HTML elements -BASE_FEATURE(kBlockElementFeature, - "BlockElementFeature", -#if BUILDFLAG(ENABLE_BLOCK_ELEMENT_FEATURE) - base::FEATURE_ENABLED_BY_DEFAULT); -#else +BASE_FEATURE(kBraveShieldsElementPicker, + "BraveShieldsElementPicker", +#if BUILDFLAG(IS_ANDROID) base::FEATURE_DISABLED_BY_DEFAULT); +#else + base::FEATURE_ENABLED_BY_DEFAULT); #endif // Enables extra TRACE_EVENTs in content filter js. The feature is diff --git a/components/brave_shields/core/common/features.h b/components/brave_shields/core/common/features.h index 10bc7d8c7ced..f147d9f0f148 100644 --- a/components/brave_shields/core/common/features.h +++ b/components/brave_shields/core/common/features.h @@ -41,7 +41,7 @@ BASE_DECLARE_FEATURE(kCosmeticFilteringJsPerformance); BASE_DECLARE_FEATURE(kCosmeticFilteringSyncLoad); BASE_DECLARE_FEATURE(kBlockAllCookiesToggle); BASE_DECLARE_FEATURE(kCosmeticFilteringCustomScriptlets); -BASE_DECLARE_FEATURE(kBlockElementFeature); +BASE_DECLARE_FEATURE(kBraveShieldsElementPicker); extern const base::FeatureParam kComponentUpdateCheckIntervalMins; extern const base::FeatureParam kCosmeticFilteringSubFrameFirstSelectorsPollingDelayMs; diff --git a/components/cosmetic_filters/common/cosmetic_filters.mojom b/components/cosmetic_filters/common/cosmetic_filters.mojom index 2221503e0806..e9e113cf5557 100644 --- a/components/cosmetic_filters/common/cosmetic_filters.mojom +++ b/components/cosmetic_filters/common/cosmetic_filters.mojom @@ -17,11 +17,6 @@ interface CosmeticFiltersResources { mojo_base.mojom.Value result); }; -enum RunningPlatform { - kDesktop, - kAndroid -}; - interface CosmeticFiltersHandler { // Adds a user cosmetic rule for the current site. // (currently from the content picker feature). @@ -32,8 +27,6 @@ interface CosmeticFiltersHandler { GetElementPickerThemeInfo() => ( bool is_nightly_mode_enabled, int32 background_color); - - InitElementPicker() => (RunningPlatform platform); }; // An interface to render frame agent `CosmeticFiltersJSHandler` to control diff --git a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc index e8f3e74fb8ee..8247b9d618d5 100644 --- a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc +++ b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc @@ -33,6 +33,7 @@ #include "third_party/blink/public/web/web_local_frame.h" #include "third_party/blink/public/web/web_script_source.h" #include "ui/base/resource/resource_bundle.h" +#include "v8/include/v8-primitive.h" #include "v8/include/v8.h" namespace { @@ -362,45 +363,15 @@ void CosmeticFiltersJSHandler::OnGetCosmeticFilterThemeInfo( std::ignore = resolver->Resolve(context, result); } -v8::Local CosmeticFiltersJSHandler::InitElementPicker( +v8::Local CosmeticFiltersJSHandler::GetPlatform( v8::Isolate* isolate) { - v8::MaybeLocal resolver = - v8::Promise::Resolver::New(isolate->GetCurrentContext()); - - if (!resolver.IsEmpty()) { - auto promise_resolver = - std::make_unique>(); - promise_resolver->Reset(isolate, resolver.ToLocalChecked()); - auto context_old = std::make_unique>( - isolate, isolate->GetCurrentContext()); - - GetElementPickerRemoteHandler()->InitElementPicker(base::BindOnce( - &CosmeticFiltersJSHandler::OnInitElementPicker, - weak_ptr_factory_.GetWeakPtr(), std::move(promise_resolver), isolate, - std::move(context_old))); - - return resolver.ToLocalChecked()->GetPromise(); - } - - return v8::Local(); -} - -void CosmeticFiltersJSHandler::OnInitElementPicker( - std::unique_ptr> promise_resolver, - v8::Isolate* isolate, - std::unique_ptr> context_old, - mojom::RunningPlatform platform) { - v8::HandleScope handle_scope(isolate); - v8::Local context = context_old->Get(isolate); - v8::Context::Scope context_scope(context); - v8::MicrotasksScope microtasks(isolate, context->GetMicrotaskQueue(), - v8::MicrotasksScope::kDoNotRunMicrotasks); - - v8::Local resolver = promise_resolver->Get(isolate); - v8::Local result; - result = v8::Integer::New(isolate, static_cast(platform)); - - std::ignore = resolver->Resolve(context, result); + return gin::StringToV8(isolate, +#if BUILDFLAG(IS_ANDROID) + "android" +#else // !BUILDFLAG(IS_ANDROID) + "desktop" +#endif // !BUILDFLAG(IS_ANDROID) + ); } void CosmeticFiltersJSHandler::AddJavaScriptObjectToFrame( @@ -482,8 +453,8 @@ void CosmeticFiltersJSHandler::BindFunctionsToObject( base::Unretained(this))); BindFunctionToObject( - isolate, javascript_object, "initElementPicker", - base::BindRepeating(&CosmeticFiltersJSHandler::InitElementPicker, + isolate, javascript_object, "getPlatform", + base::BindRepeating(&CosmeticFiltersJSHandler::GetPlatform, base::Unretained(this), isolate)); if (perf_tracker_) { diff --git a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h index f84725535516..0e136cbd5420 100644 --- a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h +++ b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h @@ -80,12 +80,7 @@ class CosmeticFiltersJSHandler : public mojom::CosmeticFiltersAgent { bool OnIsFirstParty(const std::string& url_string); void OnAddSiteCosmeticFilter(const std::string& selector); void OnManageCustomFilters(); - v8::Local InitElementPicker(v8::Isolate* isolate); - void OnInitElementPicker( - std::unique_ptr> promise_resolver, - v8::Isolate* isolate, - std::unique_ptr> context_old, - mojom::RunningPlatform platform); + v8::Local GetPlatform(v8::Isolate* isolate); v8::Local GetCosmeticFilterThemeInfo(v8::Isolate* isolate); void OnGetCosmeticFilterThemeInfo( std::unique_ptr> promise_resolver, diff --git a/components/cosmetic_filters/resources/data/element_picker.ts b/components/cosmetic_filters/resources/data/element_picker.ts index eeb203e79633..01a2ca34ce98 100644 --- a/components/cosmetic_filters/resources/data/element_picker.ts +++ b/components/cosmetic_filters/resources/data/element_picker.ts @@ -22,8 +22,8 @@ const api = { callback(val.isDarkModeEnabled, val.bgcolor) }) }, - initElementPicker: (callback: (platform: number) => void) => { - cf_worker.initElementPicker().then(callback) + getPlatform: ():string => { + return cf_worker.getPlatform() } } @@ -846,8 +846,6 @@ const highlightElements = () => { const active = document.getElementById('brave-element-picker') if (!active) { - api.initElementPicker((platform: number) => { - isAndroid = platform === 1 - launchElementPicker(attachElementPicker()) - }) + isAndroid = api.getPlatform() === 'android' + launchElementPicker(attachElementPicker()) } diff --git a/components/definitions/chromel.d.ts b/components/definitions/chromel.d.ts index 2f5e9c227be1..b947c37ac567 100644 --- a/components/definitions/chromel.d.ts +++ b/components/definitions/chromel.d.ts @@ -226,7 +226,7 @@ declare namespace cf_worker { const manageCustomFilters: () => void const getElementPickerThemeInfo: () => Promise<{isDarkModeEnabled: boolean; bgcolor: number}> - const initElementPicker: () => Promise + const getPlatform: () => string } declare namespace chrome.test {