348737 - restoring defaults in "manage search engines..." results in unordered list

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Chris Beard]

If you use the "Manage Search Engines..." UI to delete a search engine and then click on "Restore Defaults" it places the deleted search engine at the top of the list. Whereas, it should restore the default set of search engines in the same order as shipped.

Steps to reproduce:
1. Note that search engines are ordered Google, Yahoo!, Amazon.com, Creative Commons, eBay in the drop down.
2. Open the "Manage Search Engines..." UI and delete "Creative Commons".
3. Click on "Restore Defaults".
4. Note that the search engines are now ordered Creative Commons, Google, Yahoo!, Amazon.com, eBay.

Comment 1

[Michael Wu [:mwu]]

Known problem. The restored engines are in order with respect to each other, however.

Comment 2

[Chris Beard]

(In reply to comment #1)
> Known problem. The restored engines are in order with respect to each other,
> however.

Is there a bug filed on this already? I disagree that this behaviour is acceptable . It is confusing to the end-user and does not "restore" you to a known good state. It also does not meet the product requirement. We should correct this, and it should block release.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Won't block beta2, should be fixed by final. Can we use the fact that we have the order in which these searchplugins are first created to help here?

Comment 4

[Michael Wu [:mwu]]

(In reply to comment #2)
> (In reply to comment #1)
> > Known problem. The restored engines are in order with respect to each other,
> > however.
> 
> Is there a bug filed on this already?
Nope.

> I disagree that this behaviour is
> acceptable .
I never said it was. I was merely saying that there is some ordering done, so it is possible to restore the default order if you just delete everything.

(In reply to comment #3)
> Won't block beta2, should be fixed by final. Can we use the fact that we have
> the order in which these searchplugins are first created to help here?
> 
We already have the ordering info - we merely have to use it. I had some issues trying to get that to work in bug 343707, so that was taken out in order to get some sort of restore defaults functionality landed in the engine manager for beta 2. The safe mode functionality also needs the same change.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Michael: can the "restore" function do a second pass through the list once it's restored the defaults, and produce the correct engineMoveOps to restore the original engine order (defaults at the top)? Obviously not the most efficient algorithm (potentially resulting in a lot of redundant moveOps), but if it works...

Comment 6

[Mike Connor [:mconnor]]

Welcome back Gavin!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This patch:
-Fixes getDefaultEngines to use getLocalizedPref. Calling getLocalizedPref in a loop isn't optimal, because it re-gets the prefservice on each call, but since that loop won't occur very often and isn't perf-critical, I favored readability
-Fixes both moveEngine functions to do nothing if the engine doesn't actually need to move (to ensure that "restoring" one engine in an already sorted list won't move and fire notifications for all engines in that list)
-Makes restoreDefaultEngines move already present default engines to their original positions when restoring

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 235578 [details] [diff] [review]
patch

ok.

Is there a reason we don't cache the prefbranch in this impl?  its not that much work, and it helps perf a great deal

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Is there a reason we don't cache the prefbranch in this impl?  its not that
> much work, and it helps perf a great deal

Cache it where, in a global? I'd rather not hold on to it for the life of the app. This could certainly be optimized otherwise, but I don't think it's really worth the trouble for the trivial gain.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/content/engineManager.js 	1.12
mozilla/browser/components/search/nsSearchService.js 	1.75

Comment 12

[Mike Schroepfer]

Comment on attachment 235578 [details] [diff] [review]
patch

a=schrep/beltnzer for drivers.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/nsSearchService.js 	1.1.2.63
mozilla/browser/components/search/content/engineManager.js 	1.1.2.14

Comment 14

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060827 Minefield/3.0a1 ID:2006082718 [cairo]

search-box missing, manage search engines no response.
the checkin cause this ?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> search-box missing, manage search engines no response.
> the checkin cause this ?

Nope, see bug 350424.

Comment 16

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b2) Gecko/20060828 BonEcho/2.0b2.

Deleted engines are now restored to their original order.

(If we're going to consider reordering engines making things non-default, shouldn't we activate the "Restore Defaults..." button if someone reorders an engine?)

Comment 17

[Chris Beard]

Awesome! Thanks!