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

Updated default search providers in KR #10590

Merged
merged 3 commits into from Jan 24, 2023
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 19, 2021

Resolves brave/brave-browser#18855

Desktop & android will have Brave Search, Daum, Naver and Google as a default search provider.

Desktop search engine settings page

image

Android onboarding page

Android search engine settings page

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:

  1. Set Region settings as South Korea
  2. Launch Brave with fresh profile
  3. Check search engine settings has SK specific provider list - Brave, Naver, Daum and Google
  4. Check Naver is set as an initial default provider

@simonhong simonhong self-assigned this Oct 19, 2021
@simonhong simonhong force-pushed the default_search_provider_sk branch 2 times, most recently from 061226d to 3161ace Compare June 27, 2022 06:10
@simonhong
Copy link
Member Author

@rebron @bradleyrichter Do we want to update Android onboarding with this default search provider change?
If so, I need Naver and Daum 's logo image and their description.

@rebron
Copy link
Collaborator

rebron commented Jun 29, 2022

@timchilds @anthonypkeane We need getting assets for Daum and Naver, logo and description.

@anthonypkeane
Copy link

We don't show this screen in Android during onboarding. If we are, it is a bug.

@simonhong
Copy link
Member Author

We don't show this screen in Android during onboarding. If we are, it is a bug.

@anthonypkeane This search provider onboarding is launched for first touch on omnibox.
AFAIK, it's not launched for some regions that brave search is configured as a default.
@deeppandya Am I right?

@anthonypkeane
Copy link

@deeppandya
Copy link
Contributor

We don't show this screen in Android during onboarding. If we are, it is a bug.
@anthonypkeane we still show the onboarding except countries listed here :

private static final List<String> mBraveSearchEngineDefaultRegions =

@simonhong
Copy link
Member Author

Kindly ping for logo and description :)

@anthonypkeane
Copy link

@rmcfadden3 can you please help with the copy for Daum & Naver?
@Sam-Sibley could you pls provide search icons for Daum & Naver?

@rmcfadden3
Copy link

How about these descriptions?

  • Daum: South Korean web portal & search
  • Naver: South Korean web portal & search

I could try to add more to differentiate them, but character count is really limited. LMK.

@anthonypkeane
Copy link

Hi @simonhong

#10590 (comment)

Does this work ok? Thanks

@simonhong
Copy link
Member Author

simonhong commented Aug 9, 2022

Hi @simonhong

#10590 (comment)

Does this work ok? Thanks

@anthonypkeane Desc seems fine. and I was also waiting both providers' logo icon.
If we don't want to use different icon, I could get their official logo icon from their web site and use it.

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 68e4b7d
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: f8f14e7
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@simonhong simonhong changed the title WIP: Updated default search provider in KR Updated default search provider in KR Jan 12, 2023
@simonhong simonhong marked this pull request as ready for review January 12, 2023 07:10
@simonhong simonhong requested a review from a team as a code owner January 12, 2023 07:10
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: f8f14e7
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

1 similar comment
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: f8f14e7
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 2204ec2
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: f60a04b
reason: unsigned
Please follow the handbook to configure commit signing
cc: @simonhong

Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

Android changes LGTM

@deeppandya
Copy link
Contributor

@simonhong were you able to verify the change for android on device ?

@simonhong
Copy link
Member Author

Yes, above screenshots came from my android phone.

@simonhong simonhong changed the title Updated default search provider in KR Updated default search providers in KR Jan 20, 2023
fix brave/brave-browser#18855

Default search provider list will include
Brave Search, Naver, Daum and Google.
And Naver will be set as a default provider.
Daum & Naver are listed in search provider list
@@ -81,6 +81,13 @@ const std::vector<BravePrepopulatedEngineID> brave_engines_AU_IE = {
PREPOPULATED_ENGINE_ID_ECOSIA,
};

const std::vector<BravePrepopulatedEngineID> brave_engines_KR = {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why these are vectors instead of plain arrays?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll try to handle this as a f/u. Filed brave/brave-browser#27943

@simonhong simonhong merged commit 58dfd60 into master Jan 24, 2023
@simonhong simonhong deleted the default_search_provider_sk branch January 24, 2023 11:53
@github-actions github-actions bot added this to the 1.49.x - Nightly milestone Jan 24, 2023
simonhong added a commit that referenced this pull request Jan 25, 2023
Updated default search providers in KR
@kjozwiak
Copy link
Member

kjozwiak commented Feb 2, 2023

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.49.81 Chromium: 110.0.5481.52 (Official Build) nightly (64-bit)
-- | --
Revision | 979113183ded4544a2c443aceb1629c430907e52-refs/branch-heads/5481@{#636}
OS | Windows 11 Version 22H2 (Build 22621.1105)

Test Case #1 - Clean Profile

  • installed 1.49.82 Chromium: 110.0.5481.52 and ensured that brave://settings/search was displaying the correct SE
  • ensured that Naver is selected as the default SE
  • ensured that Brave is at the top of the Normal Window dropdown
  • ensured that Brave is selected as the default SE for Private Window
  • ensured that the correct SE are being listed via brave://settings/searchEngines
  • ensured that you can change the SE for both Normal & Private windows without issues
    • ensured that the changes are retained after restarting the browser several times
  • ensured that the new default SE is also reflected via brave://settings/searchEngines
Example Example Example Example Example Example
cleanProfile1 cleanProfile2 cleanProfile3 cleanProfile4 cleanProfile5 cleanProfile6

Test Case #2 - Upgrade from 1.49.55 Chromium: 109.0.5414.86 --> 1.49.82 Chromium: 110.0.5481.52


Verification FAILED on Pixel 6 running Android 13 using the following build(s):

Brave | 1.49.82 Chromium: 110.0.5481.52 (Official Build) canary (32-bit)
--- | ---
Revision | 979113183ded4544a2c443aceb1629c430907e52-refs/branch-heads/5481@{#636}
OS | Android 13; Build/TQ1A.230105.002

Test Case #1 - Clean Profile

  • installed 1.49.82 Chromium: 110.0.5481.52 and ensured that the SE onboarding is displayed when tapping on omnibox
  • ensured that the new SE mentioned via Updated default search providers in KR #10590 (comment) are being displayed
  • ensured that Naver is selected as the default SE via the SE onboarding
  • ensured that you can switch to different SE via Hamburger Menu -> Search engines without any issues
  • ensured that users choice is being retained when restarting the browser
Example Example Example Example
Screenshot_20230202-042344 Screenshot_20230202-042356 Screenshot_20230202-042400 Screenshot_20230202-042405

Test Case #2 - Upgrade from 1.49.55 Chromium: 109.0.5414.86 --> 1.49.82 Chromium: 110.0.5481.52

Test Case #1 - Default SE (Google) selected via omnibox search onboarding

  • installed 1.49.55 Chromium: 109.0.5414.86 and ensured that the previous onboarding appeared once tapping on omnibox
  • ensured that Google was selected as the default SE via the search onboarding and continued with Google selected
  • ensured that Google was selected as the default SE for both Standard Tab & Private Tab
Example Example Example Example
Screenshot_20230202-023804 Screenshot_20230202-023814 Screenshot_20230202-023818 Screenshot_20230202-023822
  • upgraded from 1.49.55 Chromium: 109.0.5414.86 --> 1.49.82 Chromium: 110.0.5481.52
  • ensured that Google was still set as the default SE for both Standard Tab & Private Tab
  • ensured that the new SE are being displayed within Standard Tab & Private Tab
  • ensured that Google is also being displayed via both Standard Tab & Private Tab
  • ensured that Brave is at the top of the list for both Standard Tab & Private Tab
  • ensured that you can switch to other SE after upgrading without any issues
    • ensured that the changes are retained when restarting the browser several times
Example Example Example
Screenshot_20230202-023854 Screenshot_20230202-023858 Screenshot_20230202-023903

Test Case #2 - Selecting other SE (DDG) via omnibox search onboarding

  • installed 1.49.55 Chromium: 109.0.5414.86 and ensured that the previous onboarding appeared once tapping on omnibox
  • ensured that Google was selected as the default SE via the search onboarding
  • select DDG as the default SE and ensure both Standard Tab & Private Tab are using DDG
Example Example Example Example
Screenshot_20230202-031605 Screenshot_20230202-031614 Screenshot_20230202-031618 Screenshot_20230202-031622
  • upgraded from 1.49.55 Chromium: 109.0.5414.86 --> 1.49.82 Chromium: 110.0.5481.52
  • ensured that DDG was still set as the default SE for both Standard Tab & Private Tab
  • ensured that the new SE are being displayed within Standard Tab & Private Tab
  • ensured that DDG is also being displayed via both Standard Tab & Private Tab
  • ensured that Brave is at the top of the list for both Standard Tab & Private Tab
  • ensured that you can switch to other SE after upgrading without any issues
    • ensured that the changes are retained when restarting the browser several times
Example Example Example
Screenshot_20230202-031652 Screenshot_20230202-031657 Screenshot_20230202-031704

Test Case #3 - Selecting two different SE via Search Settings before upgrading

  • installed 1.49.55 Chromium: 109.0.5414.86 and ensured that the previous onboarding appeared once tapping on omnibox
  • ensured that Google was selected as the default SE via the search onboarding
  • select Startpage as the default SE and ensure both Standard Tab & Private Tab are using Startpage
  • changed the default SE for both Standard Tab (Bing) & Private Tab (Qwant)
Example Example Example Example Example
Screenshot_20230202-041202 Screenshot_20230202-041208 Screenshot_20230202-041214 Screenshot_20230202-041221 Screenshot_20230202-041224
  • upgraded from 1.49.55 Chromium: 109.0.5414.86 --> 1.49.82 Chromium: 110.0.5481.52
  • ensured that Bing was still set as the default SE for Standard Tab
  • ensured that Qwant was still set as the default SE for Private Tab
  • ensured that the SE are being displayed/selected within Standard Tab & Private Tab
  • ensured that Brave is at the top of the list for both Standard Tab & Private Tab
  • ensured that you can switch to other SE after upgrading without any issues
    • ensured that the changes are retained when restarting the browser several times
Example Example Example
Screenshot_20230202-041346 Screenshot_20230202-041350 Screenshot_20230202-041355

@kjozwiak
Copy link
Member

kjozwiak commented Feb 2, 2023

Created brave/brave-browser#28232 which should be fixed on Android before uplifting the above into 1.48.x via #16842. Also created brave/brave-browser#28235 for Desktop which also needs to be resolved/fixed before uplifting.

Also found brave/brave-browser#28262 which isn't related to the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants