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

ATB parameter in MV3 #1585

Merged
merged 11 commits into from Dec 7, 2022
Merged

ATB parameter in MV3 #1585

merged 11 commits into from Dec 7, 2022

Conversation

jdorweiler
Copy link
Contributor

@jdorweiler jdorweiler commented Dec 5, 2022

Reviewer:

Description:

Adds ATB to search URL.

Steps to test this PR:

  1. From a new Chrome profile, install the extension
  2. Do a search from URL bar, atb should be added to the URL
  3. Check other DDG pages, they should not have atb added to them
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@jdorweiler jdorweiler added the WIP label Dec 5, 2022
@jdorweiler jdorweiler requested a review from kzar December 5, 2022 21:16
@jdorweiler jdorweiler removed the WIP label Dec 6, 2022
Copy link
Collaborator

@kzar kzar left a comment

Choose a reason for hiding this comment

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

Can we enable the ATB integration tests for MV3 too?

shared/js/background/declarative-net-request.js Outdated Show resolved Hide resolved
shared/js/background/declarative-net-request.js Outdated Show resolved Hide resolved
shared/js/background/declarative-net-request.js Outdated Show resolved Hide resolved
@jdorweiler jdorweiler requested a review from kzar December 6, 2022 22:02
Copy link
Collaborator

@kzar kzar left a comment

Choose a reason for hiding this comment

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

Looks good % a couple of nits. The main thing that concerns me is why we needed to disable some of the integration tests, are you sure the tests are broken and not the feature itself?

shared/js/background/atb.es6.js Outdated Show resolved Hide resolved
integration-test/background/atb.js Show resolved Hide resolved
integration-test/background/atb.js Show resolved Hide resolved
shared/js/background/declarative-net-request.js Outdated Show resolved Hide resolved
Co-authored-by: Dave Vandyke <kzar@kzar.co.uk>
@jdorweiler jdorweiler requested a review from kzar December 7, 2022 16:05
Copy link
Collaborator

@kzar kzar left a comment

Choose a reason for hiding this comment

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

LGTM

@jdorweiler jdorweiler merged commit 3744bbe into develop Dec 7, 2022
@jdorweiler jdorweiler deleted the jd/atb-mv3 branch December 7, 2022 18:52
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.

None yet

2 participants