340868 - Offer to re-add an autodiscovered engine if user removes it

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Pam Greene]

If you're viewing a page with an autodiscovered <link rel...> OpenSearch engine that you've already added to your searchbox list, you correctly won't be offered the chance to add it to that list (once bug 340122 is fixed).

However, if you remove that engine from your list using the search engine manager, you still won't be offered the chance to re-add it, and the UI won't change to show that an autodiscovered engine is available, until you reload the page.

Comment 1

[Pam Greene]

Attachment: Updates list of offered engines whenever one is added or removed

I'd appreciate your having a look at this, Gavin.  I'll get module owner approval too.

Comment 2

[Pam Greene]

Attachment: Same code, now with more comments

Comment 3

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

Would it be terrible for performance to just let the browser keep one array of the available engines, and have rebuildPopupDynamic filter out already-installed ones (by asking the search service) when it shows the popup? That'd make this code a lot simpler, but maybe the xpconnect round-tripping and frequent lookups onpopupshowing isn't such a good idea. Alternatively, maybe the search bar could keep an array of visible engine titles, kept up to date with the engine-removed/added notifications, and then use that in rebuildPopupDynamic to filter the engines added to the popup. That's be cleaner, I think, but maybe I'm missing something.

Comment 4

[Pam Greene]

(In reply to comment #3)
> Would it be terrible for performance to just let the browser keep one array of
> the available engines, and have rebuildPopupDynamic filter out
> already-installed ones (by asking the search service) when it shows the popup?

That's a thought.  Since it only has to ask about autodetected engines, which aren't common, the performance hit wouldn't be too bad.  One or two queries per onpopupshowing, but only for pages with any <link> engines at all.

> Alternatively, maybe
> the search bar could keep an array of visible engine titles, kept up to date
> with the engine-removed/added notifications, and then use that in
> rebuildPopupDynamic to filter the engines added to the popup.

I'm not sure about the benefit of duplicating the whole list of engine titles.  Adding and removing engines won't be common either, but the memory would be used all the time.

Comment 5

[Pam Greene]

(In reply to comment #3)
> Would it be terrible for performance to just let the browser keep one array of
> the available engines, and have rebuildPopupDynamic filter out
> already-installed ones (by asking the search service) when it shows the popup?
> That'd make this code a lot simpler [...]

I started into this, but I don't think it'll make the code much simpler, if at all.  The problem is that the search-button "addengines" attribute needs to be updated so it shows the right UI (a red magnifying glass, currently) depending on whether there are engines available to be added.  That means that search.xml still needs to observe engine-added and engine-removed, and re-filter the list on each of those; and updateSearchButton() in browser.js also has to do the same filtering.

Given all that, I think it's actually simpler to let the filtering happen when the <link> is detected and only have to update it when its status changes -- that is, to stick with the current patch.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

So, does this mean that if I do this:

- Remove Google (which is part of the default list that ships with the app)

that "Add Google" will show up in the menu?

Comment 7

[Pam Greene]

No, this is only for autodiscovered engines, ones with <link rel...> tags on the current page.  It means that if you go to www.technorati.com, add the engine that shows up in the list, then remove it using the Engine Manager, you'll be offered the chance to add it again without having to reload the page.

Comment 8

[Pam Greene]

(In reply to comment #6)
> So, does this mean that if I do this:
> 
> - Remove Google (which is part of the default list that ships with the app)
> 
> that "Add Google" will show up in the menu?

The case you mention is bug 334486.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 225795 [details] [diff] [review]
Same code, now with more comments

r=[e-mail]

Comment 10

[Pam Greene]

Checked in on trunk to bake before 1.8 branch landing.

Comment 11

[Mike Connor [:mconnor]]

Not a blocker, but we'll take the fix.

Comment 12

[Pam Greene]

fixed-1.8-branch, fixed-on-trunk