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

ShivanKaul · 2022-04-12T23:55:03Z

Resolves brave/brave-browser#21643

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:

Comprehensive QA/test plan at #11750 (comment)

In addition to that, the test cases mentioned in #12923 (comment) should work on Android.

SergeyZhukovsky · 2022-04-13T02:22:08Z

components/de_amp/common/features.cc

-      base::FEATURE_ENABLED_BY_DEFAULT
-#endif
-};
+const base::Feature kBraveDeAMP{"BraveDeAMP", base::FEATURE_ENABLED_BY_DEFAULT};


do we want it to be enabled by default? An original issue says that it will be controlled by Griffin.

I think the idea was to have it enabled by default, and there's a feature flag that we could use to turn it off via Griffin in case there are issues. Can you confirm @pes10k

Yep, exactly right @ShivanKaul

SergeyZhukovsky · 2022-04-13T17:31:41Z

there is a failed java tests because of an added a new pref.

ShivanKaul · 2022-04-13T18:36:42Z

@SergeyZhukovsky incremented BRAVE_PRIVACY_SETTINGS_NUMBER_OF_ITEMS, I think that was the only failing test.

SergeyZhukovsky

++

kjozwiak · 2022-04-22T02:12:33Z

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

Used the STR/Cases outlined via #11750.

Test Case 1: Inspecting AMP elements on pages

Disable Auto-redirect AMP pages in brave://settings/privacy
Disable Enable De-AMP flag - brave://flags/#brave-de-amp
Relaunch Brave
Visit AMP page https://shivankaul.com/brave/de-amp/test-small-amp.html.
- Inspected element and amp attribute will be shown in the top level <html amp> tag for the AMP page and has canonical link that redirects to html
Close the tab and visit another AMP page https://shivankaul.com/brave/de-amp/test-small-amp-with-lightning.html
- Inspected element and it contains lightning symbol in the top-level <html ⚡> tag for the AMP page and has canonical link that redirects to html
Close the tab and visit https://shivankaul.com/brave/de-amp/test-no-canonical-link.html
- Inspected element and it doesn't contain any canonical links or top-level tags
Close the tab and visit https://shivankaul.com/brave/de-amp/test-wrong-scheme.html
- Inspected element for canonical link that is non-HTTP(s) - <link rel="canonical" href="ipfs://QmcniBv7UQ4gGPQQW2BwbD4ZZHzN3o3tPuNLZCbBchd1zh">
Close the tab and visit https://shivankaul.com/brave/de-amp/test-small-amp-same-canonical.html
- Inspected the element and it has canonical link that is pointed to original/same URL
Close the tab and visit https://www.buzzfeed.com/amphtml/ryanschocket2/snl-spoofs-will-smith-chris-rock-slap
- Inspected the element and it has canonical link that is pointed https://www.buzzfeed.com/ryanschocket2/snl-spoofs-will-smith-chris-rock-slap

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

Test Case 2: Verifying AMP page will not be redirected when both de-AMP flag and auto-redirect AMP page are disabled

Disable Auto-redirect AMP pages in brave://settings/privacy
Disable #brave-de-amp flag in brave://flags/
Relaunch the browser
Visit https://shivankaul.com/brave/de-amp/test-small-amp.html
Inspect the page and it showedamp as attribute
Contains canonical in the tag
AMP Page is not redirected to brave.com

`Example`	`Example`	`Example`

Prerequisites for the following test cases:

Verified both brave://settings/privacy & Auto-redirect AMP pages are enabled by default
Verified Enable De-AMP is enabled by default via brave://flags/#brave-de-amp

Test Case 3: Verifying de-AMPing works as expected i.e redirects to brave.com with default settings

Visit https://shivankaul.com/brave/de-amp/test-small-amp.html
De-AMPing worked i.e AMP page is redirected to brave.com, as it contained the markup for top-level <html amp>
The markup was verified in Test Case 1 for the above AMP page and amp attribute will be shown in the top level <html amp> tag for the AMP page and has canonical link that redirects to html

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

Test Case 4: Verifying de-AMPing works if the AMP page contains the lightning emoji instead of "amp" as attribute

Visit https://shivankaul.com/brave/de-amp/test-small-amp-with-lightning.html
De-AMPing worked i.e AMP page is redirected to brave.com as it contained markup for top-level <html ⚡> tag
The markup was verified in Testcase 1 for the above AMP page has lightning <html ⚡> tag and contains canonical in the tag for redirect

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

Test Case 5: Verifying de-AMPing will not work for AMP page with no canonical link

Visit https://shivankaul.com/brave/de-amp/test-no-canonical-link.html
De-AMPing did not work i.e AMP page is not redirected since there is no canonical link tag
The markup was verified in Test Case 1, for the above AMP page has no canonical link tag hence no redirect available

`Example`	`Example`	`Example`

Test Case 6: Verifying de-AMPing will not work for AMP page that includes canonical link with scheme != HTTP(S)

Visit https://shivankaul.com/brave/de-amp/test-wrong-scheme.html
De-AMPing did not work i.e AMP page is not redirected since the AMP page has canonical link with scheme non HTTP(S)
Verified in Test Case 1, Step 8, that the above AMP page has canonical link with scheme non HTTP(S)

`Example`	`Example`	`Example`

Test Case 7: Verifying de-AMPing will not work for AMP page with canonical link same as original URL

