Closed Bug 552744 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Keywords: regression, Whiteboard: [rewrite])

Attachments

(1 file)

No description provided.
Priority: P1 → P2
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: in-litmus-
blocking2.0: --- → beta1+
Keywords: regression
Assignee: dtownsend → nobody
Status: ASSIGNED → NEW
Attached patch patch rev 1Splinter Review
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #447551 - Flags: review?(robert.bugzilla)
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 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.
Attachment #447551 - Flags: review?(robert.bugzilla) → review+
Took those changes and landed: http://hg.mozilla.org/mozilla-central/rev/b0e031d5b5c9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(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
And landed a bustage fix for the xpinstall browser chrome tests with r=robstrong over IRC: http://hg.mozilla.org/mozilla-central/rev/5ec0d33625c1
Depends on: 568480
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.