Allow relative path for icon url #498
Conversation
|
@johannhof since you are more familiar, could you review this patch? I think it should be fine since it's not moving files around, right? |
|
@kumar303 sure, I'll take a look! The main issue is that this patch (and the earlier patch) are using
So I would recommend going for this approach, however I'm a bit biased since I wrote the initial patch. Do you know what's up with |
|
Thanks for the detailed info. The fact that it's completely broken as is sounds like a good enough reason to go ahead with this patch. @yan12125 yes, please continue the patch with tests. |
|
@johannhof also, thanks for your continued help on jpm. I added you as a project member so that you can also help merge patches if you'd like to in the future. |
| @@ -64,6 +75,15 @@ function createRDF(manifest) { | |||
| delete jetpackMeta["em:icon64URL"]; | |||
| } | |||
|
|
|||
| if (needsBootstrapJS) { | |||
There was a problem hiding this comment.
Why would you only do this if no bootstrap.js is present? Is there a reason for that if clause?
There was a problem hiding this comment.
When bootstrap.js is not provided by the addon developer, the default bootstrap.js is used. With this bootstrap.js, the addon's resource:// URI registration occurs in resource://gre/modules/commonjs/sdk/addon/bootstrap.js. If a custom bootstrap.js is provided, the program flow mentioned above may not occur, so the real icon URLs are unknow.
There was a problem hiding this comment.
Huh, I didn't know this was done in bootstrap.js. In any case I suppose we can trust developers using custom bootstrap.js with this. Can you please add a very short comment above that line so that people like me understand what's going on? 😅
|
@yan12125 in https://github.com/mozilla-jetpack/jpm/pull/267/files#diff-dc54654b4741edd9be2ea246c9563573R61 it logs a warning that using the manifest icon key is not recommended. It would be cool if could include a warning like that. |
https://developer.mozilla.org/en-US/Add-ons/Install_Manifests says: For me the wording you can also indicates both ways are acceptable, and neither is more preferred than another. Are there texts explicit recommending not using For the following statement: Things may have been changed since this sentence is written. On Firefox 44.0.2, at least EPUBReader, GreaseMonkey, Advanced Cookie Manager and Anonymox uses As stated in https://bugzilla.mozilla.org/show_bug.cgi?id=1141839, relative paths should be supported, and with some hacks in jpm it really works. I don't think warnings here are necessary. Finally, concerning another statement in https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#iconURL: There should be a note in this section. Something like "relative paths are supported if you're using jpm >= x.y.z" if this PR is finally merged. |
|
@johannhof Can I adapt your test cases from #267 in this PR? |
I can still reproduce it on my nightly build using a dummy add-on. Did you reload the page after disabling them? Also, they might still have an Apart from that, I would like a warning because falling back to
Sure! |
|
@yan12125 do you need any help finalizing this patch? :) |
|
This needs to be rebased on master to resolve the conflicts. |
|
Rebased and tests added. |
|
Ok, thanks! I see you didn't add a warning, which is not a breaker for me. We can make a small follow-up bug for this. Thank you for contributing. 👍 |
|
See #522 |
Fixes #197 and fixes #341.
It's a workaround before https://bugzilla.mozilla.org/show_bug.cgi?id=1141839 is resolved. Tested with https://github.com/whuhacker/Unblock-Youku-Firefox on Linux. If this patch is OK I'll try to add some tests.