Visit https://shivankaul.com/brave/de-amp/test-small-amp-same-canonical.html
"De-AMPing did not work i.e AMP page is not redirected since the AMP page has canonical link same as original URL
Verified in Test Case 1, Step 9, that the above AMP page has canonical link same as original URL

`Example`	`Example`	`Example`

Test Case 8: Verifying that navigating backward and forward ignores the AMP page in history

Visited example.com
Visited AMP page in the same tab- https://shivankaul.com/brave/de-amp/test-small-amp.html
Page is redirected to brave.com
Visited https://basicattentiontoken.org in the same tab
clicked back in the browser
Returned to brave.com
Clicked back again
AMP page https://shivankaul.com/brave/de-amp/test-small-amp.html is ignored and is not listed in history
Clicked forward
User is taken brave.com
Clicked forward again
Returned to https://basicattentiontoken.org
AMP page https://shivankaul.com/brave/de-amp/test-small-amp.html is ignored and is not listed in history
Verified the history lists example.com, brave.com, and basicattentiontoken.org

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

Test Case 9: Verifying restore de-AMPed page

Visit https://shivankaul.com/brave/de-amp/test-small-amp.html
Page should be redirected to brave.com
Opened a few tabs
Close brave.com tab
Restore the tab
De-AMPed page loaded

`Example (Before Restore)`	`Example (After Restore)`

Test Case 10: Verfying de-AMPing works well with small pages < 64 K works expected

Visit https://shivankaul.com/brave/de-amp/test-small-amp.html
De-AMPing worked i.e AMP page is redirected to brave.com

Test Case 11: Verifying large AMP page (> 64K) works as expected

Visit https://www.buzzfeed.com/amphtml/ryanschocket2/snl-spoofs-will-smith-chris-rock-slap
De-AMP ing worked i.e AMP page is redirected to https://www.buzzfeed.com/ryanschocket2/snl-spoofs-will-smith-chris-rock-slap
The markup was verified in Test Case 1 for the above AMP page has canonical link to https://www.buzzfeed.com/ryanschocket2/snl-spoofs-will-smith-chris-rock-slap

Test Case 12: Verifying de-AMPing will not work with Auto-redirect AMP pages ON and de-AMPing feature flag ON (default)

Disable Auto-redirect AMP pagesinbrave://settings/privacy
Keep Enable De-AMP flag as a default in brave://flags/#brave-de-amp
Visit AMP page https://shivankaul.com/brave/de-amp/test-small-amp.html
Page is not redirected to brave.com

`Example`	`Example`	`Example`

Test Case 13: Verifying de-AMPing will not work with Auto-redirect AMP pages ON and de-AMPing feature flag OFF

EnableAuto-redirect AMP pagesinbrave://settings/privacy
Disable Enable De-AMP flag in brave://flags/#brave-de-amp
Visit AMP page https://shivankaul.com/brave/de-amp/test-small-amp.html
Page is not redirected to brave.com

`Example`	`Example`	`Example`

Add De-AMP setting for Android, enable by default

45b8bc8

ShivanKaul self-assigned this Apr 12, 2022

SergeyZhukovsky reviewed Apr 13, 2022

View reviewed changes

Increment number of settings for test

3b81e71

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

SergeyZhukovsky approved these changes Apr 13, 2022

View reviewed changes

ShivanKaul merged commit e0ad15b into master Apr 13, 2022

ShivanKaul deleted the feature/de-amp/android-pref branch April 13, 2022 21:37

ShivanKaul restored the feature/de-amp/android-pref branch April 19, 2022 15:51

brave-builds pushed a commit that referenced this pull request Apr 19, 2022

Uplift of #13001 (squashed) to beta

2a37d55

brave-builds mentioned this pull request Apr 19, 2022

Add De-AMP setting for Android, enable by default (uplift to 1.38.x) #13063

Merged

7 tasks

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

kjozwiak mentioned this pull request Apr 22, 2022

[De-AMP] Follow up issues for Android brave/brave-browser#21643

Closed

ShivanKaul mentioned this pull request Jun 24, 2022

[Android] Bypass/Disable Accelerated mobile pages(AMP) from Google search. brave/brave-browser#7366

Closed

ShivanKaul mentioned this pull request Dec 6, 2022

Refactor De-AMP user preference brave/brave-browser#27179

Open

bsclifton deleted the feature/de-amp/android-pref branch November 10, 2023 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ShivanKaul commented Apr 12, 2022 •

edited

Loading

SergeyZhukovsky Apr 13, 2022

ShivanKaul Apr 13, 2022

pes10k Apr 13, 2022

SergeyZhukovsky commented Apr 13, 2022

ShivanKaul commented Apr 13, 2022

SergeyZhukovsky left a comment

kjozwiak commented Apr 22, 2022 •

edited

Loading

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

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

Conversation

ShivanKaul commented Apr 12, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

SergeyZhukovsky Apr 13, 2022

Choose a reason for hiding this comment

ShivanKaul Apr 13, 2022

Choose a reason for hiding this comment

pes10k Apr 13, 2022

Choose a reason for hiding this comment

SergeyZhukovsky commented Apr 13, 2022

ShivanKaul commented Apr 13, 2022

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

kjozwiak commented Apr 22, 2022 • edited Loading

Prerequisites for the following test cases:

ShivanKaul commented Apr 12, 2022 •

edited

Loading

kjozwiak commented Apr 22, 2022 •

edited

Loading