Last Comment Bug 511091 - Support custom icons for disabled extensions
: Support custom icons for disabled extensions
Status: VERIFIED FIXED
: dev-doc-complete, verified1.9.2
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on: 525672
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-18 05:40 PDT by Dave Townsend [:mossop]
Modified: 2009-12-16 12:09 PST (History)
3 users (show)
dtownsend: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
patch rev 1 (11.01 KB, patch)
2009-08-18 05:57 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Splinter Review
patch rev 2 (11.28 KB, patch)
2009-08-19 03:46 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review
Screenshot (137.24 KB, image/png)
2009-08-20 08:08 PDT, u88484
no flags Details
followup fix (898 bytes, patch)
2009-09-06 04:53 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review
Test extension (3.34 KB, application/x-xpinstall)
2009-11-12 11:58 PST, Dave Townsend [:mossop]
no flags Details

Description Dave Townsend [:mossop] 2009-08-18 05:40:42 PDT
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] 2009-08-18 05:57:59 PDT
Created attachment 395045 [details] [diff] [review]
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 [:rstrong] (use needinfo to contact me) 2009-08-18 14:37:01 PDT
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] 2009-08-19 03:46:18 PDT
Created attachment 395287 [details] [diff] [review]
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] 2009-08-20 03:06:49 PDT
http://hg.mozilla.org/mozilla-central/rev/9f8181d17a14
Comment 5 Dave Townsend [:mossop] 2009-08-20 03:20:20 PDT
Will need some docs on this once it lands on 3.6 branch
Comment 6 u88484 2009-08-20 08:08:31 PDT
Created attachment 395587 [details]
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] 2009-08-20 08:21:44 PDT
Is that with the windows nightly? This change didn't make it into the nightly.
Comment 8 u88484 2009-08-20 08:27:44 PDT
(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] 2009-08-24 15:30:38 PDT
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] 2009-09-06 04:53:39 PDT
Created attachment 398931 [details] [diff] [review]
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] 2009-09-07 02:10:27 PDT
Comment on attachment 398931 [details] [diff] [review]
followup fix

Landed, include in the above approval request
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2009-09-10 05:12:24 PDT
Comment on attachment 395287 [details] [diff] [review]
patch rev 2

a192=beltzner for this and the followup
Comment 15 Dave Townsend [:mossop] 2009-11-12 11:58:03 PST
Created attachment 412025 [details]
Test extension
Comment 16 Tony Chung [:tchung] 2009-11-12 12:05:50 PST
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

Note You need to log in before you can comment on or make changes to this bug.