588119 - package.json should support add-on icon

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Dietrich Ayala (:dietrich)]

jetpack addons should be able to package and specify an icon to represent itself in the add-ons mgr and AMO, etc.

Comment 1

[Atul Varma [:atul]]

Attachment: initial patch, no unit tests.

Comment 2

[Atul Varma [:atul]]

Oh, forgot to note some things about this patch:

* It doesn't add an iconURL property to install.rdf in generated XPIs, it just dumps the image as "icon.png" in the root of the XPI, which bsterne learned about through mossop. This has lots of advantages, most notably that we don't need to figure out what an absolute URL to an icon would be.

* The package.json key being looked for is 'icon', and its value is expected to be the path to a PNG file relative to the location of package.json. By default, it's 'icon.png' if that file exists. This means a developer can actually just drop an icon.png into the root of their jetpack package dir (alongside package.json) and everything should just work.

* Note that the above bullet should be documented in the 'static-files/md/dev-guide/package-spec.md' file!

* We should make sure that the name 'icon' doesn't conflict with the CommonJS package standard somehow.

Comment 3

[Atul Varma [:atul]]

Myk, I'm adding this as to the 0.8 milestone--it shouldn't be hard to finish up and land, and I think being able to set an icon for one's addon easily has a lot of value.

Comment 4

[Brian Warner [:warner :bwarner]]

Comment on attachment 467044 [details] [diff] [review]
initial patch, no unit tests.

looks mostly ok, but a few concerns:
 - harness_options['icon'] is used to tell build_xpi() what file to copy into the XPI, but the option is also copied into the XPI's harness-options.json . That means we're copying a source pathname (including the user's home directory name, etc) into the XPI, where it can't possibly be useful. I don't think we should leave this in harness_options.json, but I don't know of a better way to inform build_xpi() about the path.
 - I think packaging.py:is_file() (and is_dir, for that matter) are redundant: if you look at the implementation of os.path.isfile, it effectively does the os.path.exists() first. I'd replace them with plain os.path.isfile/isdir .
 - docs and tests, of course

I'm r- just because of the home-directory leakage and the lack of docs (since I don't think this feature is very discoverable without docs). I'm less concerned about tests, but of course they'd be nice to have. Everything else seems fine to me. If there's no other way to get the icon pathname to build_xpi(), then I'm willing to see it left in harness-options.json, but it'd be preferable to get it out of there.

Comment 5

[Atul Varma [:atul]]

O snap, good point.

I've "ported" your review to github here:

http://github.com/toolness/jetpack-sdk/commit/5a73f1b4b071b5e3995e8c05c1a384e0089c1264

I'll issue a pull request on github for the next code review that includes the originally reviewed patch and the modifications to it, so that you don't have to review the whole thing all over again (it's not that big a deal in this case, but I'd like to try out the workflow).

Comment 6

[Atul Varma [:atul]]

Attachment: Second attempt, on github.

Comment 7

[Brian Warner [:warner :bwarner]]

oops, the recent rename broke the pull-request link.. you might want to update your HTML attachment

Comment 8

[Brian Warner [:warner :bwarner]]

looking good. One bug I think I noticed:

 1: use the new 'cfx init' to create a skeleton
 2: create nonicon.png
 3: edit package.json to set "icon": "nonicon.png"
 4: cfx xpi
 5: unpack xpi
 6: good: there's a file named icon.png with the contents of nonicon.png
 7: bad: harness-options.json has "icon": "nonicon.png"

Does the incorrect path in harness-options.json affect anything? Seems to me that either harness-options.json should always say "icon.png", or the zipfile name should be copied from the basename of the package.json's icon: property.

Comment 9

[Atul Varma [:atul]]

O snap, good catch. I will fix it and rebust.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 11

[Brian Warner [:warner :bwarner]]

Comment on attachment 481357 [details]
Second attempt, on github.

I should have marked this attachment back when I added the comment about the likely bug.. oops.

Comment 12

[Atul Varma [:atul]]

Attachment: Pointer to pull request

Comment 13

[Atul Varma [:atul]]

Comment on attachment 492826 [details]
Pointer to pull request

I think this fixes your concerns in comment 8.

Comment 14

[Brian Warner [:warner :bwarner]]

Comment on attachment 492826 [details]
Pointer to pull request

looks good to me. I'm seeing unrelated problems while testing (Bug 614699), but I think the icon is showing up properly before those problems get in the way.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 492826 [details]
Pointer to pull request

The size of this patch is a bit scary, but its code seems straightforward and safe enough. a=myk

Comment 16

[Atul Varma [:atul]]

Pushed:

https://github.com/mozilla/addon-sdk/commit/5792daa3ddf605935aaec3d4308dd38e03820487