511091 - Support custom icons for disabled extensions

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We should support loading icon.png from the extension dir if there is no iconURL or the extension is disabled. This will both allow extensions to brand themselves at all times helping users to identify them as well as removing unnecessary differences in the extension/theme codepath.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

If the item isn't a theme  and it isn't disabled then look for an iconURL. Otherwise look for icon.png. Otherwise use the fallback images.

Comment 2

[Robert Strong (they/them - no direct email)]

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

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -7725,72 +7725,73 @@ ExtensionsDataSource.prototype = {
>   },
> 
>   /**
>    * Gets an URL to a theme's image file
Please update the comment since this no longer applies to just themes

>    * @param   item
>    *          The RDF Resource representing the item
>    * @param   fileName
>    *          The file to locate a URL for
>-   * @param   fallbackURL
>-   *          If the location fails, supply this URL instead
>    * @returns An RDF Resource to the URL discovered, or the fallback
>    *          if the discovery failed.
>    */
>-  _getThemeImageURL: function EMDS__getThemeImageURL(item, fileName, fallbackURL) {
>+  _getImageURL: function EMDS__getImageURL(item, fileName) {
>     var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>     var installLocation = this._em.getInstallLocation(id);
>     if (!installLocation)
>       return fallbackURL;
>     var file = installLocation.getItemFile(id, fileName)
>     if (file.exists())
>       return gRDF.GetResource(getURLSpecFromFile(file));
> 
>-    return fallbackURL ? gRDF.GetResource(fallbackURL) : null;
>+    return null;
>   },
> 
>   /**
>    * Get the em:iconURL property (icon url of the item)
>    */
>   _rdfGet_iconURL: function EMDS__rdfGet_iconURL(item, property) {
>     var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>     var type = this.getItemProperty(id, "type");
>-    if (type & Ci.nsIUpdateItem.TYPE_THEME)
>-      return this._getThemeImageURL(item, "icon.png", URI_GENERIC_ICON_THEME);
>-
>-    if (inSafeMode())
>-      return gRDF.GetResource(URI_GENERIC_ICON_XPINSTALL);
>-
>-    var hasIconURL = this._inner.hasArcOut(item, property);
>-    // If the addon doesn't have an IconURL property or it is disabled use the
>-    // generic icon URL instead.
>-    if (!hasIconURL || this.getItemProperty(id, "isDisabled") == "true")
>-      return gRDF.GetResource(URI_GENERIC_ICON_XPINSTALL);
>-    var iconURL = stringData(this._inner.GetTarget(item, property, true));
>-    try {
>-      var uri = newURI(iconURL);
>-      var scheme = uri.scheme;
>-      // Only allow chrome URIs or when installing http(s) URIs.
>-      if (scheme == "chrome" || (scheme == "http" || scheme == "https") &&
>-          this._inner.hasArcOut(item, EM_R("downloadURL")))
>-        return null;
>-    }
>-    catch (e) {
>-    }
>-    // Use a generic icon URL for addons that have an invalid iconURL.
>+
>+    if (type != Ci.nsIUpdateItem.TYPE_THEME && !inSafeMode()) {
>+      var hasIconURL = this._inner.hasArcOut(item, property);
>+      // Only use the iconURL if there is one and the item is not disabled
>+      if (hasIconURL && this.getItemProperty(id, "isDisabled") != "true") {
Seems like the check for isDisabled should be in the parent if statement. Any reason not to do so?

>+        var iconURL = stringData(this._inner.GetTarget(item, property, true));
>+        try {
>+          var uri = newURI(iconURL);
>+          var scheme = uri.scheme;
>+          // Only allow chrome URIs or when installing http(s) URIs.
nit: this comment is a tad confusing. Perhaps something along the lines of
Only allow chrome URIs or http(s) URIs when there is a downloadURL which means that it is being installed.

I very much do prefer having the icon outside of the jar (thanks for doing this) but I don't think it is all that valuable to allow a separate icons for enabled / disabled. I personally think the icon at the root of the extension should just be used but I'm ok with this route.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 2

You're probably right, this switches it around so icon.png is used in preferences at all times. It also makes things a little cleaner.

Comment 4

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/9f8181d17a14

Comment 5

[Dave Townsend [:mossop]]

Will need some docs on this once it lands on 3.6 branch

Comment 6

[u88484]

Attachment: Screenshot

Dave, this doesn't seem to be working unless I'm missing something.  I moved my icon to the extension's main folder.  I removed the iconURL from the install.rdf and now I don't get an icon when the extension is enabled and disabled.  Screenshot shows enabled state but it is the same in disabled state.

I also removed the three extensions.*** files from my profile and no change.

Comment 7

[Dave Townsend [:mossop]]

Is that with the windows nightly? This change didn't make it into the nightly.

Comment 8

[u88484]

(In reply to comment #7)
> Is that with the windows nightly? This change didn't make it into the nightly.

Sorry, I knew it missed the first nightly but I assumed it made the second nightly.  Verified on windows.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 395287 [details] [diff] [review]
patch rev 2

This makes extensions perform more similarly to themes and allows authors to brand their extensions even when disabled. It has tests and has been baking.

Comment 10

[Dave Townsend [:mossop]]

Attachment: followup fix

Slight error in the original patch that causes errors in the error console when installing add-ons. This corrects it.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 398931 [details] [diff] [review]
followup fix

Landed, include in the above approval request

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 395287 [details] [diff] [review]
patch rev 2

a192=beltzner for this and the followup

Comment 13

[Dave Townsend [:mossop]]

Pushed to branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9d2162fc6ae3
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/77e5c3da919f

Comment 14

[Eric Shepherd [:sheppy]]

Documented. See:

https://developer.mozilla.org/en/Install_Manifests#iconURL

https://developer.mozilla.org/en/Building_an_Extension#Setting_up_the_Development_Environment

https://developer.mozilla.org/en/Updating_extensions_for_Firefox_3.6

Comment 15

[Dave Townsend [:mossop]]

Attachment: Test extension

Comment 16

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre