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 @icon metadata key, which can be a data uri scheme, or a url to a file which is downloaded when the userscript is installed. The image should be 32x32 but anything seems to work so far..

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

erikvold wrote: until what is finalized?...

Most likely #1012 as well... and most likely it should work but it needs to follow the guidelines for em:iconUrl.

+1 for when the addons tab is completed for all supported platforms but 0 for now.

Just so everyone is aware, IEScripts supports // @defaulticon ie7pro://pluginpath/bg.png in case GM doesn't want to completely reinvent the wheel. (e.g. @Defaulticon URI)

Just so everyone is aware, IEScripts supports // @defaulticon

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 em:iconUrl as most likely the "reusable code" that will probably be used in the addons dialog will require this or at very least resize it to a 32x32 matrix. Check with MDC here.

Here is a clean branch for this update.

Here are two tests:
Test 1
Test 2

@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.

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.

This should be applied upstream right? (ie: applied to the master branch). If so, then request a pull from via the gm-dev mailing list, the GitHub pull request system has many faults.

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.

erikvold wrote:
... interested...

Not too shabby although needs to support QSPs (?yadda=yidda&that=this&this...) on urls such as the gravatar... currently if a query string is used it reverts to the standard script icon. Fragment (#) support would also be a plus. :) That's a big puppy too ;)

Not too shabby although needs to support QSPs on urls such as the gravatar... currently if a query string is used it reverts to the standard script icon.

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.

erikvold wrote:
probably a much better way

Have you tried a simpler method, maybe new Image(), instead of using the GM internal API's and throwing junk at it too to see how it responds? I don't recall how far back Image object goes but might be worth a try.

Have you tried a simpler method, maybe new Image(), instead of using the GM internal API's and throwing junk at it too to see how it responds? I don't recall how far back Image object goes but might be worth a try.

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:
let me know

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.

If the @ICON file is downloaded like a @require/@resource(@resource seems like a better solution to me), it needs to be integrated with the live meta code. This means that it needs to be included in the dependhash and when it changes it should be redownloaded(along with the other dependencies).

Isn't it? I thought I had done this..

Could be. I didn't see it the first time I looked. I'll have to inspect closer when I get home.

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.

Content type seems like a more flexible solution.

Yeah, I agree.

Content type seems like a more flexible solution.

patch

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 if( with no space.

I don't like the way data: URIs are handled, checked for and mocked differently multiple times. Perhaps a new ScriptIcon object, extended from ScriptResource to handle this logic in one place, consistently?

I see private members being accessed outside their object, this should be avoided.

I don't like the way data: URIs are handled, checked for and mocked differently multiple times. Perhaps a new ScriptIcon object, extended from ScriptResource to handle this logic in one place, consistently?

Corrected now, see compare view of branch here.

I see private members being accessed outside their object, this should be avoided.

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.

http://groups.google.com/group/greasemonkey-dev/t/e659c941ceff879f

Erik, re: your issue-1096 branch:

config.js line 298: please comment with proper sentences (capitzalization, punctuation) and spelling (same throughout)
script.js: if things are working, please remove the GM_log() calls. we've gotten complaints in the past about excessive logging
scriptdownloader.js line 108: that comment doesn't seem to be related to the code just below it, what's going on there?
scriptdownloader.js line ~180: please put the closing ) { for the if on its own line, please indent continued lines by 4 spaces

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:

Thank you sizzle!

Here are some test cases:

• Test 1 - http: img
• Test 2 - data: img
• Test 3 - http: text file
• Test 4 - http: missing file
• Test 5 - non img data: url

The last one might throw a minor wrench in the branch as it is now, I haven't tried it out yet though.

The last one might throw a minor wrench in the branch as it is now, I haven't tried it out yet though.

Error loading dependency data:text/html;charset=utf-8,
SecurityException: Request to local and chrome url's is forbidden

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 /^data:image\//i url

throw a custom invalid @iconURL error

Merge remote branch 'sizzlemctwizzle/issue-1096'

Closed by 545d5a4