Dave Townsend [:mossop] Assignee
	Description • 15 years ago

We'll need some testcases where extensions can't be installed for various reasons for a couple of manual tests here

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 1 • 15 years ago

Can you point me to the code which checks for errors? That will help to find good testcases.

	Dave Townsend [:mossop] Assignee
	Comment 2 • 15 years ago

It basically download errors that we'd see, things like lost connections, going offline mid-download, invalid XPIs and such. The code is pretty simple right now and needs to be improved

	Dave Townsend [:mossop] Assignee
	Comment 3 • 14 years ago

Don't think this is vital for trunk, we need to figure out how we are displaying download progress and errors.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 4 • 14 years ago

QA can have a look at the final messages but it's not something we should consider for Litmus.

	Aakash Desai [:aakashd]
	Comment 5 • 14 years ago

Here's one instance (no testcase) where the addons manager shows two messages when a failure and a successful download had occurred on the same session.

	Dave Townsend [:mossop] Assignee
	Comment 6 • 14 years ago

Strings from Boriss over IRC:

The add-on could not be downloaded because of a connection failure on $S.

The add-on from $S could not be installed because it does not match the add-on Firefox expected.

The add-on downloaded from $S could not be installed because it appears to be corrupt.

$S could not be installed because Firefox cannot modify the needed file.

$S $S could not be installed because it is not compatible with $S $S

The add-on %S could not be installed because it has a high risk of causing stability or security problems.

	Dave Townsend [:mossop] Assignee
	Comment 7 • 14 years ago

Going to fix this in bug 553455

	Dave Townsend [:mossop] Assignee
	Comment 8 • 14 years ago

This is a first cut at doing this and also vastly improving the error messages. We don't have doorhangers so for now I'm just using the normal notification bars. Hopefully it will be an easy transition later on if doorhangers ever appear.

Mark, can I get a quick idea of what you think of this. It pushes all notification of errors and install completion through the observer service to allow the frontend to handle. I considered using a custom interface like we did for prompts but as these are just ignorable messages as far as the backend is concerned and using the prompt service is pretty sub-optimal for everyone. This way needs a little effort from every app (if they care) but they can get exactly what they want.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 9 • 14 years ago

Comment on attachment 451790 [details] [diff] [review]
patch rev 1

Looks like a nice lightweight way to get more information back to an application. Fennec uses a global install listener to do something similar. We display alerts instead of notifications.

	Dave Townsend [:mossop] Assignee
	Comment 10 • 14 years ago

The is the backend work. It stops the backend from ever displaying error messages. Instead amWebInstallListener (the code that manages a set of installs started by a webpage) sends out observer notifications when downloads start, fail and complete. It reuses the amIWebInstallInfo interface, I don't think there is really a need to define something new when only the install method is not needed. The basic flow is this:

InstallTrigger starts some installs.
amWebInstallListener dispatches an addon-install-started observer notification containing all the AddonInstalls.
Once downloads are complete it dispatches addon-install-failed if any downloads failed and then opens the confirmation UI as before.
Once installs are complete it dispatches addon-install-failed if any installs failed and then addon-install-complete.

The frontend can completely ignore these events if it wishes, they are intended to be fire and forget as far as the backend goes.

I've done some cleaning up of amWebInstallListener to make this look a bit more sensible and introduced a flag to say what phase of operation it is in. The test harness tracks which notifications have been sent out to verify that we either send addon-install-failed or addon-install-complete for all started installations.

	Dave Townsend [:mossop] Assignee
	Comment 11 • 14 years ago

This is the frontend work. Adds observer notifications for most of the new topics that we dispatch and wires them up to the new doorhanger notifications. The automated test verifies that the notifications appear as expected. It reuses some of the test files from toolkit, maybe slightly odd but I think it is better than either duplicating them in the source or copying them to multiple test directories.

	Reed Loden [:reed]
	Comment 12 • 14 years ago

