New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support @icon #1096
Comments
Well I've gone ahead and created a patch for this issue, take a look and see what you think, it needs work, but works.. 8156c4371792f9ffa8b2524f3bd66ecc7b8b4142 This patch accepts the |
I'd be -1 on this sort of change at least until scripts-in-addons is finalized, so that we have some sort of idea how it might work, if at all. |
just added a default icon for when @ICON is not defined: bd22087d1225e0e5bcac235de54f76e73f230920 |
Just so everyone is aware, IEScripts supports |
cool, thanks for pointing that out, do they have any documentation on it that you know of? In my patch the icon size can be anything, so assuming that ie7pro's @Defaulticon is not meant to be 32x32 (although it may be idk) then it should still work as an icon for GM. So we can adopt @Defaulticon (which I don't want to do), or we can accept @Defaulticon as a secondary choice (which I prefer), or we can ignore it. |
I'm all for "reuse" but this is a matter for Johan and Anthony to decide on the name... adding yet another key/value that uses the same standard that IE7Pro defined (and I think trixie might do this too) seems a bit redundant. I do know that the IEScripts website utilizes this key to display a script icon, but it can easily be turned around to show in the GM addons tab (when finished) as well. The rules for images should probably follow |
Here is a clean branch for this update. |
@sizzlemctwizzle (or anyone else interested) can you please review it to ensure that I integrated this branch with issue 1092 correctly, when you have a chance? I've tested it and it seems to work, but I'd like you to double check if possible. |
Ran into another error from the live meta portion. Here's the patch. With that cherry-pick applied to your branch, the live meta seems to work properly and without any errors. |
Never mind, I see you mentioned this is in issue 1092 already. |
Thanks for reviewing this sizzle! very much appreciated. I'll look into using ScriptResource as you suggested asap. |
Yeah, I didn't spend much time on the code used to detect if the url provided is actually an image, that can certainly be improved. I could update the reg exp to support query strings, but there is probably a much better way that I haven't thought of yet. |
No I haven't.. why don't you give it a try? let me know how that goes, I'll be interested in what you discover. |
erikvold wrote: Just fooled it with a bogus QSP though... lots to check out including when Image object was implemented ... if it was just recently then there is no point to exploring that avenue. Well I found a vague reference to it being supported in IE4+ and NN3+ and of course FF, but Image appears not to be a valid Object in chrome: for some reason. Dove into the logistics of how this is saving the file and I'm not real thrilled with it keeping the original filename since there can possibly be an exploit via a fragment or a bogus QSP and essentially kill any file... say test.user.js is the script and the icon is @ICON http://example.com/test.user.js#.jpg and all the sudden we have potential catastrophic failure. Even if the save routine renames it... it would have a false identification. This could also conflict with @require filenaming as well. There is always the XPCOM interface library to load an image but I'm not sure anyone has dealt with that on GM before. Another alternative is to create an invisible XUL image tag and set it's src in JavaScript and if it fails then it's not valid and skip ahead to the default image... although this is sort of a cheat. |
Isn't it? I thought I had done this.. |
Nvm, looks good. You should probably do a check of channel.contentType here like you do on the data:uri. I'm against checking the extension of the url for this. Content type seems like a more flexible solution. |
Yeah, I agree. |
|
Can you link to (and create if necessary) a branch that is synced to a recent version of upstream's head, and contains just the changes for this issue? |
here's my issue-1096 branch, I just did a pull of GM/master. |
It would be a really good idea to get written permission from whoever owns the copyright on that icon, before we consider taking it. Please check the style guide, I see at least a few I don't like the way I see private members being accessed outside their object, this should be avoided. |
Corrected now, see compare view of branch here.
I corrected this to the extent that GM seems to follow this rule, script private members are accessed copiously w/in config.js and scriptdownloader.js atm. |
Erik, re: your issue-1096 branch: config.js line 298: please comment with proper sentences (capitzalization, punctuation) and spelling (same throughout) I think I'd generally prefer all the logic for handling both data: and other URIs to be encapsulated inside the scripticon class, rather than tested for and handled separately in lots of places. I've bumped this from 0.9.x into 0.9.0, it's about time for it to happen. I think we've done as much diligence as we can regarding the image, and we're not likely to be sued. |
re: your issue-1096 branch:Patched with this branch. |
The last one might throw a minor wrench in the branch as it is now, I haven't tried it out yet though.Yeah I got: Error loading dependency data:text/html;charset=utf-8, SecurityException: Request to local and chrome url's is forbiddenThat's probably not the error we want to show(at least not the Security Execption part). Also the install link wouldn't do anything and I only saw that error when I pressed cancel. |
Prob the best way to deal with this is to throw a custom invalid @iconURL error when the url is a data: url but not a |
throw a custom invalid @iconURL errorPatch. |
Merge remote branch 'sizzlemctwizzle/issue-1096' Closed by 545d5a4 |
I'd like to use @ICON or @iconURL to define an icon to be used in the 'User Scripts' addons tab see issue #1086 which this issue depends on.
I'd like to say the icon needs to be 32x32 like iconURL for Firefox extensions, but it turns out that for FF 3.6 (which is all that I've tested) the image is made to fit 32x32 when it's larger than that (I haven't tried smaller images), so we might be able to accept any size..
Also the url can be http://, I've tried it, so we don't have to prefetch the image, but perhaps we should. One way to do this is to prefetch the @ICON like a @resource or @require, another way is to take url's provided to @ICON literally, or otherwise take anything not a url to be a @resource reference (if a match is found).
Your thoughts?
The text was updated successfully, but these errors were encountered: