cosmetic filters for Android and desktop #6735

SergeyZhukovsky · 2020-09-30T17:44:54Z

Resolves brave/brave-browser#13070

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

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.

bridiver · 2020-09-30T20:04:40Z

browser/android/brave_cosmetic_resources_tab_helper.cc

@@ -24,6 +24,109 @@


 namespace {
+const char k_observing_script[] =


I think we'll probably want a transpile_web_ui target for just content_cosmetic.ts and put the result in a grd file to isolate it from everything else in brave_extension

it's done via transpile now

renderer/brave_content_renderer_client.cc

browser/android/brave_cosmetic_resources_tab_helper.cc

components/brave_extension/extension/brave_extension/background/api/cosmeticFilterAPI.ts

iefremov · 2020-12-10T09:34:08Z

@SergeyZhukovsky is it ready for review?

SergeyZhukovsky · 2020-12-10T12:55:26Z

@SergeyZhukovsky is it ready for review?

@iefremov yes, basically. 3 tests are failing, I'm replacing them as they use old functionality that is not there anymore. Other than that I'm happy to hear a feedback.

renderer/brave_content_renderer_client.cc

browser/extensions/api/brave_shields_api.cc

browser/brave_cosmetic_resources_tab_helper.cc

petemill · 2020-12-14T23:03:03Z

components/brave_extension/extension/brave_extension/background/api/cosmeticFilterAPI.ts

@@ -0,0 +1,51 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


can we squash this commit with the commit that removes the file? Otherwise we'll lose file history.

bridiver · 2020-12-15T19:59:38Z

patches/content-public-browser-render_frame_host.h.patch

+       const base::android::JavaRef<jobject>& jrender_frame_host_android);
+ #endif
+
+-  ~RenderFrameHost() override {}


I don't understand why this patch is here or the one for the constructor. You don't have to initialize a unique_ptr to null

there is a build error otherwise that class with complex members like added std::unique_ptr should have constructor and destructor implementations in cc file

all patches are removed

bridiver · 2020-12-15T20:20:37Z

browser/extensions/api/brave_shields_api.cc

@@ -66,31 +66,31 @@ BraveShieldsUrlCosmeticResourcesFunction::Run() {

 std::unique_ptr<base::ListValue> BraveShieldsUrlCosmeticResourcesFunction::
    GetUrlCosmeticResourcesOnTaskRunner(const std::string& url) {
-  base::Optional<base::Value> resources = g_brave_browser_process->
-      ad_block_service()->UrlCosmeticResources(url);
+  // base::Optional<base::Value> resources = g_brave_browser_process->


why are these all commented out?

as @antonok-edm pointed out above, we don't need that anymore as we want to get rid of the extension

bridiver · 2021-01-21T18:57:28Z

components/cosmetic_filters/renderer/cosmetic_filters_js_render_frame_observer.cc

+    content::RenderFrame* render_frame,
+    const int32_t isolated_world_id)
+    : RenderFrameObserver(render_frame),
+      worker_isolated_world_id_(isolated_world_id) {}


same as above worker_ doesn't really make sense here

bridiver · 2021-01-21T18:57:44Z

components/cosmetic_filters/renderer/cosmetic_filters_js_render_frame_observer.cc

+}
+
+void CosmeticFiltersJsRenderFrameObserver::DidCreateScriptContext(
+    v8::Local<v8::Context> context,


missing v8 header

it is in the corresponding header file

bridiver · 2021-01-21T18:58:08Z

components/cosmetic_filters/renderer/cosmetic_filters_js_render_frame_observer.h

+  // RenderFrameObserver implementation.
+  void DidStartNavigation(
+      const GURL& url,
+      base::Optional<blink::WebNavigationType> navigation_type) override;


missing base/optional and blink headers

iefremov · 2021-01-22T13:07:04Z

components/cosmetic_filters/browser/cosmetic_filters_resources.cc

+      base::BindOnce(HiddenClassIdSelectorsOnTaskRunner, ad_block_service_,
+                     classes, ids, exceptions),
+      base::BindOnce(&CosmeticFiltersResources::HiddenClassIdSelectorsOnUI,
+                     base::Unretained(this), std::move(callback)));


you need weak pointer here and below

iefremov · 2021-01-22T13:07:52Z

components/cosmetic_filters/browser/cosmetic_filters_resources.cc

+
+namespace {
+
+base::Optional<base::Value> HiddenClassIdSelectorsOnTaskRunner(


basically you could post brave_shields::AdBlockService::HiddenClassIdSelectors without writing these free functions

iefremov · 2021-01-22T13:10:50Z

components/cosmetic_filters/browser/cosmetic_filters_resources.h

+ public:
+  CosmeticFiltersResources(const CosmeticFiltersResources&) = delete;
+  CosmeticFiltersResources& operator=(const CosmeticFiltersResources&) = delete;
+  explicit CosmeticFiltersResources(


nit: not explicit

iefremov · 2021-01-22T13:11:16Z

components/cosmetic_filters/browser/cosmetic_filters_resources.h

@@ -0,0 +1,68 @@
+/* Copyright (c) 2020 The Brave Authors. All rights reserved.


iefremov · 2021-01-22T13:15:05Z

components/cosmetic_filters/browser/cosmetic_filters_resources.cc

+void CosmeticFiltersResources::UrlCosmeticResourcesOnUI(
+    UrlCosmeticResourcesCallback callback,
+    base::Optional<base::Value> resources) {
+  std::move(callback).Run(resources ? resources->Clone() : base::Value());


Clone() is expensive, why not just move?

iefremov · 2021-01-22T13:17:12Z

components/cosmetic_filters/browser/cosmetic_filters_resources.cc

+    const std::string& input,
+    const std::vector<std::string>& exceptions,
+    HiddenClassIdSelectorsCallback callback) {
+  base::Optional<base::Value> input_value = base::JSONReader::Read(input);


I'd rather do all this stuff in the function that works on task runner, not a blocker though

iefremov · 2021-01-22T13:36:21Z

components/cosmetic_filters/renderer/cosmetic_filters_js_render_frame_observer.cc

+void CosmeticFiltersJsRenderFrameObserver::DidStartNavigation(
+    const GURL& url,
+    base::Optional<blink::WebNavigationType> navigation_type) {
+  url_ = url;


not sure why do we need this method, can we just extract render_frame()->GetWebFrame()->GetDocument().Url() or something in DidCreateNewDocument() ?

Basically wondering whether DidStartNavigation() is called for redirects, otherwise we can cache a wrong url

it is called for redirects. This is basically done as DOM distiller does at the RenderFrameObserver, they get URL from this method as well
void DistillerJsRenderFrameObserver::DidStartNavigation(
const GURL& url,
base::Optional<blink::WebNavigationType> navigation_type) {
is_distiller_page_ = url_utils::IsDistilledPage(url);
}

iefremov · 2021-01-22T13:51:58Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h

+
+class CosmeticFiltersJSHandler {
+ public:
+  explicit CosmeticFiltersJSHandler(content::RenderFrame* render_frame,


nit - explicit is need only for constructors that have a single argument

iefremov · 2021-01-22T14:01:46Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

+  cosmetic_filters_resources_->ShouldDoCosmeticFiltering(
+      url_.spec(),
+      base::BindOnce(&CosmeticFiltersJSHandler::OnShouldDoCosmeticFiltering,
+                     base::Unretained(this)));


we need weak pointers, this object can die long before the callback is invoked, we are in a different process

that object owns the mojo receiver, the receiver will be dead if the object is dead and nothing will be invoked. Not needed WeakPtr in that case

CosmeticFilterResources posts to TaskRunner, so the Receiver and its owner could die before adblocker replies back?

I'm not following how adblocker is involved here. cosmetic_filters_resources_ is a member of CosmeticFiltersJSHandler and a mojo receiver, how the receiver can pass result back to it's owner when owner is dead. Receiver will also be dead as it's owner is.
CosmeticFiltersJSHandler owns the mojo remote if the mojo remote is destroyed then the callback will never happen.

Ah, sorry, you are right. I've reread the documentation and apparently we are just fine

iefremov · 2021-01-22T15:03:14Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

+        cosmetic_filters_resources_.BindNewPipeAndPassReceiver());
+  }
+
+  if (cosmetic_filters_resources_.is_bound())


return cosmetic_filters_resources_.is_bound();

iefremov · 2021-01-22T15:41:43Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

+                     base::Unretained(this)));
+}
+
+void CosmeticFiltersJSHandler::OnShouldDoCosmeticFiltering(


do we need this hop (aka OnShouldDoCosmeticFiltering)? I. e. can we just check !enabled on the browser side and immediately do UrlCosmeticResources, then callback to OnUrlCosmeticResources

we've discussed that with @bridiver and came to the conclusion to use it like that. The problem with the solution you propose is that we need one more boolean here enabled_1st_party_cf_filtering_ and we need to store it to use in CosmeticFiltersJSHandler calls. We could potentially return it with OnUrlCosmeticResources as well, but we've decided to use it like it is right now.

okay, nothing critical, but I think it worth doing at some point

antonok-edm · 2022-01-27T01:30:30Z

browser/brave_shields/ad_block_service_browsertest.cc

+                                            R"(function waitCSSSelector() {
+          if (checkSelector('#ad-banner', 'display', 'none')) {


In most of these tests, checkSelector returns a Promise (which evaluates to true in an if statement)... so these tests are not actually doing anything 😦

It should be like this:

(async function waitCSSSelector() { if (await checkSelector('#ad-banner', 'display', 'none')) { // ...

(Filed brave/brave-browser#20720)

antonok-edm · 2022-01-27T23:07:58Z

browser/brave_shields/ad_block_service_browsertest.cc

@@ -902,12 +938,29 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringDynamic) {
  content::WebContents* contents =
      browser()->tab_strip_model()->GetActiveWebContents();

-  ASSERT_EQ(true, EvalJs(contents,
-                         "addElementsDynamically();\n"


^ this addElementsDynamically() was also important 😅

bridiver reviewed Sep 30, 2020

View reviewed changes

bbondy reviewed Oct 5, 2020

View reviewed changes

renderer/brave_content_renderer_client.cc Show resolved Hide resolved

jumde reviewed Oct 5, 2020

View reviewed changes

browser/android/brave_cosmetic_resources_tab_helper.cc Outdated Show resolved Hide resolved

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch 2 times, most recently from 105f858 to 90ed5df Compare November 4, 2020 19:03

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch 2 times, most recently from fc5ba5f to 0282789 Compare November 10, 2020 18:17

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch from f92c1d7 to 327258c Compare November 24, 2020 15:18

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch from 2f0b9c0 to 660f733 Compare December 4, 2020 17:54

SergeyZhukovsky requested review from antonok-edm, iefremov, fmarier, petemill and pes10k December 7, 2020 15:29

SergeyZhukovsky self-assigned this Dec 7, 2020

SergeyZhukovsky added this to the 1.20.x - Nightly milestone Dec 7, 2020

SergeyZhukovsky marked this pull request as ready for review December 7, 2020 15:31

SergeyZhukovsky requested a review from a team as a code owner December 7, 2020 15:31

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch from b5da8b8 to 7561c49 Compare December 7, 2020 16:27

antonok-edm reviewed Dec 7, 2020

View reviewed changes

components/brave_extension/extension/brave_extension/background/api/cosmeticFilterAPI.ts Outdated Show resolved Hide resolved

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch 2 times, most recently from 81c8a58 to f5794c4 Compare December 9, 2020 16:08

antonok-edm reviewed Dec 14, 2020

View reviewed changes

petemill reviewed Dec 14, 2020

View reviewed changes

bridiver reviewed Dec 15, 2020

View reviewed changes

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch from 7bf3915 to 2f41ca1 Compare December 16, 2020 15:40

SergeyZhukovsky changed the title ~~cosmetic filters render frame observer~~ cosmetic filters for Android and desktop Dec 16, 2020

bridiver reviewed Jan 21, 2021

View reviewed changes

bridiver approved these changes Jan 21, 2021

View reviewed changes

addresses minor review feedback for cosmetic filters

e4df7f8

SergeyZhukovsky force-pushed the cometic_filters_css_rules branch from 19ff267 to e4df7f8 Compare January 21, 2021 20:08

iefremov reviewed Jan 22, 2021

View reviewed changes

iefremov approved these changes Jan 22, 2021

View reviewed changes

addresses some code reviews for cosmetic filters

058b71e

SergeyZhukovsky merged commit 88bfb17 into master Jan 22, 2021

SergeyZhukovsky deleted the cometic_filters_css_rules branch January 22, 2021 19:19

This was referenced Jan 25, 2021

[Desktop] Run cosmetic filtering on about:blank frames brave/brave-browser#10827

Closed

[Android] Add "Aggressive" ads/trackers blocking config option in Shields Panel brave/brave-browser#9831

Closed

bsclifton mentioned this pull request Feb 3, 2021

Brave Cosmetic filtering script blocked on website due to CSP restriction brave/brave-browser#13929

Closed

antonok-edm mentioned this pull request Feb 4, 2021

Pre-fetch adblock cosmetic resources data as early as possible #5992

Closed

32 tasks

SergeyZhukovsky modified the milestones: 1.20.x - Release, 1.21.x - Beta Feb 11, 2021

antonok-edm mentioned this pull request Mar 4, 2021

Add back application of legacy cosmetic filters #8126

Merged

24 tasks

antonok-edm reviewed Jan 27, 2022

View reviewed changes

		@@ -24,6 +24,109 @@


		namespace {
		const char k_observing_script[] =

		@@ -0,0 +1,51 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public


		namespace {

		base::Optional<base::Value> HiddenClassIdSelectorsOnTaskRunner(

		@@ -0,0 +1,68 @@
		/* Copyright (c) 2020 The Brave Authors. All rights reserved.

		R"(function waitCSSSelector() {
		if (checkSelector('#ad-banner', 'display', 'none')) {

cosmetic filters for Android and desktop #6735

cosmetic filters for Android and desktop #6735

Conversation

SergeyZhukovsky commented Sep 30, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Dec 10, 2020

SergeyZhukovsky commented Dec 10, 2020

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

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

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

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

SergeyZhukovsky Jan 22, 2021 • 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

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

Choose a reason for hiding this comment

antonok-edm Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 30, 2020 •

edited

Loading

SergeyZhukovsky Jan 22, 2021 •

edited

Loading

antonok-edm Jan 27, 2022 •

edited

Loading