Closed Bug 1590860 Opened 5 years ago Closed 5 years ago

De-duplicate SearchService._buildSortedEngineList and SearchService.getDefaultEngines and fix getDefaultEngines sort order

Categories

(Firefox :: Search, task, P3)

Product:
Firefox
Component:
Search
Type:
task
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 75
Iteration:
75.2 - Feb 24 - Mar 8
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(5 files)

Bug 1590860 - Stop skipping tests in test_getDefaultEngines.js. r?daleharvey
(deleted), text/x-phabricator-request
Details
Bug 1590860 - Add tests for checking the sort order coming from SearchService.getEngines. r?daleharvey
(deleted), text/x-phabricator-request
Details
Bug 1590860 - Rename test_getDefaultEngines.js to test_sort_orders.js to better reflect what it is testing. r?daleharvey
(deleted), text/x-phabricator-request
Details
Bug 1590860 - Default engines should be sorted after distribution engines. r?daleharvey
(deleted), text/x-phabricator-request
Details
Bug 1590860 - De-duplicate parts of SearchService._buildSortedEngineList and getDefaultEngines to reduce code and fix getDefaultEngines sort order. r?daleharvey
(deleted), text/x-phabricator-request
Details

These two functions currently have a lot of code that is very similar:

https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/toolkit/components/search/SearchService.jsm#2034-2097
https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/toolkit/components/search/SearchService.jsm#2188-2246

Additionally getDefaultEngines appears to return the wrong orders when distribution settings are present. It attempts to set the order, then the later writes from processing _searchOrder can overwrite the distribution settings.

This is shown in the new test I'm adding in bug 1577733 (test_getDefaultEngines.js).

It would also likely not correctly set the default engines to be first and second, as happens with _buildSortedEngineList.

We should attempt to unify these two functions, something along the lines of:

  • Create another function to get the engines sorted as per the configuration.
    • Prefer the _buildSortedEngineList structure as that seems most correct.
  • Add more tests for _buildSortedEngineList (via the appropriate APIs) to cover:
    • Distribution engines + Default engines.

I split out the localeCompare changes to bug 1590896, as they're simple on their own, and can be done slightly sooner to enable other work to happen.

Depends on: 1590896
Blocks: 1615309
Assignee: nobody → standard8
Iteration: --- → 74.2 - Jan 20 - Feb 09
Iteration: 74.2 - Jan 20 - Feb 09 → 75.2 - Feb 24 - Mar 8

Default engines should be after distribution engines. Although all the distributions list the default engine as the first engine, if we ever specify a separate private engine, this may get inserted into the second position, which distributions wouldn't normally expect, hence breaking their orders.

Depends on D64667

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17cf596fb222 Stop skipping tests in test_getDefaultEngines.js. r=daleharvey https://hg.mozilla.org/integration/autoland/rev/2cd5f484791a Add tests for checking the sort order coming from SearchService.getEngines. r=daleharvey https://hg.mozilla.org/integration/autoland/rev/cf570b3aaf78 Rename test_getDefaultEngines.js to test_sort_orders.js to better reflect what it is testing. r=daleharvey https://hg.mozilla.org/integration/autoland/rev/d9146c992830 Default engines should be sorted after distribution engines. r=daleharvey https://hg.mozilla.org/integration/autoland/rev/ba5cade70de8 De-duplicate parts of SearchService._buildSortedEngineList and getDefaultEngines to reduce code and fix getDefaultEngines sort order. r=daleharvey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: