Content picker native inject #25411

atuchin-m · 2024-09-03T13:54:03Z

Resolves brave/brave-browser#40821
For brave/brave-browser#40994

UPD: the PR is updated to match the new product requirements. The brave/browser code to communicated with frames moved to a dedicated tab helper created on demand.

The PR:

replaces an extension-based picker context menu to a native one;
drops using Brave Extension background page to inject the bundle. Now it's done via: context menu => tab_helper =(mojo)=> a frame agent(CosmeticFiltersJSHandler) => execute JS in a isolated world. The same world as for cosmetic filters is used.
provides a native function instead of chrome.braveShields to add a new rule. The pipeline: JS => the frame agent =(mojo)=> CosmeticFiltersResources(task_runner) => AdblockService(task_runner) => AdblockService(UI)
adds browser tests to cover these pipelines + opening brave://settings/shields/filters.
makes the context menu is enabled iif blocking ads&trackers is on. Otherwise it can't efficiently hide elements.
chrome.braveShields impl are removed.

A product-related questions:
brave/brave-browser#40821 (comment)

before	after

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/brave-browser#40821 (comment)

brave-builds · 2024-09-03T15:09:23Z

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

brave-builds · 2024-09-03T15:13:39Z

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

brave-builds · 2024-09-03T15:46:27Z

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

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

app/brave_generated_resources.grd

antonok-edm · 2024-09-03T20:33:05Z

app/brave_generated_resources.grd

+ <message name="IDS_ADBLOCK_CONTEXT_BLOCK_ELEMENT" desc="Message for context menu that enables the element picker UI to apply cosmetic filter rules to the page (was 'elementPickerMode' from Brave Extension)">
+ Block element
+ </message>
+ <message name="IDS_ADBLOCK_CONTEXT_MANAGE_CUSTOM_FILTERS" desc="Message for context menu that opens a page for adding, editing, and removing custom cosmetic filters (was 'manageCustomFilters' from Brave Extension)">
+ Manage custom filters


The parenthesized parts probably aren't relevant to translators

Ok, I will remove it.
My intention was to reuse the current locale keys in the brave extension.
I suppose we can do it another way by tagging a right person.

antonok-edm · 2024-09-03T20:54:15Z

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

+ menu_model_.InsertSubMenuWithStringIdAt(
+ *print_index + 1, IDC_ADBLOCK_CONTEXT_TOOLS, IDS_ADBLOCK_CONTEXT_TOOLS,
+ &adblock_submenu_model_);


Have you tried SetIcon and/or SetMinorIcon here? I assume one of those should work if we want to restore the Brave logo

I haven't tried because normally the menu has icons only in the extension block.
No problem to restore it if we really need.

mkarolin

strings++

petemill

Asked in DM about the benefits of RenderFrameHostReceiverSet over AssociatedReceiverSet but assume it's ok. JS / HTML changes look good.

petemill · 2024-09-11T21:33:56Z

components/cosmetic_filters/resources/data/BUILD.gn

@@ -1,12 +1,23 @@
+# Copyright (c) 2020 The Brave Authors. All rights reserved.


It's a bit strange that these files are in resources/data since it's not data but is executed scripts / UI. How about resources/ui?

Your point makes sense.
But that dir also has content_cosmetic.ts that don't have UI and also injected by cosmetic_filters_js_handler.
So making a separate dir and BUILD.gn make the code more sophisticated without extra benefits.

atuchin-m · 2024-09-12T08:21:38Z

About RenderFrameHostReceiverSet:
AssociatedReceiverSet is a basic container, you have to add/remove receivers by yourself.
RenderFrameHostReceiverSet uses AssociatedReceiverSet internally. It uses RFH as a key. It also has a logic to remove the (key,value) for deleted frames.

P.S. A single (Associated)Receiver also could be used for that particular task. But adding more methods in the interface can result in issues: a one tab helper matches to multiple RFH (even the main ones).

kjozwiak · 2024-09-23T15:05:47Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.72.31 Chromium: 129.0.6668.59 (Official Build) nightly (64-bit)
-- | --
Revision | 1b4e6c1e2eec55b17718e8a1b44845afc773c20c
OS | Windows 11 Version 23H2 (Build 22631.4169)

Using the STR/Cases outlined via brave/brave-browser#40821 (comment), went through the following:

`Test Case #1` - General & Manage Filters

ensured that clicking on Block elements initiates the element picker on the particular page you're currently viewing
ensured that clicking on an element on the page correctly writes/displays the rule without any issues
ensured that clicking on Create writes the rule into brave://settings/shields/filters without any issues
ensured that clicking on Manage opens brave://settings/shields/filters without any issues

`Example`	`Example`	`Example`	`Example`

`Test Case #1` - Menu Positions/Block Trackers disabled

ensured that Block element is always above Inspect via the context menu
ensured that Block element is not being displayed when viewing the context menu when selecting text
ensured that Block element is being displayed via the context menu for links
ensured that Block element is not being displayed for NTP & brave:// pages
ensured that Block element is not being displayed when Allow all trackers & ads
- ensured that Block element is being displayed when Block trackers & ads & Aggressive is selected

`Example`	`Example`	`Example`	`Example`

`Example`	`Example`	`Example`

Uplift of #25411 (squashed) to beta

atuchin-m self-assigned this Sep 3, 2024

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

atuchin-m mentioned this pull request Sep 3, 2024

[Perf] Make Brave Extension disabled by default to optimize memory usage brave/brave-browser#39610

Open

atuchin-m marked this pull request as ready for review September 3, 2024 19:07

atuchin-m requested review from a team as code owners September 3, 2024 19:07

atuchin-m requested review from antonok-edm, petemill and iefremov September 3, 2024 19:07

github-actions bot added the puLL-Merge label Sep 3, 2024

github-actions bot reviewed Sep 3, 2024

View reviewed changes

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Sep 3, 2024

github-actions bot assigned bridiver, diracdeltas, goodov, iefremov and thypon Sep 3, 2024

antonok-edm reviewed Sep 3, 2024

View reviewed changes

atuchin-m changed the title ~~[Draft] Content picker native inject~~ Content picker native inject Sep 4, 2024

atuchin-m mentioned this pull request Sep 4, 2024

A native menu and a native inject for adblock content picker brave/brave-browser#40821

Closed

atuchin-m force-pushed the content-picker-native-inject branch 3 times, most recently from 124a67f to 78de4da Compare September 4, 2024 13:12

atuchin-m mentioned this pull request Sep 4, 2024

Make brave extension background page conditional #25425

Merged

24 tasks

atuchin-m force-pushed the content-picker-native-inject branch from 78de4da to a719d4b Compare September 4, 2024 16:28

atuchin-m added 17 commits September 11, 2024 17:24

Move the menu

674eec8

update grd

a709fd0

Fix TODO & presubmit

0eac315

Fix and add tests

a713343

fix the last TODO

0909dcb

minor fixes

6b4e896

fix compilation

6213856

Fix grd strings

3d1da35

Remove cosmeticFilterEvents_test.ts

b7e59b5

Disable the tests for android

5789362

Fix strings

4642c6f

Fix menu position

425c9a1

disable render_view_context_menu_test_util.h for Android

303a6fc

Rework the menu, add a tab helper.

9be474e

Add SanitizeSelector()

c7c0f9c

Fix android compilation

a12ca31

Move the menu, fix review issues

00663c9

atuchin-m force-pushed the content-picker-native-inject branch from 8ed38da to 00663c9 Compare September 11, 2024 10:24

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Sep 11, 2024

mkarolin approved these changes Sep 11, 2024

View reviewed changes

fmarier approved these changes Sep 11, 2024

View reviewed changes

petemill approved these changes Sep 11, 2024

View reviewed changes

atuchin-m merged commit 76a5a49 into master Sep 12, 2024
18 of 19 checks passed

atuchin-m deleted the content-picker-native-inject branch September 12, 2024 08:24

github-actions bot added this to the 1.72.x - Nightly milestone Sep 12, 2024

brave-builds added a commit that referenced this pull request Sep 16, 2024

Uplift of #25411 (squashed) to beta

5d3edde

brave-builds mentioned this pull request Sep 16, 2024

Content picker native inject (uplift to 1.71.x) #25574

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Sep 23, 2024

Content picker native inject (uplift to 1.71.x) (#25574)

f1a3909

Uplift of #25411 (squashed) to beta

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content picker native inject #25411

Content picker native inject #25411

atuchin-m commented Sep 3, 2024 •

edited

Loading

brave-builds commented Sep 3, 2024

brave-builds commented Sep 3, 2024

brave-builds commented Sep 3, 2024

antonok-edm Sep 3, 2024

atuchin-m Sep 4, 2024

antonok-edm Sep 3, 2024

atuchin-m Sep 4, 2024

mkarolin left a comment

petemill left a comment

petemill Sep 11, 2024

atuchin-m Sep 12, 2024

atuchin-m commented Sep 12, 2024

kjozwiak commented Sep 23, 2024 •

edited

Loading

		@@ -1,12 +1,23 @@
		# Copyright (c) 2020 The Brave Authors. All rights reserved.

Content picker native inject #25411

Content picker native inject #25411

Conversation

atuchin-m commented Sep 3, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Sep 3, 2024

brave-builds commented Sep 3, 2024

brave-builds commented Sep 3, 2024

antonok-edm Sep 3, 2024

Choose a reason for hiding this comment

atuchin-m Sep 4, 2024

Choose a reason for hiding this comment

antonok-edm Sep 3, 2024

Choose a reason for hiding this comment

atuchin-m Sep 4, 2024

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

petemill Sep 11, 2024

Choose a reason for hiding this comment

atuchin-m Sep 12, 2024

Choose a reason for hiding this comment

atuchin-m commented Sep 12, 2024

kjozwiak commented Sep 23, 2024 • edited Loading

Test Case #1 - General & Manage Filters

Test Case #1 - Menu Positions/Block Trackers disabled

atuchin-m commented Sep 3, 2024 •

edited

Loading

kjozwiak commented Sep 23, 2024 •

edited

Loading

`Test Case #1` - General & Manage Filters

`Test Case #1` - Menu Positions/Block Trackers disabled