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

VERIFIED FIXED in fennec1.0b4

Status

VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: jmaher, Assigned: mfinkle)

Tracking

Trunk
fennec1.0b4

Details

(Whiteboard: [fennec l10n] [polish])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
I heard it was lame that we couldn't remove these. So maybe this should block.
tracking-fennec: --- → ?
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!

Updated

9 years ago
tracking-fennec: ? → 1.0+
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"
Summary: no way to remove or uninstall awesomebar search providers! → List and Uninstall awesomebar search providers via the Add-ons Manager
Assignee: nobody → mark.finkle
Created attachment 400958 [details] [diff] [review]
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.
Whiteboard: [fennec l10n]
The patch adds one string
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.
(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) ?
We decided to get this landed with the restart for uninstall. Enable/Disable will not require a restart.
Created attachment 401527 [details] [diff] [review]
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.
Attachment #400958 - Attachment is obsolete: true
Attachment #401527 - Flags: review?(gavin.sharp)
Whiteboard: [fennec l10n] → [fennec l10n] [polish]
Created attachment 402255 [details] [diff] [review]
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.
Created attachment 402256 [details] [diff] [review]
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.
Attachment #401527 - Attachment is obsolete: true
Attachment #401527 - Flags: review?(gavin.sharp)
Comment on attachment 402256 [details] [diff] [review]
final patch

Actually, this is the final patch.
Attachment #402256 - Attachment description: interdiff #2 → final patch
Created attachment 402258 [details] [diff] [review]
interdiff #2

and this is the real interdiff #2.
Attachment #402255 - Flags: review?(mark.finkle)
Attachment #402258 - Flags: review?(mark.finkle)
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
Comment on attachment 402258 [details] [diff] [review]
interdiff #2

Looks great
Attachment #402258 - Flags: review?(mark.finkle) → review+
We can file a followup on any kind of "Undo" mechanism.
Attachment #402255 - Flags: review?(mark.finkle) → review+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.