Add support for checking de_amp_enabled user pref in cosmetic js filter #12923

ShivanKaul · 2022-04-06T20:59:53Z

This PR adds support for a new scriptlet that will remove jsaction attribute from anchor tags on SERPs, if DeAmp flag and pref is enabled. After this PR we will merge in brave/adblock-resources#63 (for the de-amp resource) and brave/adblock-lists#818 (for the rule)

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
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, npm run lint, 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:

Test Plan:

Given that the adblock component containing the DeAMP scriplet is already deployed, it's a lot easier to just verify that the scriplet is working as intended which it would only do if this PR is properly working.

To manually test:

Switch off DeAMP user preference in brave://settings/shields
Go to google.com. If on Desktop, you'll have to emulate mobile to get this to work - this can be done through DevTools (helpful link on how to do that in case unfamiliar). Choose a non-iPhone version - I typically use Samsung S8 or the like.
Search for "top stories". You can try out a few different queries but this one reliably gave me AMP results.
Click on a panel in the Top Stories marquee. Something like
Check that you get the AMP result. The URL would look something like https://www.google.com/amp/s/xyz. Also, inspect the link for the search result on google.com and check that the anchor tag has the "jsaction" attribute. It would look something like
Now, switch on DeAMP user pref.
Reload the google.com search page. Click the same link as before ^. It should give you the non-AMP version of the page i.e. something like https://www.wsj.com/livecoverage/xyz.
Also verify that the anchor tag does not have the "jsaction" attribute.

These steps can be repeated for google.co.uk, google.co.in, etc. - they should work on all google search TLDs.

ShivanKaul · 2022-04-06T21:03:02Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

@@ -41,12 +41,12 @@ const char kObservingScriptletEntryPoint[] =

 const char kScriptletInitScript[] =
    R"((function() {
-          let text = %s;
+          let text = 'let deAmpEnabled = %s;\n' + %s;


This is a bit ugly, but we can't just add let deAmpEnabled = %s; before this line because deAmpEnabled will then not be visible to the scriptlet when the script is actually executed as part of appendChild on line 50.

do we need

let text = '(function() {\nlet deAmpEnabled = %s;\n' + %s + '})()';

?

In the current form, deAmpEnabled just becomes part of the scriplet and will go out of scope once the scriptlet is done executing, right? Which is what we want.

The actual scriptlet is that second %s above. text becomes essentially <script>text</script>, so I think the deAmpEnabled needs its own IIF scope as well.

In other words, the code you have right now will currently inject something like this:

<script> let deAmpEnabled = true; try { (function() { // scriptlet 1 })() } catch (e) {} try { (function() { // scriptlet 2 })() } catch (e) {} </script>

which makes deAmpEnabled visible to other scripts, so we'd want something like this instead:

<script> (function() { let deAmpEnabled = true; try { (function() { // scriptlet 1 })() } catch (e) {} try { (function() { // scriptlet 2 })() } catch (e) {} })() </script>

Ah that's right, good catch. Updated

antonok-edm

Would be nice to have a browsertest - you can see an example of injecting a custom scriptlet in a test using AdBlockServiceTest.CosmeticFilteringWindowScriptlet.

Otherwise it looks good to me, but @bridiver should probably take a look as well

Add test

bridiver · 2022-04-13T17:45:25Z

components/cosmetic_filters/renderer/DEPS

@@ -1,4 +1,5 @@
 include_rules = [
+  "+brave/common",


this isn't allowed, it's a layering violation. Instead of passing in GetDynamicParamsCallback you can wrap it in a closure that just returns the value of de_amp_enabled

bridiver · 2022-04-13T17:55:00Z

browser/profiles/brave_renderer_updater.cc

@@ -102,7 +109,11 @@ void BraveRendererUpdater::UpdateRenderer(
      default_wallet ==
      brave_wallet::mojom::DefaultWallet::BraveWalletPreferExtension;

+  PrefService* pref_service = profile_->GetPrefs();
+  bool de_amp_enabled = pref_service->GetBoolean(de_amp::kDeAmpPrefEnabled);


can we make this the combined value of the pref plus the feature? And also create a utility method in the component that gives the result of both given the PrefService and also use that in DeAmpThrottle? Easy to make a mistake otherwise when checking.

But then this would rely on components/de_amp/browser. I thought we didn't want that, and hence the suggestion to just check using the PrefService.

no, it would rely on components/de_amp/common :)

sorry, actually this code wouldn't access it at all, the combined value would be sent to the renderer

kjozwiak · 2022-04-22T03:00:02Z

Verification PASSED on Samsung S10+ running Android 12 using the following build(s):

Brave | 1.40.3 Chromium: 101.0.4951.41 (Official Build) canary (32-bit)
--- | ---
Revision | 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS | Android 12; Build/SP1A.210812.016

Went through the STR/Cases outlined via #12923 (comment).

`de-AMP Disabled`

ensured that Auto-redirect AMP pages was disabled via brave://settings/privacy
loaded https://www.google.com and then searched for Top Stories
inspected the first article via DevTools and ensured that jsaction attribute appeared in the anchor link
clicked on the link and ensured that the AMP version of the website/news article was being loaded
- Example --> https://www.cbc.ca/amp/1.6425708

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

`de-AMP Enabled`

ensured that Auto-redirect AMP pages was enabled via brave://settings/privacy
loaded https://www.google.com and then searched for Top Stories
inspected the first article via DevTools and ensured that jsaction attribute wasn't appeared in the anchor link
clicked on the link and ensured that the de-AMP version of the website/news article was being loaded

`Example`	`Example`	`Example`

Quick note: I also tried https://www.google.ca & https://www.google.co.uk

Brave | 1.40.2 Chromium: 101.0.4951.41 (Official Build) nightly (64-bit)
-- | --
Revision | 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS | Windows 11 Version 21H2 (Build 22000.613)

Went through the STR/Cases outlined via #12923 (comment).

`de-AMP Disabled`

ensured that Auto-redirect AMP pages was disabled via brave://settings/privacy
loaded https://www.google.com and then searched for Top Stories
inspected the first article via DevTools and ensured that jsaction attribute appeared in the anchor link
clicked on the link and ensured that the AMP version of the website/news article was being loaded
- Example --> https://www.google.com/amp/s/www.cbc.ca/amp/1.6425708

`Example`	`Example`	`Example`

`de-AMP Enabled`

ensured that Auto-redirect AMP pages was enabled via brave://settings/privacy
loaded https://www.google.com and then searched for Top Stories
inspected the first article via DevTools and ensured that jsaction attribute wasn't appeared in the anchor link
clicked on the link and ensured that the de-AMP version of the website/news article was being loaded

`Example`	`Example`	`Example`

ShivanKaul self-assigned this Apr 6, 2022

ShivanKaul commented Apr 6, 2022

View reviewed changes

ShivanKaul requested review from bridiver and antonok-edm April 6, 2022 21:05

ShivanKaul force-pushed the feature/de-amp/remove-jsaction-attr branch from 78eec0d to 1d50dc8 Compare April 6, 2022 21:11

antonok-edm approved these changes Apr 6, 2022

View reviewed changes

ShivanKaul force-pushed the feature/de-amp/remove-jsaction-attr branch from 59c972e to 14ec86c Compare April 12, 2022 00:39

ShivanKaul marked this pull request as ready for review April 12, 2022 00:40

ShivanKaul requested a review from a team as a code owner April 12, 2022 00:40

github-actions bot added the rebase label Apr 12, 2022

Add support for checking de_amp_enabled user pref in cosmetic js filter

9facd25

Add test

github-actions bot removed the rebase label Apr 12, 2022

brave-builds force-pushed the feature/de-amp/remove-jsaction-attr branch from 14ec86c to 9facd25 Compare April 12, 2022 00:42

Add dep

7de93b4

ShivanKaul added this to the 1.39.x - Nightly milestone Apr 13, 2022

bridiver reviewed Apr 13, 2022

View reviewed changes

Use closure instead of dynamic params callback

102d405

bridiver approved these changes Apr 14, 2022

View reviewed changes

bridiver merged commit 6b29263 into master Apr 14, 2022

bridiver deleted the feature/de-amp/remove-jsaction-attr branch April 14, 2022 15:44

ShivanKaul restored the feature/de-amp/remove-jsaction-attr branch April 19, 2022 21:01

This was referenced Apr 19, 2022

Uplift to Beta - Add support for checking de_amp_enabled user pref in cosmetic js filter #13082

Merged

Add De-AMP setting for Android, enable by default #13001

Merged

ShivanKaul added the feature/De-AMP label Apr 21, 2022

kjozwiak deleted the feature/de-amp/remove-jsaction-attr branch April 22, 2022 02:57

kjozwiak mentioned this pull request Apr 22, 2022

[De-AMP] Add support for checking de_amp_enabled user pref in cosmetic js filter brave/brave-browser#22228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for checking de_amp_enabled user pref in cosmetic js filter #12923

Add support for checking de_amp_enabled user pref in cosmetic js filter #12923

ShivanKaul commented Apr 6, 2022 •

edited

Loading

ShivanKaul Apr 6, 2022

antonok-edm Apr 6, 2022

ShivanKaul Apr 6, 2022

antonok-edm Apr 6, 2022

antonok-edm Apr 6, 2022 •

edited

Loading

ShivanKaul Apr 6, 2022

antonok-edm left a comment

bridiver Apr 13, 2022

bridiver Apr 13, 2022

ShivanKaul Apr 13, 2022 •

edited

Loading

bridiver Apr 13, 2022

bridiver Apr 13, 2022

kjozwiak commented Apr 22, 2022 •

edited

Loading

Add support for checking de_amp_enabled user pref in cosmetic js filter #12923

Add support for checking de_amp_enabled user pref in cosmetic js filter #12923

Conversation

ShivanKaul commented Apr 6, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShivanKaul Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjozwiak commented Apr 22, 2022 • edited Loading

de-AMP Disabled

de-AMP Enabled

de-AMP Disabled

de-AMP Enabled

ShivanKaul commented Apr 6, 2022 •

edited

Loading

antonok-edm Apr 6, 2022 •

edited

Loading

ShivanKaul Apr 13, 2022 •

edited

Loading

kjozwiak commented Apr 22, 2022 •

edited

Loading

`de-AMP Disabled`

`de-AMP Enabled`

`de-AMP Disabled`

`de-AMP Enabled`