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

Another instance of DuckDuckGo is added to search engines on upgrade for AU/NZ/IE locale #6453

Closed
btlechowski opened this issue Oct 12, 2019 · 8 comments

Comments

@btlechowski
Copy link

btlechowski commented Oct 12, 2019

Follow up to #6187
Users who have set custom search engine to duckduckgo in AU/NZ/IE will have another instance of DuckDuckGo added to search engines upon upgrade to 0.70.x

Steps to Reproduce

  1. Set system locale to AU
  2. Install 0.69.x
  3. Set default search engine to duckduckgo in chrome://settings/searchEngines
  4. Upgrade to 0.70.x
  5. Check list of search engines in chrome://settings/searchEngines

Actual result:

DuckDuckGo is listed twice
Another instance of DuckDuckGo is added to search engines
The search strings are different from each other.
image

Expected result:

DuckDuckGo is listed once

Reproduces how often:

Always

Brave version (brave://version info)

Brave 0.70.112 Chromium: 77.0.3865.90 (Official Build) beta (64-bit)
Revision 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
OS Ubuntu 18.04 LTS

cc @brave/legacy_qa @rebron @bsclifton

@LaurenWags
Copy link
Member

Reproduced on macOS.

Also, I have a 3rd DDG listed - it's in Other Search Engines section.

Default DDG:
Screen Shot 2019-10-14 at 10 08 19 am

Added DDG entry to Default search engines section on upgrade:
Screen Shot 2019-10-14 at 10 09 01 am

Additional DDG entry under Other search engines:
Screen Shot 2019-10-14 at 10 09 42 am

Brave 0.70.115 Chromium: 77.0.3865.90 (Official Build) beta (64-bit)
Revision 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
OS macOS Version 10.13.6 (Build 17G5019)

@LaurenWags
Copy link
Member

Occurs for DE as well if DDG was set to be the default SE prior to upgrade:
Screen Shot 2019-10-14 at 15 43 53

Brave 0.70.115 Chromium: 77.0.3865.90 (Offizieller Build) beta (64-Bit)
Überarbeitung 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
Betriebssystem macOS Version 10.13.6 (Build 17G5019)

@LaurenWags
Copy link
Member

something to note - the 'Default' DDG no longer has the shortcut as :d, the new one does. Not sure if that's part of the problem?

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Oct 15, 2019

Reproduced in Windows 10 x64- 0.70.113. DDG was set in 069.x prior upgrade, after upgrade to 0.70.113 another instance of DDG added in the search engines

image

@bsclifton bsclifton self-assigned this Oct 15, 2019
@bsclifton bsclifton added this to the 0.70.x - Beta milestone Oct 15, 2019
@bsclifton
Copy link
Member

Although it's confusing, this is following the proper behavior. So it's not a bug- it just looks weird.

Default search engines will always show the 5 pre-populated engines, no matter what. Your default will also show in there too (ex: if you set youtube.com as your default, it moves itself from Other search engines to Default search engines and puts (Default) next to it)

Will try to think about a way to resolve this- maybe we can not add pre-populated entries if the default is basically the same (search keyword matches or most URL matches)?

@bsclifton bsclifton removed the bug label Oct 17, 2019
@bsclifton
Copy link
Member

bsclifton commented Oct 17, 2019

We can keep this issue open and in the milestone for now - but my proposal would be to leave it as-is (as awkward as it looks)

Here's why it shows on brave://settings/searchEngines...

  • When an engine is set as the default, it's added to Default/Secure Preferences
  • The code is pulling prepopulated engines for the given region and then merging in the default option

Possible solutions include:

  • Creating a template URL service instance and removing the duplicate entry from Default/Web Data
  • Filtering out the duplicate entry on each call to get prepopulated engines

Both solutions require the code in chromium_src/components/search_engines/template_url_prepopulate_data.cc to either:

  1. fetch the profile in order to get a reference to the template url service. I ran into a race condition which causes the app to crash (profile has a lock on it, at the time it's requesting access).
  2. have a different interface where the caller passes default engine ID (breaking the interface that we're overriding from Chromium)

I'm not sure this one is worth fixing, given the extra complexity it would introduce
cc: @rebron

@rebron rebron added the priority/P5 Not scheduled. Don't anticipate work on this any time soon. label Oct 18, 2019
@bsclifton
Copy link
Member

Removing from milestone, per the above notes

@bsclifton bsclifton removed this from the 0.70.x - Release milestone Oct 21, 2019
@bsclifton bsclifton removed their assignment Oct 21, 2019
@bsclifton
Copy link
Member

Given that this isn't a bug, I'm going to close as wontfix (notes above describing)

@bsclifton bsclifton added closed/wontfix and removed priority/P5 Not scheduled. Don't anticipate work on this any time soon. labels Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants