514510 - List and Uninstall awesomebar search providers via the Add-ons Manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

it is easy to install an awesomebar search provider, but there is no way to get rid of it or move it to the end of the list.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I heard it was lame that we couldn't remove these. So maybe this should block.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Mark, I've seen on IRC that you and madhava have ideas about that and how to avoid adding strings/panels, I'll be more than happy to do something here!

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

We decided to show the search providers in the add-ons manager and allow them to be uninstalled from there as well.

The search provider row should not display "Options" and "Disable" features, only "Uninstall"

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

This patch adds support for search providers in the add-ons manager including:
* Disable/Enable provider
* Uninstall/Cancel provider

When any of the above actions are executed, the "Restart" banner appears just like add-ons. This is not strictly needed as search providers don't need a restart, but since they are commingled with add-ons which do need a restart, I felt we wanted the consistency.

The last part bit of code that needs to be added is really removing providers during a restart. Up until that point the user and "undo" the uninstall - just like the add-ons.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

The patch adds one string

Comment 6

[Madhava Enros [:madhava]]

I don't think we should require a restart here;  having to do a restart is painful, so the consistency argument becomes one to have a consistently bad experience.  I'm not a fan of consistency at all costs.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> I don't think we should require a restart here;  having to do a restart is
> painful, so the consistency argument becomes one to have a consistently bad
> experience.  I'm not a fan of consistency at all costs.

The Restart notification added a bit of an "undo" to the process. Without it, will we just disable (not a big deal) and remove (more painful to undo) ?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

We decided to get this landed with the restart for uninstall. Enable/Disable will not require a restart.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch lists all search engines in the add-on manager. The user can enable/disable to make an engine appear/disappear from the searchbar in the awesomebar. This happens right away, no restart needed.

The user can not uninstall default engines, only disable them.

The user can uninstall non-default engines, but it requires a restart. The user can cancel the uninstall before restarting.

Comment 10

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

Attachment: interdiff

I addressed my review comments directly to get this landed tonight, so here's the interdiff. Comments:

diff --git a/chrome/content/browser.js b/chrome/content/browser.js

+  observe: function (aSubject, aTopic, aData) {

moved the observer here to allow disable/enable changes to take effect in the awesomebar immediately.

+  get engines() {

Had to rework this a bit since updatePageSearchEngines doesn't deal with a null this.engines, which could happen now that I blow away the cache.

(This actually also fixes an existing bug if you visit a site with a search engine to offer before having opened the awesomebar)

So I made it a smart getter, and added one for searchService while I was at it.

diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

-    // Remove any pending search engines
-    let start = this._localItem.nextSibling;
-    let end = this._repoItem;
-    while (start != end) {
-      if (start.getAttribute("type") == "1024" &&
-          start.getAttribute("appManaged") == "false" &&
-          start.getAttribute("opType") == "needs-uninstall") {
-        let engine = this._search.getEngineByName(start.getAttribute("name"));
-        this._search.removeEngine(engine);
-      }
-      start = start.nextSibling;
-    }

removed this per discussion on IRC
 
-      addon.id = engine.name; // engine.alias always seems to be null
+      addon.id = engine.name;

engine.alias should really be "engine.keyword", most engines don't have one

+      addon.iconURL = engine.iconURI ? engine.iconURI.spec : "";

icon can be null, need to account for that

+    if (engines.length + items.length == 0) {
+      let strings = document.getElementById("bundle_browser");
+      this.displaySectionMessage("local", strings.getString("addonsLocalNone.label"), null, true);

needed to take into account both types

   cancelUninstall: function ev_cancelUninstall(aItem) {
-    let opType;
-    if (aItem.getAttribute("type") == "1024") {
-      this._search.getEngineByName(aItem.getAttribute("name")).hidden = false;
-      opType = "";
-    } else {
-      let id = this._getIDFromURI(aItem.id);
-      this._extmgr.cancelUninstallItem(id);
-      opType = this._getRDFProperty(id, "opType");
-    }
-    
+    let id = this._getIDFromURI(aItem.id);
+    this._extmgr.cancelUninstallItem(id);
+
     this.hideRestart();
+
+    let opType = this._getRDFProperty(id, "opType");
     aItem.setAttribute("opType", opType);
   },

this is just reverting these changes, since it's now impossible to cancelUninstall for a search item.

Comment 11

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

Attachment: final patch

Forgot a couple of changes...

One other comment I had was that it'd be nice to avoid duplicating the xbl, since it seems to be largely the same as the existing bindings. Could also use attribute inheritance rather than code in the constructor, I think. Will check into that separately, though.

Comment 12

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

Comment on attachment 402256 [details] [diff] [review]
final patch

Actually, this is the final patch.

Comment 13

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

Attachment: interdiff #2

and this is the real interdiff #2.

Comment 14

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

I pushed this:
https://hg.mozilla.org/mobile-browser/rev/70d674d320b0

We should address review comments mark has on my changes in a followup, I guess.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 402258 [details] [diff] [review]
interdiff #2

Looks great

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

We can file a followup on any kind of "Undo" mechanism.

Comment 17

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

filed bug 518277 on that

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b1pre) Gecko/20090923 Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2b1pre) Gecko/20090923
Fennec/1.0b4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20090923
Fennec/1.0b4pre