Additional Shields menu item to show all blocked elements(same as reset) #27910

vadimstruts · 2025-03-03T09:24:05Z

Implemented additional menu item to the shields popup dialog. It appears just in case the current page has custom filters related to the element blocker feature (i.e. some blocked elements)
Blocked elements count is not available yet, as needs adblock engine modifications.

Desktop:

2025-03-03.11-11-35.mp4

Android:

2025-03-03.10-15-46.mp4

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

brave-builds · 2025-03-03T10:21:11Z

A Storybook has been deployed to preview UI for the latest push

mkarolin

strings++

deeppandya

a couple of minor changes. the rest LGTM

deeppandya · 2025-03-04T16:22:44Z

android/java/res/layout/brave_shields_third_layout.xml

@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright (c) 2020 The Brave Authors. All rights reserved.


nit : year should be 2025

deeppandya · 2025-03-04T16:23:31Z

android/java/res/layout/brave_shields_third_layout.xml

+        android:layout_height="0dp"
+        android:layout_weight="1"
+        android:paddingHorizontal="16dp"
+        android:paddingTop="16dp"


nit : we can use paddingVertical here as paddingTop and paddingBottom have the same values.

Signed-off-by: Vadym Struts <[email protected]>

github-actions · 2025-03-11T12:42:11Z

browser/cosmetic_filters/cosmetic_filters_tab_helper.cc

-  GetWebContents().GetController().Reload(content::ReloadType::NORMAL, true);
+void CosmeticFiltersTabHelper::ManageCustomFilters() {
+#if !BUILDFLAG(IS_ANDROID)
+  Browser* browser = chrome::FindBrowserWithTab(&GetWebContents());


_{reported by reviewdog 🐶}
[semgrep] There are several global functions that facilitate dependency inversion. It will not be possible to call them from modularized features (no dependency cycles), and their usage in non-modularized features is considered a red flag

Don't use Browser*. This is functionally a container of hundreds of other pointers. It is impossible to specify dependencies, since Browser* functionally depends on everything. Instead, pass in the relevant pointers, e.g. Profile*, FooFeatureController, etc

References:
- https://chromium.googlesource.com/chromium/src/+/main/docs/chrome_browser_design_principles.md#structure_modularity

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/browser-dependency-inversion.yaml

Cc @goodov @cdesouza-chromium @bridiver

cc @bridiver I think the wording can be improved to mention valid use cases when we use Browser* as a "window" object, i.e. we have some functionality bound to the browser window. We shouldn't convert such scenarios to Profile* as it will lose the connection to the exact window.

But we're not supposed to have functionality tied to Browser anymore. This wording is pulled directly from https://source.chromium.org/chromium/chromium/src/+/main:docs/chrome_browser_design_principles.md;l=1?q=design_principles.&ss=chromium

I think in particular it's bad to have a dependency on Browser here in the tab helper. I'm on my phone right now so I can't see what this helper is doing internally, but at the very least we should pass the webcontents and keep the Browser dependency inside the helper. It's possible we should not be using Browser here at all, but I can't say for sure until I get home and see the details

Also FindBrowserWithTab should have been flagged here. Does it only catch one per line @thypon ?

this appears to be the only usage of ShowBraveAdblock so we should change it to use

void ShowSingletonTabOverwritingNTP( Profile* profile, const GURL& url, NavigateParams::PathBehavior path_behavior)

instead of the Browser version

this appears to be the only usage of ShowBraveAdblock so we should change it to use

void ShowSingletonTabOverwritingNTP( Profile* profile, const GURL& url, NavigateParams::PathBehavior path_behavior)

instead of the Browser version

It doesn't make sense to create a tab using a Profile* instance when you're acting from a specific tab that is bound to a specific Browser* instance.

I think technically we should follow this example here:

// Do not do this: FooTabFeature { // As per (1) above, we shouldn't be passing or storing Browser* anyways. // Another common anti-pattern is to dynamically look up Browser* via // browser_finder methods. These often result in the wrong Browser* Browser* browser_; // This will point to the wrong BrowserView if the tab is dragged into a new // window. Furthermore, BrowserView* itself is a container of hundreds of // other views. The tab-scoped feature typically wants something much more // specific. BrowserView* browser_view_; // Extensions are profile-scoped, and thus any state/logic should be in a // ProfileKeyedService. If the user has multiple tabs (possibly across // multiple windows) simultaneously interacting with FooTabFeature, then it's // likely that the extension will uninstall while it's still in use. void InstallExtension(); void UninstallExtension(); }; // Instead do this: FooTabFeature { // Guaranteed to remain valid for the lifetime of this class. This can be used // to dynamically access relevant window state via // tab_->GetBrowserWindowInterface()->GetFeatures().GetFooWindowFeature() TabInterface* tab_; };

So ShowSingletonTabOverwritingNTP should be a part of GetBrowserWindowInterface or something like that.

If we don't want to change the function signature atm we can use smth like:
TabInterface::MaybeGetFromContents(contents)->GetBrowserWindowInterface()->GetBrowserForMigrationOnly()
note: don't forget to check pointers on every step.

In context of mentioned approach we could change the function ShowBraveAdblock to look like:

void ShowBraveAdblock(BrowserWindowInterface* browser_window_interface) { if(!browser_window_interface) { return; } content::OpenURLParams params(GURL(kBraveUIAdblockURL)), content::Referrer(), WindowOpenDisposition::SINGLETON_TAB, ui::PAGE_TRANSITION_AUTO_TOPLEVEL, false); browser_window_interface->OpenURL(params, {}); }

It does the same like the current one, doesn't require Browser dependency, and doesn't use the "GetBrowserForMigrationOnly"
WDYT?
@goodov, @bridiver, @boocmp

This looks like a separate task to refactor these functions all at once.

Signed-off-by: Vadym Struts <[email protected]>

goodov · 2025-03-12T06:51:01Z

browser/ui/brave_pages.cc

@@ -24,8 +24,8 @@ void ShowBraveRewards(Browser* browser) {
  ShowSingletonTabOverwritingNTP(browser, GURL(kRewardsPageURL));
 }

-void ShowBraveAdblock(Browser* browser) {
-  ShowSingletonTabOverwritingNTP(browser, GURL(kBraveUIAdblockURL));
+void ShowBraveAdblock(Profile* profile) {


@bridiver all these functions use Browser* to display the requested page in the exact browser window, NOT in the last active window. Passing Profile* here will switch those function to look for the last active browser window and load the requested page there instead of the explicit browser window the caller asks for.

Signed-off-by: Vadym Struts <[email protected]>

brave-builds · 2025-03-13T16:04:51Z

Released in v1.78.41

vadimstruts linked an issue Mar 3, 2025 that may be closed by this pull request

Additional Shields menu item to show all blocked elements(same as reset) brave/brave-browser#43955

Closed

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Mar 3, 2025

github-actions bot assigned vadimstruts Mar 3, 2025

vadimstruts marked this pull request as ready for review March 3, 2025 20:17

vadimstruts requested review from a team and deeppandya as code owners March 3, 2025 20:17

mkarolin approved these changes Mar 3, 2025

View reviewed changes

deeppandya approved these changes Mar 4, 2025

View reviewed changes

goodov approved these changes Mar 5, 2025

View reviewed changes

iefremov approved these changes Mar 5, 2025

View reviewed changes

vadimstruts added 5 commits March 11, 2025 11:08

started to add new button for desktop

0b69fc0

Signed-off-by: Vadym Struts <[email protected]>

started to add reset shields item

cce3f40

Signed-off-by: Vadym Struts <[email protected]>

implemented additional menu for reset all blocked elements for page

d3cd4f8

Signed-off-by: Vadym Struts <[email protected]>

format

537c82e

Signed-off-by: Vadym Struts <[email protected]>

format + fix presubmit warning

726797e

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts force-pushed the 43955-additional-shields-menu-item-to-show-all-blocked-elementssame-as-reset branch from e6296be to e820257 Compare March 11, 2025 10:58

github-actions bot assigned bridiver, cdesouza-chromium and goodov Mar 11, 2025

vadimstruts force-pushed the 43955-additional-shields-menu-item-to-show-all-blocked-elementssame-as-reset branch from e820257 to 027d1ed Compare March 11, 2025 12:20

fix according code review suggestions

aa071c1

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts force-pushed the 43955-additional-shields-menu-item-to-show-all-blocked-elementssame-as-reset branch from 027d1ed to aa071c1 Compare March 11, 2025 12:37

github-actions bot reviewed Mar 11, 2025

View reviewed changes

removed browser dependency

cfda786

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts requested a review from a team as a code owner March 11, 2025 18:40

vadimstruts added 3 commits March 11, 2025 23:31

applied code review suggestions

64d7d24

Signed-off-by: Vadym Struts <[email protected]>

fix according code review suggestions

2c63ea3

Signed-off-by: Vadym Struts <[email protected]>

format

4c99952

Signed-off-by: Vadym Struts <[email protected]>

goodov reviewed Mar 12, 2025

View reviewed changes

format

86c7a20

Signed-off-by: Vadym Struts <[email protected]>

boocmp approved these changes Mar 13, 2025

View reviewed changes

vadimstruts merged commit b3e162d into master Mar 13, 2025
18 checks passed

vadimstruts deleted the 43955-additional-shields-menu-item-to-show-all-blocked-elementssame-as-reset branch March 13, 2025 14:55

github-actions bot added this to the 1.78.x - Nightly milestone Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional Shields menu item to show all blocked elements(same as reset) #27910

Additional Shields menu item to show all blocked elements(same as reset) #27910

vadimstruts commented Mar 3, 2025 •

edited

Loading

brave-builds commented Mar 3, 2025

mkarolin left a comment

deeppandya left a comment

deeppandya Mar 4, 2025

deeppandya Mar 4, 2025

github-actions bot Mar 11, 2025

goodov Mar 11, 2025

bridiver Mar 11, 2025

bridiver Mar 11, 2025

bridiver Mar 11, 2025 •

edited

Loading

bridiver Mar 11, 2025 •

edited

Loading

goodov Mar 12, 2025 •

edited

Loading

boocmp Mar 12, 2025

vadimstruts Mar 12, 2025

goodov Mar 13, 2025

goodov Mar 12, 2025

brave-builds commented Mar 13, 2025

		@@ -0,0 +1,35 @@
		<?xml version="1.0" encoding="utf-8"?>
		<!-- Copyright (c) 2020 The Brave Authors. All rights reserved.

Additional Shields menu item to show all blocked elements(same as reset) #27910

Additional Shields menu item to show all blocked elements(same as reset) #27910

Conversation

vadimstruts commented Mar 3, 2025 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Mar 3, 2025

mkarolin left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

bridiver Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

goodov Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Mar 13, 2025

vadimstruts commented Mar 3, 2025 •

edited

Loading

bridiver Mar 11, 2025 •

edited

Loading

bridiver Mar 11, 2025 •

edited

Loading

goodov Mar 12, 2025 •

edited

Loading