556437 - nsSearchService.restoreDefaultEngines should also restore the default order

Status: NEW

(Firefox :: Search, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.3pre) Gecko/20100324 Namoroka/3.6.3pre ID:20100324033852

While implementing Mozmill tests for the search component I have noticed that the restoreDefaultEngines method of the SearchService doesn't restore the default order of the engines. There is also no other method available which offers that functionality in an easy way. Only the "Restore Defaults" button inside the Engine Manager resets everything. Means the sorting and hidden status of the engines get reset.

The code live here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/engineManager.js#396

As Gavin told on IRC this logic should better be moved to the SearchService itself.

Comment 1

[:shell escalante]

defaults get restored and new search UI this should go away.  code in pref panel we believe restore order - but need to restore order.  low/no visible user impact

Comment 2

[Mark Banner (:standard8)]

When reviewing bug 1499095, I noticed that preferences is spinning its own reset engines loop, when really it should be using the search service's restoreDefaultEngines - which is exactly what this bug is about.

I think we should change this code and the code being added in bug 1499095 to be contained within SearchService.restoreDefaultEngines.

The revised function will:

1. unhide any application provided engines, except for those defined as hidden by enterprise policies
2. restore the order of the search engines

The first part can be done by moving the enterprise code across and extending the existing loop within SearchService.restoreDefaultEngines.

For the second part, I think we should reset the user's useSavedOrder setting and use that to our advantage. The setting is there to indicate that we should use the order from the saved settings, but resetting that attribute here as well is a reasonable thing to do IMO.

Doing it this way also means we also don't have to mess around with reordering the engines - we can use the existing code to set the order. So this would look something like:

setAttribute(useSavedOrder, false);
this.__sortedEngines = null; // Note: two underscores
// Maybe call this._saveSortedEngineList(false) here (see below)
for (let engine of this._sortedEngines) { // Note: one underscore
  // Notify engine modified (just in case, unless there's an easy way to skip notifications we know we don't need).
}

I put the maybe call this._saveSortedEngineList(false) in as a comment as I think we should probably update the saved orders for the engines in settings even if we're not actively using them - I'm a little concerned that if we don't we might have old orders saved, and so if the user subsequently moves an engine, we might get an incorrect order.

The first two lines could also potentially be moved into _saveSortedEngineList.