Skip to content

redirecting AMP to canonical pages #4950

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
Closed
wants to merge 1 commit into from
Closed

redirecting AMP to canonical pages #4950

wants to merge 1 commit into from

Conversation

@AndriusA
Copy link

@AndriusA AndriusA commented Mar 16, 2020 •
edited
Loading

Trivial script (similar to what existing extensions use) to locate canonical URL in AMP pages and redirect, if user has turned the option ON in settings, under shields. Off by default.

Resolves brave/brave-browser#8720

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.

@AndriusA AndriusA requested a review from bridiver as a code owner March 16, 2020 11:40
@AndriusA AndriusA force-pushed the amp_neutralizer branch 2 times, most recently from dd7db51 to e2edd53 Compare March 16, 2020 14:07
simonhong
simonhong requested changes Mar 17, 2020
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.
Can you file an issue for this PR if not filed yet and write it in this PR's description?
and Please check submitter's checklist.

@AndriusA AndriusA added enhancement CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 17, 2020
@AndriusA
Copy link
Author

AndriusA commented Mar 17, 2020

Previous build passed on all platforms but macos, with what looked like transient errors on macos. Skipping other platforms to speed up

simonhong
simonhong approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

@dkolba
Copy link

dkolba commented Mar 18, 2020

I would prefer it the other way around 😂

@AndriusA AndriusA removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Apr 27, 2020
bridiver
bridiver reviewed Apr 27, 2020
View reviewed changes
components/brave_extension/extension/brave_extension/background/reducers/ampReducer.ts
if (action.isMainFrame) {
ampAPI.getAMPNeutralized().then(ampNeutralized => {
if (ampNeutralized) {
chrome.tabs.executeScript(action.tabId, {
Copy link
Collaborator

@bridiver bridiver Apr 27, 2020

Choose a reason for hiding this comment

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

we can do the redirect in the native code, we really want to avoid using extensions for stuff like this. Check out brave_common_static_redirect_network_delegate_helper

Copy link
Collaborator

@bridiver bridiver Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

so actually in moving this to greaselion we will just conditionally enable this content script when the pref values changes so this won't be needed at all

Copy link
Collaborator

@bridiver bridiver Jun 25, 2020

Choose a reason for hiding this comment

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

same with this https://github.com/brave/brave-core/pull/4950/files#diff-e5fa22746ae0f1e347e9e93cd7113627R9

bridiver
bridiver requested changes Apr 27, 2020
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

we should use native code for this

@andmade
Copy link

andmade commented May 20, 2020

I don't care where it happens. I just wish it would happen. 😔

@bsclifton bsclifton mentioned this pull request Feb 5, 2021
Closed
@bsclifton bsclifton removed the request for review from cezaraugusto September 23, 2021 06:27
This was referenced Sep 23, 2021
Closed
Closed
@AndriusA AndriusA requested a review from a team as a code owner April 15, 2022 21:04
@iefremov
Copy link
Contributor

iefremov commented May 25, 2022

De-amp was implemented and shipped

@iefremov iefremov closed this May 25, 2022
@bsclifton bsclifton deleted the amp_neutralizer branch February 27, 2025 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bridiver bridiver bridiver requested changes

@simonhong simonhong simonhong approved these changes

Assignees

@pes10k pes10k

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Redirecting AMP to canonical pages

8 participants

@AndriusA @dkolba @andmade @iefremov @bridiver @simonhong @pes10k