Default search engines by region

[mkarolin]

Fixes brave/brave-browser#893
Fixes brave/brave-browser#731

Overrides Chromium's set of prepopulated search engines with our own set and regional defaults.
Makes Qwant the default search engine for Germany and France.
Modifies search engine retrieval code to preserve the order of search engines.
Adds unit tests for search engine prepopulate data and util.

Deviates from Issue 731 in the following ways:

• Yahoo search engine is localized (e.g. in France you'll get Yahoo! France, not Yahoo)
• Yandex search engine is localized
• In Canada, in addition to Yahoo!, Yahoo! Quebec is also listed (keyword: yq)

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.
• Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Automated Test Plan

npm run test -- brave_unit_tests --filter=BraveTemplateURLPrepopulateDataTest.*
npm run test -- brave_unit_tests --filter=BraveTemplateURLServiceUtilTest.*

Test Plan:

1. Test correct default search engines list on installation

• Delete any current profile you may have
• Set your locale to U.S.
• Install Brave and run it
• Navigate to chrome://settings and check that under the Search Engine section Search engine used in the address bar is set to Google.
• Click on the pull-down and check that all engines listed in Fix compile error due to rewards resources #731 are present (Note that the order is Google, DuckDuckGo, Qwant, then the rest alphabetically).
• Click on Manage search engines and verify that the same engines are listed and have correct Keyword associated with them.

2. Test correct default search engines list for CA

• Same steps as in 1, but set your locale to Canada
• Verify that Yahoo! Quebec is also present in the list

3. Test correct default search engines list for FR, DE

• Same steps as in 1, but set your locale to France (or Germany)
• Verify that Qwant is listed on top and is set as the default search engine
• Also, verify that Yahoo! is listed as localized version (Yahoo! France or Yahoo! Deutschland)

4. Test that keyword search is working correctly

• Open a new tab
• In the omnibox type one of the keywords (e.g. :g) and press Tab
• Omnibox should display Search XXX where XXX is the search engine associated with the typed keyword

5. Test that the order of engines is preserved when default is changed

• Navigate to chrome://settings/searchEngines
• Click on ... of an engine that isn't the default and select Make default
• Verify that the engine is selected as default (bold)
• Verify that the order of engines on the page has not changed

6. Test removing search engine

• Navigate to chrome://settings/searchEngines
• Click on ... of an engine that isn't the default and select Remove from list
• Verify that the engine has been removed from the list
• Verify that the order of engines on the page has not changed

7. Test resetting settings

• Perform 5 or 6
• Navigate to chrome://settings/resetProfileSettings?origin=userclick
• In the Reset settings pop-up click Reset settings
• Navigate to chrome://settings/searchEngines and verify that the list has been restored to the original state (removed items are back in the list, default is reset)

8. Test that the engines order is preserved on second run

• Repeat test 1.
• Exit Brave
• Start Brave again and navigate to chrome://settings/searchEngines
• Verify that the search engines remain in the correct order

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]

Immediate questions:

1. I placed brave_prepopulated_engines.h/cc into brave/components/search_engines/ (as opposed to under chromium_src) because these are new files and don't overwrite anything in chromium. Because they need to be compiled as part of the search_engines static library they are both included from brave/chromium_src/components/search_engines/template_url_prepopulate_data.cc. Is there a better way to do this?

2. I defined all added search engines with the type SEARCH_ENGINE_OTHER. This type is only used in UMA (User Metrics Analysis). Since the engines are all the same type we won't get the separate data for each engine. Is this ok? If we need to gather specific data on each engine we need to update SearchEngineType enum and this may get complicated since the types need to persist in the future if Chromium updates the enum (otherwise already collected data will become invalid).

[bsclifton]

@mkarolin checking out the PR now...

