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

Cleans up pre-populated search engines. #1648

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Feb 13, 2019

Engines kept:

  • Google
  • Duckduckgo
  • Qwant
  • Bing
  • Startpage.

Adds a unit test to verify the search engines overridden by us are used
instead of the original ones.

Lint fixes.

Fixes brave/brave-browser#3316

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Automated testing:
npm run test -- brave_unit_tests --filter=BraveTemplateURLPrepopulateDataTest.*
npm run test -- brave_unit_tests --filter=BraveTemplateURLServiceUtilTest.*

Manual testing:

  1. Test transition from pre-existing profile; default not changed.
  • Start Brave version prior to version that contains this change,
  • Navigate to brave://settings/searchEngines and verify that the default search engine has not been
    changed (e.g. Google for U.S., Qwant for DE & FR),
  • Exit Brave,
  • Start Brave version that contains this change,
  • Navigate to brave://settings/searchEngines and verify that only the 5 engines listed above are
    present and that the default engine has not changed.
  1. Test transition from pre-existing profile; default changed.
  • Start Brave version prior to version that contains this change,
  • Navigate to brave://settings/searchEngines and change the default search engine to Amazon,
  • Exit Brave,
  • Start Brave version that contains this change,
  • Navigate to brave://settings/searchEngines and verify that only the 5 engines listed above + Amazon
    are present and that the default engine is Amazon.
  1. Test new profile.
  • Start Brave version that contains this change with a fresh profile,
  • Navigate to brave://settings/searchEngines and verify that only the 5 engines listed above are present
    and that Qwant is the default engine for DE & FR and Google for all others.
  1. Test import from Muon; removed engine.
  • Start Brave (Muon) and change the default search engine to Amazon, exit the browser,
  • Start Brave version that contains this change with a fresh profile,
  • In the Welcome screen, click next, then Import,
  • Pick import from Brave (old) and leave all boxes checked,
  • Finish import,
  • Navigate to brave://settings/searchEngines and verify that Google (or Qwant for DE, FR locales)
    remains the default engine.
  1. Test import from Muon; kept engine.
  • Start Brave (Muon) and change the default search engine to DuckDuckGo, exit the browser,
  • Start Brave version that contains this change with a fresh profile,
  • In the Welcome screen, click next, then Import,
  • Pick import from Brave (old) and leave all boxes checked,
  • Finish import,
  • Navigate to brave://settings/searchEngines and verify that DuckDuckGo is set as the
    default engine.

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

@mkarolin mkarolin added this to the 0.62.x - Nightly milestone Feb 13, 2019
@mkarolin mkarolin self-assigned this Feb 13, 2019
@mkarolin mkarolin changed the title WIP: Cleans up pre-populated search engines. Cleans up pre-populated search engines. Feb 13, 2019
simonhong
simonhong previously approved these changes Feb 13, 2019
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.

Tested this PR on MacOS and works great. Also all manual tests works as expected.

@diracdeltas
Copy link
Member

@mkarolin we are also discussing keeping yahoo; let me cc you on the discussion in slack

@diracdeltas
Copy link
Member

👍 confirmed these are the 5 we want to keep

@bbondy
Copy link
Member

bbondy commented Feb 14, 2019

Thanks @mkarolin, could you do uplift PRs too?

@bbondy bbondy removed their request for review February 14, 2019 04:12
Engines kept:
- Google
- Duckduckgo
- Qwant
- Bing
- Startpage.

Adds a unit test to verify the search engines overridden by us are used
instead of the original ones.

Lint fixes.

Fixes brave/brave-browser#3316
@bsclifton
Copy link
Member

Did a rebase (sorry for invaliding review! in case you wanted to re-approve, @simonhong)

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.

++

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

For future PRs, it might be better to have two commits; one fixing the issue and the second fixing linting errors... but awesome work 😄👍

@bsclifton bsclifton merged commit aaae2ea into master Feb 14, 2019
@bsclifton bsclifton deleted the maxk-cleanup-prepopulated-search-engines branch February 14, 2019 17:27
@bsclifton
Copy link
Member

Trying to uplift; looks like this works cleanly on dev (0.61.x) #1674

But appears to be some conflicts when uplifting to beta... @mkarolin can you check this out?

{"Google", TemplateURLPrepopulateData::google},
{"Infogalactic", TemplateURLPrepopulateData::infogalactic},

Choose a reason for hiding this comment

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

🎉

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.

Clean up Default search engines settings
7 participants