Allow for assigning of default search engine based on country #15154

bsclifton · 2018-09-05T06:59:29Z

Fixes #14647

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).
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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

(perform steps on Windows, macOS, and Linux)

Changing the country

macOS

Settings > Language & Region

Windows

Open up settings (on Windows 10, click the notifications icon in bottom right and then pick All settings)
Pick "Time & Language"
Pick "Region & Language"

You'll also need to go into Control Panel > Region and set the Format

Linux

Push the Windows key and search for region (or language)
Pick Language Support
Install other languages as needed
Pick the appropriate language (all choices should show country)
reboot for changes to take effect

Setup

Check out source code; do a fresh rebuild
Either Delete or move out of the way your brave-development profile directory
Uncomment these two lines: https://github.com/bsclifton/browser-laptop/blob/222c500977f77a6de63eaa587c4cc11f5d820505/js/constants/config.js#L48-L49

Test default (new install - country doesn't match any config entry)

Delete your brave-development profile directory
Set your country to USA
Launch browser (npm run watch / npm start)
Verify default search is Google
Open a new PRIVATE tab and ensure DuckDuckGo option DOES show on private tab. Try a search and verify it uses the proper engine
Open a new PRIVATE TOR tab and ensure DuckDuckGo DOES show (is mentioned). Try a search and verify it uses the proper engine
Go to about:preferences#search
Verify your default search engine is Google
Verify the "Private tab" options show up on the page
Exit Brave
Set your country back to original setting

Test country matches a config entry (new install)

Delete your brave-development profile directory
Set your country as France
Re-launch browser
Verify default search is GitHub
Open a new PRIVATE tab and ensure DuckDuckGo option DOES NOT show on private tab. Try a search and verify it uses the proper engine
Open a new PRIVATE TOR tab and ensure DuckDuckGo DOES NOT show (is NOT mentioned). Try a search and verify it uses the proper engine
Go to about:preferences#search
Verify your default search engine is GitHub
Verify the "Private tab" options DO NOT show up on the page
Change your default to Google
Verify that "Private tab" options DO show up
Exit Brave
Set your country back to original setting

Ensure default not changed for existing profile

Delete your brave-development profile directory
Set your country as USA
Launch browser
Verify default search is Google
Exit Brave
Set your country as France
Re-launch brave
Ensure default search engine is still Google (no change made)
Exit Brave
Set your country back to original setting

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

Depends on brave/muon#647 being merged / available Fixes #14647 - Don't show private tab options on search tab (preferences) if default browser was set - Re-show DDG after user manually changes search engine - Use default search engine for private/tor tabs when search engine is defaulted. If user overrides, it'll fall back to DDG

bsclifton · 2018-09-05T20:44:46Z

@darkdh made a few more changes - ready for re-review 😄

LaurenWags · 2018-09-06T22:41:14Z

Test Cases + fix for context menu search lgtm on macOS 👍

diracdeltas · 2018-09-06T23:09:18Z

with this PR there seems to be no way to use a different search engine in Tor mode than for regular browsing. is that intentional?

bsclifton · 2018-09-06T23:26:40Z

@diracdeltas that is correct- it'll use the default search engine. However, if the user modifies their search engine preferences in any way, it'll change back to the default behavior (ex: DuckDuckGo is default for Tor- and DDG can also be default (up to user) for regular private tabs)

diracdeltas · 2018-09-06T23:30:29Z

@bsclifton i see. i tried changing my default search engine to google, and the options for Use DDG in (Tor) Private Tabs appeared in about:preferences. However I did not see the switches in about:newtab in private or tor tabs until i restarted.

bsclifton · 2018-09-06T23:38:01Z

@diracdeltas that is correct (and that is known; apologies for not calling it out). I wasn't sure if that was worth fixing or not

kjozwiak · 2018-09-07T01:53:46Z

Went through the above STR that @bsclifton mentioned using Ubuntu 18.04 x64. Results:

Case 1 - Test default (new install - country doesn't match any config entry) - PASSED
Case 2 - Test country matches a config entry (new install) (using German) - PASSED
Case 2 - Test country matches a config entry (new install) (using France) - PASSED
Case 3 - Ensure default not changed for existing profile (using German) - PASSED
Case 3 - Ensure default not changed for existing profile (using France) - PASSED

Issues:

Only issue that I managed to find was the one @diracdeltas mentioned above regarding the DDG snippet not appearing under PT when the search engine is switched. However, once you either restart or toggle Use DDG by default for search in Private Tabs, the snippet will appear under both PT and Tor PT.

bsclifton · 2018-09-07T05:11:21Z

@kjozwiak @diracdeltas I fixed the issue with private & tor tabs not immediately re-showing the DDG options (ended up being a lot easier than I had thought 😄). Ready for re-review! 👍

@kjozwiak

- only change value if going from false=>true (should only be called once) - private tab options are reflected immediately on new private/tor tabs Auditors: @kjozwiak, @diracdeltas

@darkdh

This has the new electron.app.getCountryName functionality (with Chromium 68) Auditors: @darkdh

bsclifton · 2018-09-07T06:57:37Z

@kjozwiak @LaurenWags @srirambv I also updated this to use Chromium 68 (instead of Chromium 69). Can we give the methods a spot check? 😄

kjozwiak · 2018-09-07T10:36:03Z

Awesome, thanks @bsclifton! We'll recheck the above cases on all the platforms with the new updated PR that uses C68 instead of C69 👍

srirambv · 2018-09-07T14:22:53Z

Still an issue on Windows. Verified setting France as region and system format still shows as Google. Same for Germany as well.

Issue reproduced by @GeetaSarvadnya as well

Recording of the issue: https://drive.google.com/open?id=1eXV-j8An1efM44rQUyK_-Y_-PNPIVmrr

LaurenWags · 2018-09-07T14:32:21Z

Using Test Cases/STR from description, verified on macOS 10.12.6 x64 with commit 7d5264d

Results:

Case 1 - Test default (new install - country doesn't match any config entry) - PASSED
Case 2 - Test country matches a config entry (new install) (using Germany) - PASSED
- Verified context menu issue (found previously) still works as expected
- Verified Private & Tor tabs immediately re-show the DDG options
- Verified muon version 8.0.10
Case 2 - Test country matches a config entry (new install) (using France) - PASSED
- Verified context menu issue (found previously) still works as expected
- Verified Private & Tor tabs immediately re-show the DDG options
- Verified muon version 8.0.10
Case 3 - Ensure default not changed for existing profile (using Germany) - PASSED
Case 3 - Ensure default not changed for existing profile (using France) - PASSED

kjozwiak · 2018-09-07T16:11:24Z

Still an issue on Windows. Verified setting France as region and system format still shows as Google. Same for Germany as well.

Issue reproduced by @GeetaSarvadnya as well

Recording of the issue: https://drive.google.com/open?id=1eXV-j8An1efM44rQUyK_-Y_-PNPIVmrr

Same results on my machine as the above. Seems like this PR doesn't work on Win for some reason but works fine on macOS & Ubuntu. @srirambv also mentioned that he tried changing the language of the OS but that didn't work either.

btlechowski · 2018-09-07T16:33:09Z

Tested on Ubuntu 17.10 on commit 7d5264d

Default Search engine - new install - PASS

Search engine for Germany/France - new install
- normal tab URL search - PASS
- normal tab Context Menu search - PASS
- private tab URL search - PASS
- private tab Context Menu search - PASS
- private tab duckduckgo not shown on new tab - PASS
- private tor tab URL search - PASS
- private tor tab Context Menu search - PASS

Search engine for Germany/France - new install - changed search engine to google in about:preferences#search
- normal tab URL search - PASS (google)
- normal tab Context Menu search - PASS (google)
- private tab URL search - PASS (google)
- private tab Context Menu search - PASS (google)
- private tab duckduckgo shown on new tab - PASS
- private tor tab URL search - PASS (duckduckgo)
- private tor tab Context Menu search - PASS (duckduckgo)

Search engine for Germany/France - update
- normal tab URL search - PASS (google)
- normal tab Context Menu search - PASS (google)
- private tab URL search - PASS (google)
- private tab Context Menu search - PASS (google)
- private tab duckduckgo shown on new tab - PASS
- private tor tab URL search - PASS (duckduckgo)
- private tor tab Context Menu search - PASS (duckduckgo)

bsclifton · 2018-09-07T22:32:15Z

@srirambv @kjozwiak @GeetaSarvadnya fixed the issue on Windows! Chrome 68 was putting a unicode null terminator (\u0000) at the end of the string, which caused a comparison problem. 2366ee1 removes this with RegEx

We didn't see it with previous code because that was using Chromium 69 (which must have changed this)

srirambv · 2018-09-07T23:13:07Z

Verified the following scenarios on Windows
Region: France

Clean install from source
Set System Country and Format to France
Open a new tab and search, uses YouTube as default search engine
Highlight a text and search from context menu, opens a YT page with highlighted term as search term
Open a private tab and search, uses YT as default search engine, Context menu search used YT as well in a new private tab
Open a Tor private tab and search, uses YT as default search engine, Context menu search in Tor PT opens a new Tor tab with YT as default search engine
Open about:preferences#search, no private tabs search options is shown
Change default search engine to other, private tab search options shows up
Change back default search engine to YT, private tab search engine is retained

Region: Germany

Clean install from source
Set System Country and Format to Germany
Open a new tab and search, uses GitHub as default search engine
Highlight a text and search from context menu, opens a GH page with highlighted term as search term
Open a private tab and search, uses GH as default search engine, Context menu search used GH as well in a new private tab
Open a Tor private tab and search, uses GH as default search engine, Context menu search in Tor PT opens a new Tor tab with GH as default search engine
Open about:preferences#search, no private tabs search options is shown
Change default search engine to other, private tab search options shows up
Change back default search engine to GH, private tab search engine is retained

Region: US

Clean install from source
Set system country and format to US
Normal tab uses Google as search engine. Context menu search opens google in a new tab
Private tab uses Google as search engine. Context menu search opens google in a private tab
Tor PT uses Google as search engine. Context menu search opens google in a new Tor private tab
Open about:preferences#search, private tabs search options is shown
Change default search engine to other, private tab search options is retained

srirambv

++ works on Windows now

Allow for assigning of default search engine based on country

bsclifton · 2018-09-08T05:56:46Z

master f0c9489
0.24.x 4522810
0.23.x 31c46fb

Allow for assigning of default search engine based on country

bsclifton added this to the 0.23.x w/ Chromium 69 (BETA Channel) milestone Sep 5, 2018

bsclifton self-assigned this Sep 5, 2018

bsclifton requested review from bridiver, kjozwiak, darkdh and LaurenWags September 5, 2018 06:59

darkdh previously approved these changes Sep 5, 2018

View reviewed changes

bsclifton dismissed darkdh’s stale review via 0412f88 September 5, 2018 19:15

bsclifton requested a review from diracdeltas September 7, 2018 05:12

bsclifton added 3 commits September 6, 2018 23:55

Fix context menu case found by @LaurenWags

bb66407

Fix private tab logic after search engine is changed

d742041

- only change value if going from false=>true (should only be called once) - private tab options are reflected immediately on new private/tor tabs Auditors: @kjozwiak, @diracdeltas

Update to Muon 8.0.10

7d5264d

This has the new electron.app.getCountryName functionality (with Chromium 68) Auditors: @darkdh

Strip unicode null terminator from returned string

2366ee1

srirambv approved these changes Sep 7, 2018

View reviewed changes

bsclifton merged commit f0c9489 into brave:master Sep 8, 2018

bsclifton deleted the get-country-name branch September 8, 2018 05:55

bsclifton added a commit that referenced this pull request Sep 8, 2018

Merge pull request #15154 from bsclifton/get-country-name

4522810

Allow for assigning of default search engine based on country

bsclifton added a commit that referenced this pull request Sep 8, 2018

Merge pull request #15154 from bsclifton/get-country-name

31c46fb

Allow for assigning of default search engine based on country

bsclifton modified the milestones: 0.23.x w/ Chromium 69 (BETA Channel), 0.23.x Release 6 Sep 8, 2018

bsclifton added a commit that referenced this pull request Sep 8, 2018

Merge pull request #15154 from bsclifton/get-country-name

ad0cf61

Allow for assigning of default search engine based on country

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for assigning of default search engine based on country #15154

Allow for assigning of default search engine based on country #15154

bsclifton commented Sep 5, 2018 •

edited

Loading

bsclifton commented Sep 5, 2018

LaurenWags commented Sep 6, 2018

diracdeltas commented Sep 6, 2018

bsclifton commented Sep 6, 2018

diracdeltas commented Sep 6, 2018

bsclifton commented Sep 6, 2018

kjozwiak commented Sep 7, 2018 •

edited

Loading

bsclifton commented Sep 7, 2018

bsclifton commented Sep 7, 2018

kjozwiak commented Sep 7, 2018

srirambv commented Sep 7, 2018

LaurenWags commented Sep 7, 2018 •

edited

Loading

kjozwiak commented Sep 7, 2018 •

edited

Loading

btlechowski commented Sep 7, 2018

bsclifton commented Sep 7, 2018

srirambv commented Sep 7, 2018 •

edited

Loading

srirambv left a comment

bsclifton commented Sep 8, 2018

Allow for assigning of default search engine based on country #15154

Allow for assigning of default search engine based on country #15154

Conversation

bsclifton commented Sep 5, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Changing the country

macOS

Windows

Linux

Setup

Test default (new install - country doesn't match any config entry)

Test country matches a config entry (new install)

Ensure default not changed for existing profile

Reviewer Checklist:

bsclifton commented Sep 5, 2018

LaurenWags commented Sep 6, 2018

diracdeltas commented Sep 6, 2018

bsclifton commented Sep 6, 2018

diracdeltas commented Sep 6, 2018

bsclifton commented Sep 6, 2018

kjozwiak commented Sep 7, 2018 • edited Loading

bsclifton commented Sep 7, 2018

bsclifton commented Sep 7, 2018

kjozwiak commented Sep 7, 2018

srirambv commented Sep 7, 2018

LaurenWags commented Sep 7, 2018 • edited Loading

kjozwiak commented Sep 7, 2018 • edited Loading

btlechowski commented Sep 7, 2018

bsclifton commented Sep 7, 2018

srirambv commented Sep 7, 2018 • edited Loading

srirambv left a comment

Choose a reason for hiding this comment

bsclifton commented Sep 8, 2018

bsclifton commented Sep 5, 2018 •

edited

Loading

kjozwiak commented Sep 7, 2018 •

edited

Loading

LaurenWags commented Sep 7, 2018 •

edited

Loading

kjozwiak commented Sep 7, 2018 •

edited

Loading

srirambv commented Sep 7, 2018 •

edited

Loading