562495 - Support the new add-ons manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dave Townsend [:mossop]]

Landing the new add-ons manager on trunk will break Fennec builds currently, going to elaborate on exactly how broken you'll be once my test builds are complete.

Comment 1

[Aakash Desai [:aakashd]]

One issue already found is that Clicking "Add to Firefox" on AMO doesn't perform an action on 2010-04-30 fennec trunk nightly build

Steps to Reproduce:
1. Go to addons.mozilla.org
2. Click on "Add to Firefox"

Comment 2

[Aakash Desai [:aakashd]]

Another issue is that nothing shows up in our Addons Manager:

1. Go to the controls window
2. Click on the Addons button

Actual Results:
The default search engines are not listed there nor are any recommended addons.

Comment 3

[Dave Townsend [:mossop]]

Attachment: initial WIP

This is an initial WIP patch that I threw together, it should have the main UI work, only stuff missing is making installs work, you'll likely best do that by overriding the webinstalllistener component and making it display the UI you like there.

Comment 4

[Dave Townsend [:mossop]]

If you set the preference "extensions.enabledScopes" to 5 then the add-ons manager will not look in global install locations for extensions, just in the application and profile locations.

Comment 5

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

Attachment: WIP 2

This patch builds on the first patch. New stuff includes:
* Displays installed addons and search plugins
* Handles add-on manager based installs
* Handles web based installs (uses "addon-install-blocked" and related changes)
* Handles updates

Left to do are:
* Remove the ugly dialog during web based installs and use our own
* Some cleanup of the richlistitems

Comment 6

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

Attachment: WIP 3

Added support for web based installs using the Fennec UI

Comment 7

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

Attachment: patch

Updated for changes to bug 566020, which this bug now depends on.

Comment 8

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

Tests are coming up too

Comment 9

[Dave Townsend [:mossop]]

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

First set of comments, will look over this more soon.

>diff --git a/app/mobile.js b/app/mobile.js

> /* extension manager and xpinstall */
>-pref("xpinstall.dialog.confirm", "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul");
>-pref("xpinstall.dialog.progress.skin", "chrome://browser/content/browser.xul");
>-pref("xpinstall.dialog.progress.chrome", "chrome://browser/content/browser.xul");
>-pref("xpinstall.dialog.progress.type.skin", "navigator:browser");
>-pref("xpinstall.dialog.progress.type.chrome", "navigator:browser");
> pref("xpinstall.whitelist.add", "addons.mozilla.org");
> 
> pref("extensions.autoupdate.enabled", true);
> pref("extensions.autoupdate.interval", 86400);

The new manager is actually going to do auto-update for you, though it might be worth waiting a short time while we iron ut the specific behavior of that before you remove all your stuff.

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

>           buttons = [{
>             label: strings.getString("xpinstallPromptAllowButton"),
>             accessKey: null,
>             popup: null,
>             callback: function() {
>               // Kick off the xpinstall

"Kick off the install" would be more appropriate now.

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

>     let self = this;
>-    InstallTrigger.install(params, function(aURL, aStatus) { self._installCallback(aItem, aStatus); });
>+    AddonManager.getInstallForURL(aItem.getAttribute("xpiURL"),
>+                                   function(aInstall) { self._installCallback(aItem, aInstall); },
>+                                   "application/x-xpinstall",
>+                                   aItem.getAttribute("xpiHash"));

Intending is off here. Might as well just call aInstall.install() directly here rather than using _installCallback right?

>+  onInstallEnded: function(aInstall, aAddon) {
>+    // XXX fix updating stuff
>+    ExtensionsView.showRestart(this._updating ? "update" : "normal");

You should check if aAddon.pendingOperations has PENDING_INSTALL since we now support add-ons that can install without restarts.

>-    if (!ExtensionsView.visible)
>-      return;
>+    if (ExtensionsView.visible) {

Probably better to continue bailing out early when possible here.

>+  onInstallFailed: function(aInstall, aError) {
>+    this._showAlert(false);
>+
>+    if (ExtensionsView.visible) {
>+      let element = ExtensionsView.getElementForAddon(aInstall.sourceURL);
>+      if (!element)
>+        return;
>+  
>+      element.removeAttribute("opType");
>+      let bundles = Cc["@mozilla.org/intl/stringbundle;1"].getService(Ci.nsIStringBundleService);
>+      let strings = bundles.createBundle("chrome://global/locale/xpinstall/xpinstall.properties");
>+
>+      try {
>+        var msg = strings.GetStringFromName("error" + aStatus);
>+      } catch (ex) {
>+        msg = strings.formatStringFromName("unknown.error", [aStatus]);
>+      }

This seems broken, where is aStatus coming from?

Comment 10

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

Attachment: patch 2

Updated patch per review comments

Comment 11

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

(In reply to comment #9)
> >             popup: null,
> >             callback: function() {
> >               // Kick off the xpinstall
> 
> "Kick off the install" would be more appropriate now.

Fixed

> >+    AddonManager.getInstallForURL(aItem.getAttribute("xpiURL"),
> >+                                   function(aInstall) { self._installCallback(aItem, aInstall); },
> >+                                   "application/x-xpinstall",
> >+                                   aItem.getAttribute("xpiHash"));
> 
> Intending is off here. Might as well just call aInstall.install() directly here
> rather than using _installCallback right?

Fixed indenting and refactored _installCallback away.

> >+  onInstallEnded: function(aInstall, aAddon) {
> >+    // XXX fix updating stuff
> >+    ExtensionsView.showRestart(this._updating ? "update" : "normal");
> 
> You should check if aAddon.pendingOperations has PENDING_INSTALL since we now
> support add-ons that can install without restarts.

Done

> >-    if (!ExtensionsView.visible)
> >-      return;
> >+    if (ExtensionsView.visible) {
> 
> Probably better to continue bailing out early when possible here.

Done

> >+  onInstallFailed: function(aInstall, aError) {
> 
> This seems broken, where is aStatus coming from?

Reworked the code to use a simple mapping of AddonManager errors to xpinstall errors. Also called onInstallError from onDownloadError so the code was shared.

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

This is just a sidenote as it would seem to be unrelated.
Current m-c/fennec doesn't list downloads in download manager anymore.
Applying this patch fixes it for me.

Comment 13

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

Removing blocking-? flag for now. THis only affects mobile-browser (trunk) and not mobile-1.1 (Fennec 1.1 release).

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 445911 [details] [diff] [review]
patch 2

Just one issue that needs to be changed a bit, should be a quick r+ next time around.

>     var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>-    os.addObserver(gXPInstallObserver, "xpinstall-install-blocked", false);
>+    os.addObserver(gXPInstallObserver, "addon-install-blocked", false);

Note: You may want to start using Services.jsm here and elsewhere. Doesn't need to happen for this patch though.

>+    let updateCheckListener = {
>+      _getJSON: function(aAddon, aInstall) {
>+        data = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+        if (aInstall)
>+          data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name, version: aInstall.version });
>+        else
>+          data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name });
>+        return data;
>+      },
>+      onCompatibilityUpdateAvailable: function(aAddon) {
>+        gObs.notifyObservers(this._getJSON(aAddon), "addon-update-ended", "compatibility");
>+      },
>+      onUpdateAvailable: function(aAddon, aInstall) {
>+        gObs.notifyObservers(this._getJSON(aAddon, aInstall), "addon-update-ended", "success");
>+        aInstall.install();
>+      },
>+      onNoUpdateAvailable: function(aAddon, aError) {
>+        gObs.notifyObservers(this._getJSON(aAddon), "addon-update-ended", aError ? "error" : "no-update");
>+      },
>+      onUpdateFinished: function(aAddon) {
>+      }
>+    };

I'm not sure if this is quite right. You'll end up dispatching addon-update-ended twice in the case where the update check finds a compatibility update, once for the compatibility update itself and again with either success or no-update depending on whether a new version is also available. This will obscure your status message about compatibility updates.

You probably want to keep creating a new instance of UpdateCheckListener for each check, and remembering the status as a property of it, dispatching addon-update-ended in onUpdateFinished.

Comment 15

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

Attachment: patch 3

* Using Services.jsm in Fennec is filed, I think
* I switched the update listener as you suggested. Creating a new instance for each call and only firing the "addon-updated-ended" notification once.

Comment 16

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

Attachment: patch 4

Oops. Forgot to qref the patch

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 446284 [details] [diff] [review]
patch 4

Still not quite right I think.

> function UpdateCheckListener() {
>-  this._addons = [];
>+  this._status = null;
>+  this._version = null;
> }
> 
> UpdateCheckListener.prototype = {
>-  /////////////////////////////////////////////////////////////////////////////
>-  // nsIAddonUpdateCheckListener
>-  onUpdateStarted: function ucl_onUpdateStarted() {
>+  onCompatibilityUpdateAvailable: function(aAddon) {
>+    this._status = "compatibility";
>   },
> 
>-  onUpdateEnded: function ucl_onUpdateEnded() {
>-    if (!this._addons.length)
>-      return;
>-
>-    // If we have some updateable add-ons, let's download them
>-    let items = [];
>-    for (let i = 0; i < this._addons.length; i++)
>-      items.push(gExtMgr.getItemForID(this._addons[i]));
>-
>-    // Start the actual downloads
>-    gExtMgr.addDownloads(items, items.length, null);
>-
>-    // Remember that we downloaded new updates so we don't check again
>-    gNeedsRestart = true;
>+  onUpdateAvailable: function(aAddon, aInstall) {
>+    this._status = "update";
>+    this._version = aInstall.version;
>+    aInstall.install();
>   },
> 
>-  onAddonUpdateStarted: function ucl_onAddonUpdateStarted(aAddon) {
>-    gObs.notifyObservers(aAddon, "addon-update-started", null);
>+  onNoUpdateAvailable: function(aAddon, aError) {
>+    this._status = aError ? "error" : "no-update";
>   },

The docs are correct and the source is wrong here, we'll move the error parameter onto onUpdateFinished, I guess you can either leave this or change this, now, don't think it'd be a problem to change before the platform does.

I think you want to set this._status only if this._status is null, so that you still pass through the compatibility state.

Comment 18

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

Attachment: patch 5

* Don't overwrite a compatibility status
* Move aError to onUpdateEnded

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 446300 [details] [diff] [review]
patch 5

Assuming that the status that you want to pass through in order of importance is "error" > "update" > "compatibility" > "no-update" then you want to tweak this as follows:

> function UpdateCheckListener() {
>-  this._addons = [];
>+  this._status = null;
>+  this._version = null;
> }
> 
> UpdateCheckListener.prototype = {
>-  /////////////////////////////////////////////////////////////////////////////
>-  // nsIAddonUpdateCheckListener
>-  onUpdateStarted: function ucl_onUpdateStarted() {
>+  onCompatibilityUpdateAvailable: function(aAddon) {
>+    this._status = "compatibility";
>   },
> 
>-  onUpdateEnded: function ucl_onUpdateEnded() {
>-    if (!this._addons.length)
>-      return;
>-
>-    // If we have some updateable add-ons, let's download them
>-    let items = [];
>-    for (let i = 0; i < this._addons.length; i++)
>-      items.push(gExtMgr.getItemForID(this._addons[i]));
>-
>-    // Start the actual downloads
>-    gExtMgr.addDownloads(items, items.length, null);
>-
>-    // Remember that we downloaded new updates so we don't check again
>-    gNeedsRestart = true;
>+  onUpdateAvailable: function(aAddon, aInstall) {
>+    // Don't overwrite a compatibility status
>+    if (!this._status)
>+      this._status = "update";

Always set this._status to "update" here overwriting whatever it is currently.

>+    this._version = aInstall.version;
>+    aInstall.install();
>   },
> 
>-  onAddonUpdateStarted: function ucl_onAddonUpdateStarted(aAddon) {
>-    gObs.notifyObservers(aAddon, "addon-update-started", null);
>+  onNoUpdateAvailable: function(aAddon) {
>+    this._status = "no-update";

Only set this._status if it is null.

>   },
> 
>-  onAddonUpdateEnded: function ucl_onAddonUpdateEnded(aAddon, aStatus) {
>-    gObs.notifyObservers(aAddon, "addon-update-ended", aStatus);
>-    
>-    // Save the add-on id if we can download an update
>-    if (aStatus == Ci.nsIAddonUpdateCheckListener.STATUS_UPDATE)
>-      this._addons.push(aAddon.id);
>-  },
>+  onUpdateFinished: function(aAddon, aError) {
>+    let data = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    if (this._version)
>+      data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name, version: this._version });
>+    else
>+      data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name });
> 
>-  QueryInterface: function ucl_QueryInterface(aIID) {
>-    if (!aIID.equals(Ci.nsIAddonUpdateCheckListener) &&
>-        !aIID.equals(Ci.nsISupports))
>-      throw Components.results.NS_ERROR_NO_INTERFACE;
>-    return this;
>+    if (aError)
>+      this._status = "error";
>+
>+    gObs.notifyObservers(data, "addon-update-ended", this._status);
>   }
> };

Comment 20

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

(In reply to comment #19)
> (From update of attachment 446300 [details] [diff] [review])
> Assuming that the status that you want to pass through in order of importance
> is "error" > "update" > "compatibility" > "no-update" then you want to tweak
> this as follows:

OK, that makes sense - and matches with what I see in XPIProvider.jsm (I shoulda looked there first)

Comment 21

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

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/b3d6802584bc

need to add some tests for this

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aakash, can you please test if it works as expected for Fennec and verify this bug? Thanks.

Comment 23

[Aakash Desai [:aakashd]]

verified FIXED On build:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010722 Namoroka/4.0b2pre Fennec/2.0a1pre

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aakash, can you please mention the litmus testcase id? That makes it easier to finding the test later on.

Comment 25

[Aakash Desai [:aakashd]]

There was nothing UI facing changed here. So, its the BFT's and FFT's for the Addons Manager within the Fennec 2.0 catch all test run. I'll mark it as minus, instead.