Closed Bug 562495 Opened 10 years ago Closed 10 years ago

Support the new add-ons manager

Categories

(Firefox for Android Graveyard :: General, defect, major)

x86
macOS
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mossop, Assigned: mfinkle)

References

Details

(Whiteboard: [AddonsRewriteTestday])

Attachments

(1 file, 7 obsolete files)

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.
Depends on: 562518
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"
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.
Whiteboard: [AddonsRewriteTestday]
Attached patch initial WIP (obsolete) — Splinter Review
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.
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.
Attached patch WIP 2 (obsolete) — Splinter Review
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
Assignee: nobody → mark.finkle
Attachment #445048 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
Added support for web based installs using the Fennec UI
Attachment #445293 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Updated for changes to bug 566020, which this bug now depends on.
Attachment #445435 - Attachment is obsolete: true
Attachment #445498 - Flags: review?(dtownsend)
Tests are coming up too
Depends on: 566020
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?
Blocks: 560451
Attached patch patch 2 (obsolete) — Splinter Review
Updated patch per review comments
Attachment #445498 - Attachment is obsolete: true
Attachment #445911 - Flags: review?(dtownsend)
Attachment #445498 - Flags: review?(dtownsend)
(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.
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.
Severity: normal → major
tracking-fennec: --- → ?
Flags: in-testsuite?
Flags: in-litmus?
Removing blocking-? flag for now. THis only affects mobile-browser (trunk) and not mobile-1.1 (Fennec 1.1 release).
tracking-fennec: ? → ---
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.
Attachment #445911 - Flags: review?(dtownsend) → review-
Attached patch patch 3 (obsolete) — Splinter Review
* 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.
Attachment #445911 - Attachment is obsolete: true
Attachment #446282 - Flags: review?(dtownsend)
Attached patch patch 4 (obsolete) — Splinter Review
Oops. Forgot to qref the patch
Attachment #446282 - Attachment is obsolete: true
Attachment #446284 - Flags: review?(dtownsend)
Attachment #446282 - Flags: review?(dtownsend)
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.
Attachment #446284 - Flags: review?(dtownsend) → review-
Attached patch patch 5Splinter Review
* Don't overwrite a compatibility status
* Move aError to onUpdateEnded
Attachment #446284 - Attachment is obsolete: true
Attachment #446300 - Flags: review?(dtownsend)
Attachment #446300 - Attachment is patch: true
Attachment #446300 - Attachment mime type: application/octet-stream → text/plain
Attachment #446300 - Attachment description: patch (m-192) → patch 5
Attachment #446300 - Flags: review?(dtownsend) → review+
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);
>   }
> };
(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)
pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/b3d6802584bc

need to add some tests for this
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Aakash, can you please test if it works as expected for Fennec and verify this bug? Thanks.
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
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Aakash, can you please mention the litmus testcase id? That makes it easier to finding the test later on.
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.
Flags: in-litmus+ → in-litmus-
You need to log in before you can comment on or make changes to this bug.