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.

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

Better detection of AMP pages #611

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Better detection of AMP pages #611

merged 8 commits into from
Jan 16, 2024

Conversation

SlayterDev
Copy link
Contributor

@SlayterDev SlayterDev commented Dec 21, 2023

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1202198075905303/1202190330709645/f
iOS PR: duckduckgo/iOS#2284
macOS PR: duckduckgo/macos-browser#2001
What kind of version bump will this require?: Patch

Optional:

Tech Design URL:
CC:

Description:
This PR modifies the AMP feature to check for on page elements to determine if a link is an AMP link preventing false positives. It also implements the deepExtractionEnabled key allowing us to remotely disable deep extraction if needed.

Steps to test this PR:

  1. Use this config
  2. Visit freecodecamp.org and click "Get Started". It should take you to a login/signup page
  3. Visit theprisonerwinecompany.com. Add something to your cart and click checkout. It should take you to the checkout page.
  4. Visit golf.com, click an article, it should load without looping.
  5. Visit https://privacy-test-pages.site/privacy-protections/amp/ and test the "First Party Cloaked" links. They should take you to the expected url.

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

@SlayterDev SlayterDev marked this pull request as ready for review December 21, 2023 20:41
Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

This works per the test instructions, and general smoke testing of other URL functionality hasn't turned up any issues. LGTM

@SlayterDev SlayterDev merged commit 44569e2 into main Jan 16, 2024
5 checks passed
@SlayterDev SlayterDev deleted the brad/better-amp-checks branch January 16, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants