Include icons in xpi base path (fixes #197)

[johannhof]

This fixes icons not showing when they were not placed as icon.png in the base directory (fixing #197). I tried to do the same thing cfx does, automatically including the icon file in the xpi root.

Also some tests! 🎉

Even though I do it here, I'm of the general opinion that we should be careful with creating more temporary files and cleaning them up after the xpi is created. This can only be done to a certain extent, as it comes with a lot of overhead (checking if the file exists, testing that we do not overwrite/delete the file) that could be avoided by using a tmp location.

[erikvold]

@johannhof so it doesn't appear possible to use something like { "icon": "chrome://addon/style/main.css" }, a custom icon uri, often remote uris are used here too, like { "icon": "https://google.com/icon.png" }

[johannhof]

@erikvold That's not possible with cfx, either. I tried to stay with the cfx behaviour and it refuses to assemble the xpi when you add a url as the icon path in package.json, as far as I know.

If you'd like me to, I can try to add this functionality to jpm, but I prefer the cfx approach of just requiring a valid local .png file

[erikvold]

@johannhof it's sad cfx doesn't accept urls.. I'm not sure why that would be. Anyhow it's not an AMO policy and I don't see any reason to force people to use a local icon.

[johannhof]

I guess you're right, forcing that on anyone wouldn't make sense. Still, having an icon.png has some advantages (see https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#iconURL) and I'd still copy it if the icon is local anyway. So how about:

• if the icon is local, copy it to icon.png
• otherwise put the iconUrl field in install.rdf

[johannhof]

The last commit (3ebef1d) implements the changes described in my above comment. Urls are now included in the install.rdf.

[erikvold]

hey @johannhof I'm sorry if I wasn't more clear earlier, but I have to r- this because we don't want jpm do move files around, and this patch is moving icons from one path to another in one use case.

The rest is great!

If you could separate out the relative path use case from the rest then I can r+ this. As for the relative path use case, I think that is fine to support, but we should not support it by having jpm move files, we should support it on the Firefox side, and have the add-on manager accept relative urls instead.

CFX moved files, and it was a curse, we decided JPM should not move files, and the fact that it adds a bootstrap.js and install.rdf when xpi'ing is just means to getting to the point when we don't need this files.

[johannhof]

Ok, good
That's kind of what I was trying to say in my initial comment. I dislike the file moving/temporary files but I was trying to get as close to cfx as possible. It's good to have a decision on that. I'll ask more closely next time.

I'll extract the non-moving parts and make a new PR. :)