Support custom icons for disabled extensions

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Add-ons Manager
--
minor
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({dev-doc-complete, verified1.9.2})

Trunk
mozilla1.9.3a1
dev-doc-complete, verified1.9.2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Attachment #395045 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
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.
Attachment #395045 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 3

8 years ago
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.
Attachment #395045 - Attachment is obsolete: true
Attachment #395287 - Flags: review?(robert.bugzilla)
Attachment #395287 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 4

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9f8181d17a14
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs baking]
Target Milestone: mozilla1.9.2 → mozilla1.9.3a1
(Assignee)

Comment 5

8 years ago
Will need some docs on this once it lands on 3.6 branch
Keywords: dev-doc-needed

Comment 6

8 years ago
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.
(Assignee)

Comment 7

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

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
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.
Attachment #395287 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
Whiteboard: [needs baking]
(Assignee)

Comment 10

8 years ago
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.
Attachment #398931 - Flags: review?(robert.bugzilla)
Attachment #398931 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 11

8 years ago
Comment on attachment 398931 [details] [diff] [review]
followup fix

Landed, include in the above approval request
Attachment #398931 - Flags: approval1.9.2?
Comment on attachment 395287 [details] [diff] [review]
patch rev 2

a192=beltzner for this and the followup
Attachment #395287 - Flags: approval1.9.2? → approval1.9.2+
Attachment #398931 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 13

8 years ago
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
status1.9.2: --- → beta1-fixed
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 15

8 years ago
Created attachment 412025 [details]
Test extension
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
(Assignee)

Updated

8 years ago
Depends on: 525672
You need to log in before you can comment on or make changes to this bug.