Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Brave Search as an alternative search #8966

Merged
merged 6 commits into from
Jun 17, 2021
Merged

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented May 28, 2021

Security review https://github.com/brave/security/issues/486
Resolves brave/brave-browser#15663

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:

TBD

@bsclifton bsclifton self-assigned this May 28, 2021
@deeppandya
Copy link
Contributor

@jamesmudgett can you provide icons for Brave search for onboarding ?

@bsclifton bsclifton force-pushed the bsc-brave-search branch 2 times, most recently from 9ed4c03 to e93bfe3 Compare June 11, 2021 19:01
@bsclifton bsclifton changed the title Add Brave Search as an alternate search Add Brave Search as an alternative search Jun 14, 2021
@bsclifton
Copy link
Member Author

bsclifton commented Jun 16, 2021

Ready for review 😄

@bsclifton bsclifton marked this pull request as ready for review June 16, 2021 00:35
@bsclifton bsclifton requested a review from a team as a code owner June 16, 2021 00:35
@bsclifton bsclifton requested review from petemill and simonhong June 16, 2021 00:35
@bsclifton bsclifton requested a review from alexsafe June 16, 2021 07:15
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 for chromium_src/

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.

++ with one nit(assert) 😄

@bsclifton
Copy link
Member Author

bsclifton commented Jun 17, 2021

Will fix above (thanks, @simonhong!)- looking at CI, I'm seeing this browser test failure on macOS / Linux / Windows:

[2021-06-16T20:30:13.282Z] 1 test failed:
[2021-06-16T20:30:13.282Z]     BraveSearchTestEnabled.DefaultAPIFalseNoOpenSearch (../../brave/browser/brave_search/brave_search_browsertest.cc:224)

Will check this out too...

@bsclifton bsclifton force-pushed the bsc-brave-search branch 2 times, most recently from 3ac92ad to 8b09acc Compare June 17, 2021 05:56
This ensures user is in US/CA before show Brave Search during
brave://welcome experience. We'll update this as search rolls out
This is supposed to be matching false when no opensearch provider is
there - but now Brave Search is packaged. We'll have to rethink test
@bsclifton
Copy link
Member Author

I've got a lint fix and a JavaScript unit test fix (changing beforeAll to beforeEach to fix an issue w/ jest spy). But so far CI looking great - all platforms are passing (waiting on macOS). If macOS looks good too, I'll mark this PR with CI/Skip, push the fix I have, and merge 😄

@bsclifton bsclifton added the CI/skip Do not run CI builds (except noplatform) label Jun 17, 2021
@bsclifton bsclifton merged commit 783a2cf into master Jun 17, 2021
@bsclifton bsclifton deleted the bsc-brave-search branch June 17, 2021 12:30
@bsclifton bsclifton added this to the 1.28.x - Nightly milestone Jun 17, 2021
brave-builds pushed a commit that referenced this pull request Jun 17, 2021
brave-builds pushed a commit that referenced this pull request Jun 17, 2021
@srirambv
Copy link
Contributor

Verification passed on OnePlus 6T with Android 10 running 1.28.11 x64 Nightly build

Onboarding

  • Verified Brave Search beta is listed on SE onboarding for supported countries (US & Canada)
  • Verified Brave Search beta is not listed on SE onboarding for unsupported countries
  • Verified selecting Brave Search beta during onboarding sets it as default on both normal and private tabs
image image

Search engines screen

  • Verified selecting Brave Search beta from the SE list sets it as default
  • Verified performing a search via omnibox goes to search.brave.com SERP
  • Verified selecting a text and selecting WebSearch from context menu opens a new tab with search.brave.com SERP
  • Verified works on both normal and private tab

Autocomplete suggestions test

Also encountered the following issues

@kjozwiak
Copy link
Member

kjozwiak commented Jun 21, 2021

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.28.13 Chromium: 91.0.4472.114 (Official Build) nightly (64-bit)
--- | ---
Revision | 4bb19460e8d88c3446b360b0df8fd991fee49c0b-refs/branch-heads/4472@{#1496}
OS | Windows 10 OS Version 2009 (Build 19042.1055)

Onboarding/Setting Test Cases

  • ensured that Brave Search beta is only displayed under brave://welcome onboarding for Canada & USA
    • ensured that Japan, German & France weren't displaying Brave Search beta via search onboarding
  • ensured that Brave Search beta appears via brave://settings/search & brave://settings/searchEngines for all regions
  • ensured that the query URL appears as https://search.brave.com/search?q=%s&source=desktop under brave://settings/searchEngines
Japan Onboarding Germany Onboarding France Onboarding USA & Canada Onboarding
Japan germanOnbaordig franceOnboarding USACanada
Japan SE Settings Germany SE Settings France SE Settings USA & Canada SE Settings
JapanList GermanSEsettings FranceSEsettings SElist

URL Query Test Cases

editSE

Default Callback Test Cases

Note: used the cases that I ran through iOS via brave/brave-ios#3745 (comment) as a template for desktop. Also note that at the time of running through this case for uplift, we needed to use search-dev.brave.com to verify the default callback feature.

Test Case 1: (Setting as default via page)

  • install 1.28.13 Chromium: 91.0.4472.114
  • don't run through the search onboarding and leave Google as the default browser
  • login into https://search-dev.brave.com
  • search for Brave Browser via the text field
  • click on the Set default button via the default modal within https://search-dev.brave.com
  • you should see the button turn from Set default --> Done and disappear
  • check and ensure https://search-dev.brave.com is set as the default via brave://settings/search
Example Example Example
defaultBrowser setDefaultBrowser SEList1

Test Case 2: (Private Browsing)

  • install 1.28.13 Chromium: 91.0.4472.114
  • don't run through the search onboarding and leave Google as the default browser
  • launch open * login into https://search-dev.brave.com via Private Browsing
  • search for Basic Attention Token via the text field
  • shouldn't see any Brave Search Default modals

PB

Test Case 3: (Default modal retries per session)

  • install 1.28.13 Chromium: 91.0.4472.114
  • don't run through the search onboarding and leave Google as the default browser
  • login into https://search-dev.brave.com
  • search for Brave Browser via the text field
  • the default modal should appear
  • refresh the page two times (should see the default modal appear
  • refresh again and you shouldn't see the default modal appear for this session
  • open a new tab, open https://search-dev.brave.com and try another search (shouldn't see any more modals for this session)

As per the above, this checks to make sure that the Default modal only appears three times per session.

Test Case 4: (Maximum 10 retries)

  • install 1.28.13 Chromium: 91.0.4472.114
  • don't run through the search onboarding and leave Google as the default browser
  • login into https://search-dev.brave.com
  • search for Brave Browser via the text field
  • the default modal should appear
  • refresh the page two times (should see the default modal appear
  • refresh again and you shouldn't see the default modal appear for this session
  • open a new tab, open https://search-dev.brave.com and try another search (shouldn't see any more modals for this session)
  • close Brave and move the time forward by 24hrs

Repeat the above. The basic logic for the above is that on desktop/android, we should only see 3 modals per session per day to a maximum of 10 in a span of four days.

Test Case 5: (Setting Brave Search as default via onboarding)

  • install 1.28.13 Chromium: 91.0.4472.114
  • Set Brave Search beta as the default SE via brave://welcome
  • login into https://search-dev.brave.com
  • search for Brave Browser via the text field
  • ensure that the default modal isn't being displayed

Test Case 6: (Setting Brave Search as default via brave://settings/search)

  • install 1.28.13 Chromium: 91.0.4472.114
  • Set Brave Search beta as the default SE via brave://settings/search
  • login into https://search-dev.brave.com
  • search for Brave Browser via the text field
  • ensure that the default modal isn't being displayed

Fallback Test Cases

  • ensured that the Enable Google Fallback mixing appears when searching for terms like brave 123 456 789
  • ensured that tapping on Dismiss correctly closes the Enable Google Fallback mixing card
    • ensured that it re-appears when another term that needs fallback is used
  • ensured that Learn more. opens https://search.brave.com/help/google-fallback
  • ensured that https://www.google.com isn't being contact when fallback is disabled
  • ensured that https://www.google.com is being contact once fallback has been enabled
Example Example Example
fallbackEnable fallbackDisabled enabledFallback

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI/skip Do not run CI builds (except noplatform)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Brave Search beta to the list of available search engines
9 participants