Comment on attachment 454178 [details] [diff] [review]
frontend patch rev 1

>+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.

Please use the brand name instead of Firefox.

	Dave Townsend [:mossop] Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> (From update of attachment 454178 [details] [diff] [review])
> >+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
> 
> Please use the brand name instead of Firefox.

Oops missed that one. Replaced by #3 locally.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 14 years ago

Comment on attachment 454178 [details] [diff] [review]
frontend patch rev 1

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

>   observe: function (aSubject, aTopic, aData)

>+            callback: function editPrefs() {
>+              gPrefService.setBoolPref("xpinstall.enabled", true);
>               return false;

return value isn't useful here, so might as well remove it (applies to other callbacks).

>+        return (i.addon.pendingOperations & AddonManager.PENDING_INSTALL) != 0;

This seems to be the only user of AddonManager - kind of unfortunate to Cu.import() the entire module just to get a constant, but I guess it's garanteed to already be loaded?

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

> #geo-notification-icon {
>   list-style-image: url(chrome://browser/skin/Geo.png);
>   width: 16px;
>   height: 16px;
>   margin: 0 2px;
> }
> 
>+#addons-notification-icon {
>+  list-style-image: url(chrome://mozapps/skin/extensions/extensionGeneric-16.png);
>+  width: 16px;
>+  height: 16px;
>+  margin: 0 2px;
>+}

Should probably move the width/height/margin rules into a .notification-anchor-icon block rather than duplicating them (applies to the other themes as well).

	Dave Townsend [:mossop] Assignee
	Comment 15 • 14 years ago

(In reply to comment #14)
> This seems to be the only user of AddonManager - kind of unfortunate to
> Cu.import() the entire module just to get a constant, but I guess it's
> garanteed to already be loaded?

Yeah, AddonManager gets loaded in early startup so this shouldn't really cause much of a hit.

	Robert Strong (they/them - no direct email)
	Comment 16 • 14 years ago

Comment on attachment 454176 [details] [diff] [review]
backend patch rev 1

>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js
>--- a/toolkit/mozapps/extensions/amWebInstallListener.js
>+++ b/toolkit/mozapps/extensions/amWebInstallListener.js
>@@ -43,154 +43,218 @@
>  * about blocked installs. For normal installs it pops up an install
>  * confirmation when all the add-ons have been downloaded.
>  */
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> 
>+const PHASE_DOWNLOADING = 0;
>+const PHASE_INSTALLING = 1;
>+const PHASE_COMPLETE = 2;
Seems like these should use the STATE_* constants from AddonManager.jsm unless you can think of a reason not to. Might want to change phase in the code to state as well.

> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Components.utils.import("resource://gre/modules/AddonManager.jsm");
> Components.utils.import("resource://gre/modules/Services.jsm");
> 
> ["LOG", "WARN", "ERROR"].forEach(function(aName) {
>   this.__defineGetter__(aName, function() {
>     Components.utils.import("resource://gre/modules/AddonLogging.jsm");
> 
>     LogManager.getLogger("addons.weblistener", this);
>     return this[aName];
>   });
> }, this);
> 
> /**
>  * Creates a new installer to monitor downloads and prompt to install when
>  * ready
>  *
>  * @param  aWindow
>  *         The window that started the installations
>  * @param  aUrl
>  *         The URL that started the installations
>  * @param  aInstalls
>  *         An array of AddonInstalls
>  */
> function Installer(aWindow, aUrl, aInstalls) {
>   this.window = aWindow;
>   this.url = aUrl;
>   this.downloads = aInstalls;
>-  this.installs = [];
>+  this.installed = [];
>+  this.phase = PHASE_DOWNLOADING;
> 
>-  this.bundle = Cc["@mozilla.org/intl/stringbundle;1"].
>-                getService(Ci.nsIStringBundleService).
>-                createBundle("chrome://mozapps/locale/extensions/extensions.properties");
>+  notifyObservers("addon-install-started", aWindow, aUrl, aInstalls);
> 
>-  this.count = aInstalls.length;
>   aInstalls.forEach(function(aInstall) {
>     aInstall.addListener(this);
> 
>-    // Might already be a local file
>-    if (aInstall.state == AddonManager.STATE_DOWNLOADED)
>-      this.onDownloadEnded(aInstall);
>-    else if (aInstall.state == AddonManager.STATE_DOWNLOAD_FAILED)
>-      this.onDownloadFailed(aInstall);
>-    else
>+    // Start downloading if it hasn't already begun
>+    if (aInstall.state == AddonManager.STATE_AVAILABLE)
>       aInstall.install();
>   }, this);
>+
>+  this.checkAllDownloaded();
> }
> 
> Installer.prototype = {
>+  phase: null,
>   window: null,
>   downloads: null,
>-  installs: null,
>-  count: null,
>+  installed: null,
>+
>+  removeListeners: function() {
>+    this.downloads.forEach(function(aInstall) {
>+      aInstall.removeListener(this);
>+    }, this);
>+    this.downloads = null;
>+  },
> 
>   /**
>    * Checks if all downloads are now complete and if so prompts to install.
>    */
>   checkAllDownloaded: function() {
>-    if (--this.count > 0)
>+    if (this.phase != PHASE_DOWNLOADING)
>       return;
Might be a good thing to add a comment about this check preventing re-entry similar to the comment below about changing phase.

> 
>-    // Maybe none of the downloads were sucessful
>-    if (this.installs.length == 0)
>+    var failed = [];
>+    var installs = [];
>+
>+    for (let i = 0; i < this.downloads.length; i++) {
>+      let install = this.downloads[i];
>+      switch (install.state) {
>+      case AddonManager.STATE_AVAILABLE:
>+      case AddonManager.STATE_DOWNLOADING:
>+        // Exit early if any add-ons haven't started downloading yet or are
>+        // still downloading
>+        return;
>+      case AddonManager.STATE_DOWNLOAD_FAILED:
>+        failed.push(install);
>+        break;
>+      case AddonManager.STATE_DOWNLOADED:
>+        // App disabled items are not compatible and so fail to install
>+        if (install.addon.appDisabled)
>+          failed.push(install);
>+        else
>+          installs.push(install);
>+        break;
>+      }
>+    }
>+
>+    this.phase = PHASE_INSTALLING;
>+
>+    if (failed.length > 0) {
>+      // Cancel any installs that are failed because of compatibility reasons.
>+      // This must happen after changing phase or the onDownloadCancelled
>+      // event will re-enter checkAllDownloaded.
>+      failed.forEach(function(aInstall) {
>+        if (aInstall.state == AddonManager.STATE_DOWNLOADED)
>+          aInstall.cancel();
btw: seems a little strange to not remove the listeners for cancelled installs at this point though it is cleaner and simpler the way you are doing it. I don't think this needs to change.

>+      });
>+      notifyObservers("addon-install-failed", this.window, this.url, failed);
>+    }
>+
>+    // If none of the downloads were successful then exit early
>+    if (installs.length == 0) {
>+      this.phase = PHASE_COMPLETE;
>+      this.removeListeners();
>       return;
>+    }
> 
>     if ("@mozilla.org/addons/web-install-prompt;1" in Cc) {
Would you mind adding a comment about this being the optional custom install prompt?

>       try {
>         let prompt = Cc["@mozilla.org/addons/web-install-prompt;1"].
>                      getService(Ci.amIWebInstallPrompt);
>-        prompt.confirm(this.window, this.url, this.installs, this.installs.length);
>+        prompt.confirm(this.window, this.url, installs, installs.length);
>         return;
>       }
>       catch (e) {}
>     }
> 
>     let args = {};
>     args.url = this.url;
>-    args.installs = this.installs;
>+    args.installs = installs;
>     args.wrappedJSObject = args;
> 
>     Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",
>                            null, "chrome,modal,centerscreen", args);
>   },
> 
>+  /**
>+   * Checks if all installs are now complete and if so notifies observers.
>+   */
>+  checkAllInstalled: function() {
>+    if (this.phase != PHASE_INSTALLING)
>+      return;
Comment here as well

>+
>+    var failed = [];
>+
>+    for (let i = 0; i < this.downloads.length; i++) {
>+      let install = this.downloads[i];
>+      switch(install.state) {
>+      case AddonManager.STATE_DOWNLOADED:
>+      case AddonManager.STATE_INSTALLING:
>+        // Exit early if any add-ons haven't started installing yet or are
>+        // still installing
>+        return;
>+      case AddonManager.STATE_INSTALL_FAILED:
>+        failed.push(install);
>+        break;
>+      }
>+    }
>+
>+    this.phase = PHASE_COMPLETE;
>+
>+    this.removeListeners();
>+
>+    if (failed.length > 0)
>+      notifyObservers("addon-install-failed", this.window, this.url, failed);
>+
>+    if (this.installed.length > 0)
>+      notifyObservers("addon-install-complete", this.window, this.url, this.installed);
>+    this.installed = null;
>+  },
>+

	Robert Strong (they/them - no direct email)
	Comment 17 • 14 years ago

(In reply to comment #16)
> (From update of attachment 454176 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit
>...
> >+    if (failed.length > 0) {
> >+      // Cancel any installs that are failed because of compatibility reasons.
> >+      // This must happen after changing phase or the onDownloadCancelled
> >+      // event will re-enter checkAllDownloaded.
> >+      failed.forEach(function(aInstall) {
> >+        if (aInstall.state == AddonManager.STATE_DOWNLOADED)
> >+          aInstall.cancel();
> btw: seems a little strange to not remove the listeners for cancelled installs
> at this point though it is cleaner and simpler the way you are doing it. I
> don't think this needs to change.
Maybe add a comment about why it is done this way so it is obvious to anyone looking at the code in the future.

	Dave Townsend [:mossop] Assignee
	Comment 18 • 14 years ago

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 454176 [details] [diff] [review] [details])
> > >diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit
> >...
> > >+    if (failed.length > 0) {
> > >+      // Cancel any installs that are failed because of compatibility reasons.
> > >+      // This must happen after changing phase or the onDownloadCancelled
> > >+      // event will re-enter checkAllDownloaded.
> > >+      failed.forEach(function(aInstall) {
> > >+        if (aInstall.state == AddonManager.STATE_DOWNLOADED)
> > >+          aInstall.cancel();
> > btw: seems a little strange to not remove the listeners for cancelled installs
> > at this point though it is cleaner and simpler the way you are doing it. I
> > don't think this needs to change.
> Maybe add a comment about why it is done this way so it is obvious to anyone
> looking at the code in the future.

After looking at this afresh I think it is probably better to clean up early. This actually means we can drop the state values and re-entrancy checks which makes a little simpler I think.

	Robert Strong (they/them - no direct email)
	Comment 19 • 14 years ago

Comment on attachment 455246 [details] [diff] [review]
backend patch rev 2

Cleaner still!

	Dave Townsend [:mossop] Assignee
	Comment 20 • 14 years ago

Landed: http://hg.mozilla.org/mozilla-central/rev/d96f5d831876
http://hg.mozilla.org/mozilla-central/rev/6e35c635debe

	Dave Townsend [:mossop] Assignee
	Comment 21 • 14 years ago

Backed out due to test failures.

	Dave Townsend [:mossop] Assignee
	Comment 22 • 14 years ago

I was wrong to take out the re-entrancy check on checkAllDownloaded, when the confirmation dialog cancels installs we re-enter and try to re-display the confirmation dialog. I don't think the full states is needed, just a flag to signify that downloads are in progress and I've added a test for this specific case. This is all on top of the other patches for simplicity.

	Robert Strong (they/them - no direct email)
	Comment 23 • 14 years ago

Comment on attachment 455488 [details] [diff] [review]
bustage fix

>diff --git a/toolkit/mozapps/extensions/test/xpinstall/browser_cancel.js b/toolkit/mozapps/extensions/test/xpinstall/browser_cancel.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpinstall/browser_cancel.js
Creative commons license

	Dave Townsend [:mossop] Assignee
	Comment 24 • 14 years ago

Re-landed: http://hg.mozilla.org/mozilla-central/rev/dc446291a715 and http://hg.mozilla.org/mozilla-central/rev/b1b50d483f1e

	Vlado Valastiak [:wladow]
	Comment 25 • 14 years ago

Comment on attachment 454178 [details] [diff] [review]
frontend patch rev 1

>+# LOCALIZATION NOTE (addonError-1, addonError-2, addonError-3, addonError-4, addonErrorIncompatible, addonErrorBlocklisted):
>+# #1 is the add-on name, #2 is the host name, #3 is the application name
>+# #4 is the application version
>+addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected.
Could someone explain what does this mean? Pls. give us some example when this error fires up, so we are able to translate this properly.

>+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
Hardcoded "Firefox"...

	Dave Townsend [:mossop] Assignee
	Comment 26 • 14 years ago

(In reply to comment #25)
> (From update of attachment 454178 [details] [diff] [review])
> >+# LOCALIZATION NOTE (addonError-1, addonError-2, addonError-3, addonError-4, addonErrorIncompatible, addonErrorBlocklisted):
> >+# #1 is the add-on name, #2 is the host name, #3 is the application name
> >+# #4 is the application version
> >+addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected.
> Could someone explain what does this mean? Pls. give us some example when this
> error fires up, so we are able to translate this properly.

When a webpage offers an extension to install it gives us a hash to verify that we XPI we download is the XPI the website intended us to get. This error is displayed when what we download doesn't match what we expect. It means most of the time that the website had a bad link, but could mean that the users connection was intercepted.

> >+addonError-4=#1 could not be installed because Firefox cannot modify the needed file.
> Hardcoded "Firefox"...

Gah, I could have sworn I corrected that. Landed a quick fix in http://hg.mozilla.org/mozilla-central/rev/2725e6f722cb, though let me know if you think we need to change the entity names too now in which case I'll create a patch to do that.

	Vlado Valastiak [:wladow]
	Comment 28 • 14 years ago

(In reply to comment #26)
> When a webpage offers an extension to install it gives us a hash to verify that
> we XPI we download is the XPI the website intended us to get. This error is
> displayed when what we download doesn't match what we expect. It means most of
> the time that the website had a bad link, but could mean that the users
> connection was intercepted.

All clear now, thank you.

	Robert Strong (they/them - no direct email)
	Comment 29 • 14 years ago

Dave, I think you should either change the entity name or get the locales that have already translated the string to replace Firefox with #3 in the addonError-4 entity. The five locales that have translated it so far all have Firefox.
http://mxr.mozilla.org/l10n-central/search?string=addonError-4

	Dave Townsend [:mossop] Assignee
	Comment 30 • 14 years ago

(In reply to comment #29)
> Dave, I think you should either change the entity name or get the locales that
> have already translated the string to replace Firefox with #3 in the
> addonError-4 entity. The five locales that have translated it so far all have
> Firefox.
> http://mxr.mozilla.org/l10n-central/search?string=addonError-4

Yeah, filed bug 576801, 576803, 576804, 576805 and 576806 to get those fixed. Changing the entity would probably involve changing 5 others to keep the code simple so just fixing the locales will be quicker I think.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 14 years ago

Some failures aren't caught right now. I will verify the fix once bug 577048 has been fixed.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 32 • 14 years ago

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre