356289 - No way to unhide an extension search engine that has been removed

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Mike Kaply [:mkaply]]

If you have an extension that has search engines as a part of it, and then you remove that search engine via the search engine UI, there is no way to get it back.

Restore Defaults should restore extension added search plugins as well.

Comment 1

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

This should be as simple as removing the second condition at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/search/nsSearchService.js&rev=1.86#2737

Comment 2

[Mike Kaply [:mkaply]]

That's part of it. The button also has to be enabled in this scenario as well.

What is the difference between the restoreDefaultEngines in engineManager.js vs. nsSearchService.js?

Which one is called when you click the button?

Isn't the one you are indicating the one called from the idl, not the one called by the UI?

Comment 3

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

The one in the search service is called by the one in the engine manager.

Comment 4

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

Attachment: patch

I want to double check this when I'm more awake, but this should do.

Comment 5

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

Attachment: patch

This fixes this bug, by including extension-shipped plugins in the set of "default" plugins. I also made sure that the "extra" code added in bug 347364 takes effect when determining the order of the default engines, and fixed a strict warning in engineManager.js.

Comment 6

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

Attachment: patch

This one also users the new "default" (appdir- & extension-shipped) attribute for determining whether we can use the advanced search params. This makes it easier to do engine overrides for default-shipped plugins (no loss of functionality).

Comment 7

[Asaf Romano (gone)]

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

OK code-wise. I would argue that the expected behavior of the "Restore Defaults" button is to _hide_ extensions' engines.

Comment 8

[Mike Kaply [:mkaply]]

(In reply to comment #7)
> (From update of attachment 243949 [details] [diff] [review])
> OK code-wise. I would argue that the expected behavior of the "Restore
> Defaults" button is to _hide_ extensions' engines.

Then  maybe the button should be renamed?

"Show hidden search engines" 

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 243949 [details] [diff] [review] [details])
> > OK code-wise. I would argue that the expected behavior of the "Restore
> > Defaults" button is to _hide_ extensions' engines.
> 
> Then  maybe the button should be renamed?
> 
> "Show hidden search engines"

Except that we don't tell users that they're just "hiding" the engine when they remove an extension- or app-shipped engine, so expecting them to make the connection probably isn't a good idea. I think the current patch really is what we want. In most cases, extension-shipped engines are going to be from the CCK or other "installed by default" branding-pack type extensions, so treating their search engines as "default" makes sense. In the less common case where the user manually installs an extension that ships search engines, we should perhaps make it clearer that engines were added somehow (so they know they can uninstall the extension to remove them), but I'm not sure that that's very easy to do.

Comment 10

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

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

mconnor: see comment 5 through comment 9.

Comment 11

[Daniel Veditz [:dveditz]]

Will this bug also fix the inability to re-add/recover additional search engines not added through an extension?

I must have added a bunch of search engines from addons at some point, then pared down the list by removing them through the UI. Now I want to re-add some of them and am told I can't, it already exists. The "restore defaults" button remains disabled, these weren't default engines to start with.

Let me know if that needs to go in a separate bug. It's extremely sucky, I think my only option is to delete the sqlite file and re-add all my engines.

Comment 12

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

That should be in a seperate bug, yeah. I don't understand how that would happen, since engines added from AMO should be completely removed when youu click "remove" - we don't even fall back on hiding them if that fails. In any case, deleting search.sqlite won't cause you to have to re-add all your search engines, the engine description files are stored seperately from the metadata. You should probably zip up the relevant files (<profile>/searchplugins and <profile>/search.sqlite) before deleting them and attach them to the bug, if you don't mind.

Comment 13

[Mike Connor [:mconnor]]

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

Yeah, I think this is the right thing, especially given this is how we do partner builds...

Comment 14

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

Landed that patch, with an extra comment about why we're doing ordering stuff in getDefaultEngines() because I had forgotten and had to figure it out again.

mozilla/browser/components/search/content/engineManager.js 	1.14
mozilla/browser/components/search/nsIBrowserSearchService.idl 	1.18
mozilla/browser/components/search/nsSearchService.js 	1.95