The Chrome browserAction spec (https://developer.chrome.com/extensions/browserAction#icon) says "Browser action icons can be up to 19 dips", but Firefox's native icon sizes are: 18px/36px for the toolbar (regular and retina) 32px/64px for the menu panel (regular and retina) We should choose better sizes, where possible, and not restrict ourselves to 19 and 38, so that the icons don't end up all blurry in the menu panel.

I highly agree not to restrict ourselves to 19 and 38. We should just allow them all added, and we'll use 18 and 32 when available. Although are you sure the toolbar is 18 i thought it was 16x16 img with some padding.

I'll take this one. :)

Once bug 1197422 lands, it should simply be a matter of adding the additional sizes to the array of accepted sizes, and adding a few more tests.

This should be safe to work on now, as long as it's on top of https://hg.mozilla.org/integration/fx-team/rev/958f04b7804fa587506b35b30ec616f986b637ca I'd suggest choosing the correct icon with something along the following lines: The preferred size should be `Math.round(16 * window.devicePixelRatio)`. If an icon of that exact size exists, use it. Otherwise, fall back to, in order of preference: * An icon that's exactly twice that size. * The smallest icon that's larger than that size. * The largest icon that's smaller than that size. Though it may be better to just try for the exact size, twice the preferred size, and then just fall back to the largest.

I happen to have implemented _exactly_ that logic in bug 1192432. The function is called AddonManager.getPreferredIconURL and I reckon we can use it in this case as well. We would need to wait for 1192432 to land. Cheers

Oh, perfect.

Comment on attachment 8681706 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils So 1192432 landed and I finally got around doing this. It's lacking tests right now because I'd first like to know if you think this would be a sane approach, Kris. It should be pretty straightforward, we remove the SIZES array to not limit ourselves anymore and then delegate to AddonManager.getPreferredIconURL to sort out the correct icon for us (from the list of normalized icons). The only thing I don't like is that getPreferredIconURL is expecting to receive a traditional addon object as it is used internally and so we have to cheat it a bit and pass {icons: icons}. Note that I left the desired size at 19 for now, because this issue had differing opinions on what the actual desired size would be. But that's trivial to change.

Comment on attachment 8681706 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8681706 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Johann Hofmann from comment #8) > The only thing I don't like is that getPreferredIconURL is expecting to > receive a traditional addon object as it is used internally and so we have > to cheat it a bit and pass {icons: icons}. I don't really have a problem with that, but you should update the comment at the head of that function to make it clear that it can take any object with an |icons| property, and not just an Addon instance. > Note that I left the desired size at 19 for now, because this issue had > differing opinions on what the actual desired size would be. But that's > trivial to change. I think it would be better to change the desired size to 18. The default size for a string, rather than an object, should stay at 19, though. It's probably going to take a fair amount of work to support 32/64px images in the menu panel, though, so that should probably be a separate bug. As a side note, I see that |getPreferredIconURL| is using |parseInt| to check that the sizes are actually integers. It might be a good idea to add a stricter check there, since |parseInt| will happilly accept values like "100foo" without complaint. Aside from those minor nits, this looks good. ::: browser/components/extensions/ext-utils.js @@ +23,3 @@ > > // Normalizes the various acceptable input formats into an object > + // containing icon URLs. Please include some kind of description of the keys. @@ +32,5 @@ > if (imageData instanceof Cu.getGlobalForObject(imageData).ImageData) { > imageData = {"19": imageData}; > } > > + for (let size in imageData) { It would be good to check that |size| is an integer here. Below, too. Also, I'd prefer that you use |let size of Object.keys(imageData)|, for paranoia's sake.

(In reply to Kris Maglione [:kmag] from comment #9) > Comment on attachment 8681706 [details] [diff] [review] > Allow flexible icon sizes in ExtensionUtils > > Review of attachment 8681706 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's probably going to take a fair amount of work to support > 32/64px images in the menu panel, though, so that should > probably be a separate bug. > > As a side note, I see that |getPreferredIconURL| is using > |parseInt| to check that the sizes are actually integers. It > might be a good idea to add a stricter check there, since > |parseInt| will happilly accept values like "100foo" without > complaint. > It would be good to check that |size| is an integer here. Turns out that isNaN would be working pretty well for both use cases, so we could put an isNaN check in there. Confirming that it's an integer would be harder and possibly involve bitwise operators or regexes. I'm not sure we should go down that route. Although I have to say I like that people could input "32px" and it would be coerced into 32. > I'd prefer that you use |let size of Object.keys(imageData)|, for paranoia's > sake. Haha good call.

(In reply to Johann Hofmann from comment #10) > Turns out that isNaN would be working pretty well for both use cases, so we > could put an isNaN check in there. Confirming that it's an integer would be > harder and possibly involve bitwise operators or regexes. I'm not sure we > should go down that route. Alas, isNaN also returns false for things like '0x100'. Which isn't a big deal, I guess, but still not really desirable. Your best bet is probably to just go with `/^[1-9]\d*$/.test(key)`. This is JavaScript, after all. Sometimes hoping for a clean solution is just too much. > Although I have to say I like that people could input "32px" and it would be > coerced into 32. It sounds nice, but I think it's asking for trouble. If we accept values like that, some number of people will start using them, and some of those will write tutorials, and people will copy code from those. At which point some of those people will wind up trying values like "1em". And eventually we'll make some change to the code so that it only accepts integers, and all of that code will break. I know it sounds like a pretty minor thing to worry about, but it's the kind of thing that happens all the time, and makes it so difficult for us to change things without breaking add-ons.

(In reply to Kris Maglione [:kmag] from comment #11) > (In reply to Johann Hofmann from comment #10) > > Turns out that isNaN would be working pretty well for both use cases, so we > > could put an isNaN check in there. Confirming that it's an integer would be > > harder and possibly involve bitwise operators or regexes. I'm not sure we > > should go down that route. > > Alas, isNaN also returns false for things like '0x100'. Which isn't a big > deal, I guess, but still not really desirable. Your best bet is probably to > just go with `/^[1-9]\d*$/.test(key)`. This is JavaScript, after all. > Sometimes hoping for a clean solution is just too much. Ugh, okay, it's probably for the best. :) > > Although I have to say I like that people could input "32px" and it would be > > coerced into 32. > > It sounds nice, but I think it's asking for trouble. If we accept values > like that, some number of people will start using them, and some of those > will write tutorials, and people will copy code from those. At which point > some of those people will wind up trying values like "1em". And eventually > we'll make some change to the code so that it only accepts integers, and all > of that code will break. > > I know it sounds like a pretty minor thing to worry about, but it's the kind > of thing that happens all the time, and makes it so difficult for us to > change things without breaking add-ons. Yeah, absolutely. I meant that rather jokingly :D

(In reply to Johann Hofmann from comment #12) > Yeah, absolutely. I meant that rather jokingly :D Heh. Fair enough :)

Comment on attachment 8683091 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Blake, would you like to ui-review? I'm not sure there's much to see, but it's much more flexible in the background now. Just wanted to pull you in on this since you created the bug.

Comment on attachment 8683091 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils I'm going to give it a ui-r+, based on inspecting the tests. ;) As a side note, we might want to think about how we want to handle SVGs, which can be any resolution, particularly since the style guide is likely to recommend people use SVG icons, both for resolution independence, and to let us add a highlight colour to browserActions and pageActions when appropriate. But that all seems like food for another bug.

Haha ok, thanks :D Yeah icons seems to be traditionally not very straightforward. Considering SVGs early on might save us some pain.

(In reply to Blake Winton (:bwinton) (:☕️) from comment #16) > As a side note, we might want to think about how we want to handle SVGs, > which can be any resolution, particularly since the style guide is likely to > recommend people use SVG icons, both for resolution independence, and to let > us add a highlight colour to browserActions and pageActions when > appropriate. I don't think we need to do anything special for SVGs. It's conceivable that people may want to use different SVGs for different icon sizes, or use a PNG for smaller icons, where exact pixel placement may matter more, and SVGs for everything else. As it stands, people can just specify a string pointing to an SVG, rather than an object with multiple sizes, and get the correct behavior, so I can't think of any changes that would be especially useful.

Comment on attachment 8683091 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8683091 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good. Thanks. Just needs a couple of minor changes. I'm not a reviewer for AddonManager, so flagging Mossop for that, but those changes look good to me too. ::: browser/components/extensions/ext-utils.js @@ +36,5 @@ > } > > + for (let size of Object.keys(imageData)) { > + if (!INTEGER.test(size)) { > + throw Components.Exception("Invalid icon size, must be an integer", This isn't an XPCOM interface, so please use |new Error| instead of |Components.Exception|. Same below. @@ +71,5 @@ > + try { > + Services.scriptSecurityManager.checkLoadURIStrWithPrincipal( > + extension.principal, url, > + Services.scriptSecurityManager.DISALLOW_SCRIPT); > + } catch (e if !context) { Since we're now throwing validation errors elsewhere in the function, can you move this try-catch to the top level, and return an empty object if you hit it? The message should be something like |`Invalid icon data: ${e}`| ::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js @@ +122,5 @@ > + "48": "48.png", > + "128": "128.png" } }, > + resolutions: { > + "1": browser.runtime.getURL("data/18.png"), > + "2": browser.runtime.getURL("data/48.png"), } }, Nice tests. @@ +270,5 @@ > + } > + }); > + > + yield extension.startup(); > + yield extension.awaitFinish(); Unfortunately, there's currently a race in code like this. Please use |Promise.all([extension.startup(), extension.awaitFinish()])| so the result message doesn't get lost. Also, it's best to pass an explicit message to |awaitFinish| so we're certain the correct extension is being loaded.

Great, will do! > This isn't an XPCOM interface, so please use |new Error| instead of |Components.Exception|. Same below. Oh cool, I was wondering about that. :)

(In reply to Kris Maglione [:kmag] from comment #18) > (In reply to Blake Winton (:bwinton) (:☕️) from comment #16) > > As a side note, we might want to think about how we want to handle SVGs, > > which can be any resolution, particularly since the style guide is likely to > > recommend people use SVG icons, both for resolution independence, and to let > > us add a highlight colour to browserActions and pageActions when > > appropriate. > > I don't think we need to do anything special for SVGs. It's conceivable that > people may want to use different SVGs for different icon sizes, or use a PNG > for smaller icons, where exact pixel placement may matter more, and SVGs for > everything else. As it stands, people can just specify a string pointing to > an SVG, rather than an object with multiple sizes, and get the correct > behavior, so I can't think of any changes that would be especially useful. I mostly agree, but the line that worried me a little was: 'this.convertImageDataToPNG(imageData[size], context);' ;)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #21) > I mostly agree, but the line that worried me a little was: > 'this.convertImageDataToPNG(imageData[size], context);' ;) That's only for ImageData objects :) They're always raster anyway. Image URLs stay as image URLs.

Comment on attachment 8684083 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Mossop, this should be ready to review now Kris, I implemented the changes you suggested, would be cool if you could take another look to confirm that's what you meant :)

Comment on attachment 8684083 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8684083 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, needs a bit more work. My fault for not being clearer. ::: browser/components/extensions/ext-browserAction.js @@ +39,5 @@ > popup = extension.baseURI.resolve(popup); > } > > + let icon = {}; > + try { I'd rather keep this try-catch inside the normalize function so we don't need it in two places. Sorry I wasn't clearer. @@ +45,5 @@ > + { path: options.default_icon }, > + extension, > + null, > + true > + ); Please keep this to two lines. ::: browser/components/extensions/ext-utils.js @@ +26,5 @@ > // Normalizes the various acceptable input formats into an object > + // with icon size as key and icon URL as value. > + // > + // throws an error if an invalid icon size was provided or the > + // extension is not allowed to load the specified resources Please capitalize the first word and add a period. @@ +70,5 @@ > + // The Chrome documentation specifies these parameters as > + // relative paths. We currently accept absolute URLs as well, > + // which means we need to check that the extension is allowed > + // to load them. > + try { No need for a try-catch here once we have an outer one. ::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js @@ +386,5 @@ > message: new RegExp(`Loading extension.*Access to.*'${url}' denied`), > }); > > + // because the underlying method throws an error on invalid data, > + // only the first invalid URL of each component will be logged Please always capitalize the first word of comments, and add a period when it's more than one line.

(In reply to Kris Maglione [:kmag] from comment #25) > Comment on attachment 8684083 [details] [diff] [review] > Allow flexible icon sizes in ExtensionUtils > > Review of attachment 8684083 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry, needs a bit more work. My fault for not being clearer. No worries, it's good that you're thorough. :) > ::: browser/components/extensions/ext-browserAction.js > @@ +39,5 @@ > > popup = extension.baseURI.resolve(popup); > > } > > > > + let icon = {}; > > + try { > > I'd rather keep this try-catch inside the normalize function so we don't > need it in two places. Sorry I wasn't clearer. > I actually assumed you meant that, but the problem is that the setIcon function in Chrome is throwing an error if the icon size is incorrect. If I put a big try-catch around that here it won't fail in Firefox anymore but rather just log a warning. Since it's a public facing API I think we should follow the behavior of the Chrome API. That's also the reason why I - didn't remove the inner try-catch and made it throw a better worded error - don't catch any exception in the normalize call in setIcon(), extension authors are supposed to handle it here

(In reply to Johann Hofmann [:johannh] from comment #26) > I actually assumed you meant that, but the problem is that the setIcon > function in Chrome is throwing an error if the icon size is incorrect. If I > put a big try-catch around that here it won't fail in Firefox anymore but > rather just log a warning. Since it's a public facing API I think we should > follow the behavior of the Chrome API. That's what the `if !context` guard in the current try-catch is for. Although, as catch guards are a non-standard feature, that should probably be replaced with something like `if (context) { throw e; }` in the catch block. > - didn't remove the inner try-catch and made it throw a better worded error That's such a rare corner case that I'm not worried about adding a second try-catch just for a better error. We need the check for security, but if people hit it, it's almost certainly because they're doing something shady.

(In reply to Kris Maglione [:kmag] from comment #27) > (In reply to Johann Hofmann [:johannh] from comment #26) > > I actually assumed you meant that, but the problem is that the setIcon > > function in Chrome is throwing an error if the icon size is incorrect. If I > > put a big try-catch around that here it won't fail in Firefox anymore but > > rather just log a warning. Since it's a public facing API I think we should > > follow the behavior of the Chrome API. > > That's what the `if !context` guard in the current try-catch is for. > Although, as catch guards are a non-standard feature, that should probably > be replaced with something like `if (context) { throw e; }` in the catch > block. Ooh right, I'm braindead. Didn't think of using that. Sorry, I'll prepare a new patch. > > - didn't remove the inner try-catch and made it throw a better worded error > > That's such a rare corner case that I'm not worried about adding a second > try-catch just for a better error. We need the check for security, but if > people hit it, it's almost certainly because they're doing something shady. Ok, if you say so :)

Comment on attachment 8684083 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8684083 [details] [diff] [review]: ----------------------------------------------------------------- I'll look once the new patch is ready.

Comment on attachment 8686052 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Ok, implemented the things you remarked. :) Want to give it another look before we pass it on to :Mossop?

Comment on attachment 8686052 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8686052 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks again!

Comment on attachment 8686052 [details] [diff] [review] Allow flexible icon sizes in ExtensionUtils Review of attachment 8686052 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the AddonManager pieces

Hi, this failed to apply: applying name.patch patching file browser/components/extensions/ext-utils.js Hunk #1 FAILED at 0 1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/ext-utils.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh name.patch Tomcats-MacBook-Pro-2:fx-team Tomcat$ can you take a look, thanks!

Sure, I'll update the patch. Thanks!

So the try is looking good I guess. Kris, could you give it a quick look to make sure I merged correctly? Thanks!

Looks good to me

Thanks Johann! Your contribution has been added to https://wiki.mozilla.org/Add-ons/Contribute/Recognition#November_2015

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action, in particular https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Choosing_icon_sizes Let me know if this is all right. Your comment 4 talked about picking an icon twice the best size, but I didn't see that in the code: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2301.

(In reply to Will Bamberg [:wbamberg] from comment #45) > Your comment 4 talked about picking an icon twice the best size, but I > didn't see that in the code: > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > AddonManager.jsm#2301. Ah. Indeed, it does not. *shrug* > -> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > browser_action, in particular > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > browser_action#Choosing_icon_sizes > > Let me know if this is all right. It should also mention menu panel buttons, which use 32px or 64px icons, depending on pixel density.

OK, I've added that: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Choosing_icon_sizes . (ultimately this is the kind if thing we should have in a separate UX guide, I think)

(In reply to Will Bamberg [:wbamberg] from comment #47) > OK, I've added that: > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > browser_action#Choosing_icon_sizes . Looks good. > (ultimately this is the kind if thing we should have in a separate UX guide, > I think) Agreed. I think Blake's working on that.

It'll live somewhere under http://firefoxux.github.io/StyleGuide/#/home (That part in particular is planned to be filled in by the end of this week. :)