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

Update search engines #9397

Merged
merged 2 commits into from
Jul 14, 2021
Merged

Update search engines #9397

merged 2 commits into from
Jul 14, 2021

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Jul 11, 2021 •
edited
Loading

Fixes brave/brave-browser#16825
Fixes brave/brave-browser#16870

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
  • 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:

@bsclifton bsclifton self-assigned this Jul 11, 2021
@bsclifton bsclifton changed the title WIP: Update search engines Update search engines Jul 12, 2021
@bsclifton bsclifton marked this pull request as ready for review July 12, 2021 05:44
@bsclifton bsclifton requested a review from a team as a code owner July 12, 2021 05:44
@bsclifton bsclifton force-pushed the bsc-search-updates branch 2 times, most recently from 9418a18 to 211bc35 Compare July 13, 2021 06:18
bsclifton
bsclifton commented Jul 13, 2021
View reviewed changes
chromium_src/components/search_engines/template_url_prepopulate_data.cc
@@ -159,66 +159,97 @@ const std::map<int, const std::vector<BravePrepopulatedEngineID>*>

// A versioned map tracking the singular default search engine per-country.
BravePrepopulatedEngineID GetDefaultSearchEngine(int country_id, int version) {
const BravePrepopulatedEngineID default_v6 =
Copy link
Member Author

Choose a reason for hiding this comment

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

cpplint complained about a chunk of this, so I just ran the whole function through clang-format

bsclifton
bsclifton commented Jul 13, 2021
View reviewed changes
chromium_src/components/search_engines/template_url_prepopulate_data.cc
{country_codes::CountryCharsToCountryID('U', 'Z'),
PREPOPULATED_ENGINE_ID_YANDEX},
});
static const base::NoDestructor<
Copy link
Member Author

@bsclifton bsclifton Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this is the only real change (in this file); adding a v16 and then below looking for > 15

mariospr
mariospr approved these changes Jul 13, 2021
View reviewed changes
Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment/question on removing the unit tests (vs updating expectations)

chromium_src/components/search_engines/brave_template_url_prepopulate_data_unittest.cc Outdated Show resolved Hide resolved
simonhong
simonhong approved these changes Jul 14, 2021
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.

++

@bsclifton bsclifton merged commit 97bf350 into master Jul 14, 2021
@bsclifton bsclifton deleted the bsc-search-updates branch July 14, 2021 05:57
@bsclifton bsclifton added this to the 1.28.x - Nightly milestone Jul 14, 2021
brave-builds pushed a commit that referenced this pull request Jul 14, 2021
@brave-browser-releases
Uplift of #9397 (squashed) to beta
fbe7323
@brave-builds brave-builds mentioned this pull request Jul 14, 2021
Merged
7 tasks
@stephendonner
Copy link
Contributor

stephendonner commented Jul 14, 2021 •
edited
Loading

Verified PASSED for brave/brave-browser#16825 using

Brave 1.28.80 Chromium: 92.0.4515.93 (Official Build) nightly (x86_64)
Revision 6eb43ff7850a1d710c3f827a0555737c74edab5c-refs/branch-heads/4515@{#1378}
OS macOS Version 11.4 (Build 20F71)

and

Brave 1.28.80 Chromium: 92.0.4515.93 (Official Build) nightly (64-bit)
Revision 6eb43ff7850a1d710c3f827a0555737c74edab5c-refs/branch-heads/4515@{#1378}
OS Windows 10 OS Version 2009 (Build 22000.65)

Steps:

  1. new profile
  2. launched Brave
  3. stepped through onboarding until I came to the search-engine screen
  4. confirmed -- with the exception of Skip welcome tour vs SKIP (Figma) -- that the copy is verbatim.
nightly macOS nightly Windows Figma
Screen Shot 2021-07-14 at 9 05 26 AM onboarding-windows 125126193-9219e000-e0af-11eb-95ff-0e4298ecf04d

@stephendonner stephendonner mentioned this pull request Jul 14, 2021
Closed
@bsclifton
Copy link
Member Author

NOTE: unfortunately, this pull request caused brave/brave-browser#16950

If we uplift this PR, we'll also want to uplift #9486

@bsclifton bsclifton mentioned this pull request Jul 21, 2021
Merged
7 tasks
bsclifton pushed a commit that referenced this pull request Jul 21, 2021
@brave-browser-releases @bsclifton
Uplift of #9397 (squashed) to beta
2a8a401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@mariospr mariospr mariospr approved these changes

@mkarolin mkarolin Awaiting requested review from mkarolin

@nullhook nullhook Awaiting requested review from nullhook

@petemill petemill Awaiting requested review from petemill

@deeppandya deeppandya Awaiting requested review from deeppandya

Assignees

@bsclifton bsclifton

Labels
None yet
Projects
None yet
Milestone
1.28.x - Release
Development

Successfully merging this pull request may close these issues.

Update search engine defaults for DE / AU / NZ / IE update search engine onboarding screen
4 participants
@bsclifton @stephendonner @mariospr @simonhong