Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Misc fixes for DeAMP #12999

Merged
merged 4 commits into from
Apr 13, 2022
Merged

Misc fixes for DeAMP #12999

merged 4 commits into from
Apr 13, 2022

Conversation

ShivanKaul
Copy link
Collaborator

@ShivanKaul ShivanKaul commented Apr 12, 2022 •
edited
Loading

Resolves brave/brave-browser#22262

  1. Tweak DeAMP regexes to be lazy
  2. The amp html attribute might have empty value i.e. amp=""
  3. Move DeAMP (from Security & Privacy section) and ReduceLanguage (from bottom of brave://settings/shields) settings to be near-top of brave://settings/shields so as to separate them from other per-site privacy settings

For 3, this is what the brave://settings/shields page looks like after this PR:
image

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:

  1. Go to
    https://amp.theguardian.com/us-news/commentisfree/2016/feb/16/thomas-piketty-bernie-sanders-us-election-2016
    https://www.buzzfeed.com/amphtml/hbraga/your-new-favorite-products
    https://www.foxnews.com/world/russian-crematoria-mariupol-new-auschwitz-mayor.amp
    https://www.nbcnews.com/news/amp/rcna23233 and make sure that they all load non-AMP version instead (i.e. view source should not have <html ... amp> anywhere. Applicable for both Android and Desktop.
  2. Auto-redirect AMP pages no longer lives in brave://settings/privacy but instead lives in brave://settings/shields, and Prevent sites from fingerprinting me based on my language preferences setting is next to the former setting. NOTE: this is only applicable for Desktop.

@ShivanKaul ShivanKaul self-assigned this Apr 12, 2022
pes10k
pes10k approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as those regex's do what they say on the tin, LGTM

@ShivanKaul ShivanKaul added this to the 1.39.x - Nightly milestone Apr 13, 2022
ShivanKaul
ShivanKaul commented Apr 13, 2022
View reviewed changes
browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html Outdated
@@ -56,6 +56,18 @@
pref="{{prefs.brave.shields.stats_badge_visible}}"
label="$i18n{showStatsBlockedBadgeLabel}">
</settings-toggle-button>
<settings-toggle-button
class="cr-row"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to fix the indentation here, rest everything looks good and all CI passed.

@ShivanKaul ShivanKaul merged commit f7a643b into master Apr 13, 2022
@ShivanKaul ShivanKaul deleted the feature/de-amp/misc-fixes branch April 13, 2022 16:27
@ShivanKaul ShivanKaul restored the feature/de-amp/misc-fixes branch April 19, 2022 15:50
ShivanKaul added a commit that referenced this pull request Apr 19, 2022
@ShivanKaul
Misc fixes for DeAMP (#12999)
51439b5 
- Tweak DeAMP regexes to be lazy
- The amp html attribute might have empty value i.e. amp=""
- Move DeAMP (from Security & Privacy section) and ReduceLanguage (from bottom of brave://settings/shields) settings to be near-top of brave://settings/shields so as to separate them from other per-site privacy settings
@ShivanKaul ShivanKaul mentioned this pull request Apr 19, 2022
Merged
25 tasks
@aguscruiz
Copy link
Collaborator

aguscruiz commented Apr 20, 2022 •
edited
Loading

Added the AMP option here:
https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/Desktop-Settings?node-id=1874%3A32653

image

Screen without the option
https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/Desktop-Settings?node-id=1832%3A32253

image

@ShivanKaul
Copy link
Collaborator Author

@aguscruiz I think that mockup might also need to include Reduce Language setting that @pilgrim-brave added in #12234:
image

@aguscruiz
Copy link
Collaborator

Ok, added. @rebron would you confirm that's right? I can always revert

@kjozwiak kjozwiak deleted the feature/de-amp/misc-fixes branch April 22, 2022 02:14
@ShivanKaul ShivanKaul mentioned this pull request Apr 22, 2022
Merged
25 tasks
@kjozwiak
Copy link
Member

kjozwiak commented Apr 22, 2022 •
edited
Loading

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

Brave | 1.40.5 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)

Ensured that Auto-redirect AMP pages was moved from brave://settings/privacy -> brave://settings/shields

Before After
image image
image image

Also as per #12999 (comment), went thorough the websites and ensured that they're loading the de-amp website and <html ... amp> shouldn't be visible.

de-AMP Disabled de-AMP Enabled
image image
image image
image image
image image

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

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

As per @ShivanKaul, the settings changes do not apply to Android so we basically just need to ensure that the websites mentioned via #12999 (comment) are working as expected.

de-AMP Disabled de-AMP Enabled
image image
image image
image image
image image

kjozwiak added a commit that referenced this pull request Apr 22, 2022
@kjozwiak
Merge pull request #13080 from brave/1.38.x-de-amp-misc-fixes
2a4059e 
Uplift - Misc fixes for DeAMP (#12999)
@Uni-verse Uni-verse mentioned this pull request Apr 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pilgrim-brave pilgrim-brave pilgrim-brave approved these changes

@pes10k pes10k pes10k approved these changes

@petemill petemill Awaiting requested review from petemill

Assignees

@ShivanKaul ShivanKaul

Labels
feature/De-AMP
Projects
None yet
Milestone
1.39.x - Release
Development

Successfully merging this pull request may close these issues.

[De-AMP] Miscellaneous fixes
5 participants
@ShivanKaul @aguscruiz @kjozwiak @pes10k @pilgrim-brave