552744 - Support reading the icon.png from the package for the iconURL

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This implements reading the icon.png from the installed directory or even the XPI file directly for not-yet-installed items. As a bonus it also adds the theme preview to the screenshots array.

As a part of doing this I discovered a few more things that really can't be serialised into the temporary JSON manifest that we use to pass info to the next process. I've implemented the slightly nicer toJSON method which JSON.stringify uses to get a filtered version of the object for serialisation.

Comment 2

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

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

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -4433,16 +4418,39 @@ AddonInternal.prototype = {
>         if (aTargetApp.id == aUpdateTarget.id && (aSyncCompatibility ||
>             Services.vc.compare(aTargetApp.maxVersion, aUpdateTarget.maxVersion) < 0)) {
>           aTargetApp.minVersion = aUpdateTarget.minVersion;
>           aTargetApp.maxVersion = aUpdateTarget.maxVersion;
>         }
>       });
>     });
>     this.appDisabled = !isUsableAddon(this);
>+  },
>+
>+  toJSON: function(key) {
>+    const BLACKLIST = ["_sourceBundle", "_install", "_wrapper"];
Since all properties starting with '_' should be excluded (at least I think this is the case) I am kind of leaning toward just excluding any property that starts with '_'.

>+    let obj = {};
>+    for (let prop in this) {
>+      // Ignore blacklisted properties
>+      if (BLACKLIST.indexOf(prop) != -1)
>+        continue;
>+
>+      // Ignore getters
>+      if (this.__lookupGetter__(prop))
>+        continue;
Would it make sense to also check __lookupSetter__? Probably wouldn't hurt but I am fine without it.

Comment 3

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

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

Please add a doc comment for
>+  toJSON: function(key) {

I'm fine with or without the other two review comments fixed... just want you to consider them.

Comment 4

[Dave Townsend [:mossop]]

Took those changes and landed: http://hg.mozilla.org/mozilla-central/rev/b0e031d5b5c9

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (From update of attachment 447551 [details] [diff] [review])
> Please add a doc comment for
> >+  toJSON: function(key) {

Landed this as http://hg.mozilla.org/mozilla-central/rev/2aeb00859448 after checking it with Rob over IRC

Comment 6

[Dave Townsend [:mossop]]

And landed a bustage fix for the xpinstall browser chrome tests with r=robstrong over IRC: http://hg.mozilla.org/mozilla-central/rev/5ec0d33625c1

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100530 Minefield/3.7a5pre (.NET CLR 3.5.30729) ID:20100530040319