1. Will take a look and let you know (I'm still ramping up on patching myself)
2. Per convo with @bridiver on Slack, we don't gather UMA; it should be disabled completely. So keeping it as SEARCH_ENGINE_OTHER should be perfect 👍

[mkarolin]

@bsclifton Thanks, I chatted with @bridiver about the patching as well. Making some changes now to override only what's necessary and minimize code duplication.

[bsclifton]

Notes as I'm testing 😄

• We'll want to update the Keyword field for each engine to match browser-laptop; ex: :g for Google, :q for Qwant, etc
• None of the favicons are downloaded for the search engines. This appears to be a problem with Chrome too
• Since we're adding all the engines (matching browser-laptop), we now have a huge list. @rebron can you work with @bradleyrichter and create an issue for restyling this? 😄 Here's a pic for reference (notice the size of the scrollbar):
  
• Set country to France and it properly defaulted to Qwant 👍
• Set country to Germany and it properly defaulted to Qwant 👍
• Otherwise defaults to Google
• I deleted all the search engines, added a few custom ones, and then reset my profile; It properly chose the expected default again 😄
• URL / details for the default engines (added by this PR) are not editable, which is expected 👍
• URLs provided for all default engines match the ones in browser-laptop 👍 (including all query params, like &t=brave)

[mkarolin]

@bsclifton from what I have seen, the favicons show up only once you have visited the site.

[mkarolin]

@bsclifton: updated keywords to match those in browser-laptop
@bridiver: cleaned up a bit as we discussed - did unit test differently, in util.cc used a different function to apply sorting which cut down patch to only that function. In template_url_prepopulate_data.cc still have 4 _Unused functions, but don't see a way around.

[diracdeltas]

None of the favicons are downloaded for the search engines. This appears to be a problem with Chrome too

For privacy reasons, please do not download the favicons remotely. In b-l we include them in the Brave package and load them from file:///.

[diracdeltas]

you can find them in app/extensions/brave/img/favicons in browser-laptop

[diracdeltas]

looks like originally clid was set by client OS? https://github.com/brave/browser-laptop/pull/5964/files#diff-1e3d00f00cc34a0805abaafa53c7d7a0R134

i'm ok with setting it to be a constant

[mkarolin]

I copied this directly from https://github.com/brave/browser-laptop/blob/master/js/data/searchProviders.js which doesn't appear to have "platformClientId" any more.

[bsclifton]

Good find, @diracdeltas - I forgot about that... I wonder if that regressed on purpose? Gonna take a peek

[bsclifton]

OK interesting- it was removed by @bridiver via brave/browser-laptop@c001b8b#diff-1e3d00f00cc34a0805abaafa53c7d7a0 during a refactor. I'll double check that we don't want platform ID and report back (having the platform could potentially be a privacy violation, right?)

[bsclifton]

update: we are good to go 😄 we don't need to pass platformClientId 👍

[diracdeltas]

question - where is the code that actually loads/renders the favicons in the list? looks like it's not in this PR

[mkarolin]

@diracdeltas

question - where is the code that actually loads/renders the favicons in the list? looks like it's not in this PR

That's Chromium functionality that I didn't touch in this PR.

It looks to me like in the UI the icon is loaded via src/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
<div class="favicon-image" style="background-image: [[getIconSet_(engine.iconURL)]]"></div>
which is in src/ui/webui/resources/js/icon.js and requests the icon via chrome://favicon/ URL (e.g. size/16@2x/https://www.qwant.com/favicon-32.png?1538404864893) . Then it looks like FaviconSource::StartDataRequest and favicon::FaviconService do the actual work of fetching the icon.

[diracdeltas]

@mkarolin thanks. do you know at what point Chromium makes a fetch to the favicon URL specified in PrepopulatedEngine (ex: https://github.com/brave/brave-core/pull/516/files#diff-7e5cc9096d62b4d079ea4a59f4d66d1bR18)?

[mkarolin]

@diracdeltas:

• The omnibox attempts to get the favicon for the default search provider on startup,
• When you go to chrome://settings/searchEngines page, it tries to get the favicon for each engine.

The process of getting a favicon is the same in either case:

• if the favicon URL ends in .ico, it calls FaviconService::GetRawFavicon, which in turn goes to HistoryService and HistoryBackend and asks ThumbnailDatabase for the icon. If ThumbnailDatabase doesn't have the icon, then nothing is returned and the searchEngines page uses the default icon (empty page);
• if the favicon URL ends on anything else (e.g. .png) then the prepopulated pages of TopSites is checked and if not found there, FaviconService::GetRawFaviconForPageURL is called. This looks more involved but ends up in HistoryBackend::GetFaviconsFromDB, which also looks for a suitable icon in the ThumbnailDatabase.

So, it looks to me like at no point it actually attempts to fetch the favicon "live". This is consistent with the observed behavior on the search engines settings page that the icon for a search engine only appears after you'd visited that search engine (and, likely, only if the current search engine favicon URL matches what we have in our prepopulated engines). Once the icon is in the thumbnails database, then it is shown.

[bsclifton]

@mkarolin I have confirmed your findings above 😄 Basically, it tries to load a favicon (using CSS) via:
url("chrome://favicon/size/16@1x/iconurl/https://fav-icon-url-from-search-providers-here")

This lookup will hit the Favicons SQLite DB located at:
(macOS) ~Library/Application Support/BraveSoftware/Brave-Browser-Development/Default
(Windows) %APPDATA%/BraveSoftware/Brave-Browser-Development/Default

If an icon is found, it shows. If not, it doesn't show.

I confirmed these icons don't show after deleting the Favicons file 👍

[mkarolin]

Thanks @bsclifton. If that's the case then favicons are probably not a privacy concern.
Certainly, it is visually more appealing to have them pre-populated when the user first sees the search engines settings page. However, I don't know if this is a requirement for this PR or if we can create a separate issue for it. We may be able to follow Chromium's example in LargeIconService which fetches certain icons and then calls FaviconService::SetOnDemandFavicons (except we'd load the icons from resources instead).

[bsclifton]

I ran through each test in the test plan: works perfectly! Code + unit tests looks great to me too (tests pass). I verified the findings about favicon URL (page will not make outbound requests, which is great). Awesome job on this one, @mkarolin! 😄👍

[mkarolin]

master: b519219
0.56.x: ab195ec
0.55.x: 97e86b6