Closed Bug 514510 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b4
Tracking Status
fennec 1.0+ ---

People

(Reporter: jmaher, Assigned: mfinkle)

Details

(Whiteboard: [fennec l10n] [polish])

Attachments

(3 files, 2 obsolete files)

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!
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
Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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]
Attached patch interdiffSplinter Review
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.
Attached patch final patchSplinter Review
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
Attached patch interdiff #2Splinter Review
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
Closed: 10 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.