Support custom icons in Web Extensions

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: mossop, Assigned: johannh)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(1 attachment, 11 obsolete attachments)

26.68 KB, patch
mossop
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
We need to parse the manifest's icons property

Comment 1

2 years ago
We can set a custom icon in browserAction right now, this refer to something different?
(Assignee)

Comment 2

2 years ago
@cesar I'm fairly sure it refers to the icon in e.g. the extension manager.

I'll give this a try if it's okay. Can somebody assign me?
Assignee: nobody → mail
(Assignee)

Comment 3

2 years ago
Created attachment 8666254 [details] [diff] [review]
Support custom icons in Web Extensions
Attachment #8666254 - Flags: feedback?(wmccloskey)
Attachment #8666254 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 4

2 years ago
Created attachment 8666266 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8666254 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8666266 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 5

2 years ago
Created attachment 8666279 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8666266 - Attachment is obsolete: true
Attachment #8666266 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 6

2 years ago
Created attachment 8666945 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8666279 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Comment on attachment 8666945 [details] [diff] [review]
Support custom icons in Web Extensions

@billm I'd like to know if you think this approach of selecting an icon is good enough or if I should do it differently :)
Attachment #8666945 - Flags: feedback?(wmccloskey)
Comment on attachment 8666945 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8666945 [details] [diff] [review]:
-----------------------------------------------------------------

This seems pretty reasonable to me. Blake, do you have any thoughts about this? Chrome asks developers to provide 16x16, 48x48, and 128x128 icons. As far as I can tell, the add-on manager sort of looks for 32x32 and 64x64, but it always prefers 64x64 regardless of the display resolution.

Ideally I think we would ask developers for different icon sizes and then actually display the best one for the user's monitor resolution.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +834,5 @@
> +
> +  let icons = getOptionalProp('icons');
> +
> +  if (icons) {
> +    // Try so select the icon that is closest to 48px

"Try to", not "Try so".

@@ +6430,5 @@
>          icons[size] = aAddon._repositoryAddon.icons[size];
>        }
>      }
> +
> +    if(aAddon.type === 'webextension'){

This should be formatted as:
if (aAddon.type === "webextension") {
(Extra spacing and double quotes.) Same for the stuff below.
Attachment #8666945 - Flags: ui-review?(bwinton)
Attachment #8666945 - Flags: feedback?(wmccloskey)
Attachment #8666945 - Flags: feedback?
Attachment #8666945 - Flags: feedback+
Attachment #8666945 - Flags: feedback?
(Assignee)

Comment 9

2 years ago
Created attachment 8667679 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8666945 - Attachment is obsolete: true
Attachment #8666945 - Flags: ui-review?(bwinton)
(Assignee)

Updated

2 years ago
Attachment #8667679 - Flags: ui-review?(bwinton)
Attachment #8667679 - Flags: review?(wmccloskey)
Comment on attachment 8667679 [details] [diff] [review]
Support custom icons in Web Extensions

So…  On my retina screen, the icon this chooses looks kind of ugly, because it's using the 1dpp version instead of the 2dpp version.  I'm not sure what we should do there, so let me think it out…

Looking into the code, it looks like the add-on manager should be looking for a 48x48 for the list view, and a 64x64 for the details view, but I really want to provide a 96x96 for the list view and a 128x128 for the details view for double-density screens, but providing a double-sized icon under the single-sized name seems like the wrong thing…

Looking at other addons (Well, 1Password on Chrome and Firefox), it looks like if they specify a size-48 icon, they'll get upscaled and be blurry on retina devices, so I suggest that we make two changes to this patch:
1) Look for a 96x96 icon for the iconURL, and a 128x128 icon for the icon64URL to handle double-density screens.
2) Instead of selecting the largest icon that's <= 48px and upscaling (which is guaranteed to give us a blurry image), we select the smallest icon that's >= 48px (or 96px, as per part 1), and let Firefox downscale it.

I think the ideal fix is probably to teach our add-on code about retina displays, and figure out something better, but while we wait for that, the suggestions above should give us something better than Chrome.  :)

And for clarity, my ui-r- isn't because I think the patch is horrible or anything, it's just because I'ld like to look over the next version before we ship it.  :)  Thanks!
Attachment #8667679 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 11

2 years ago
I think that sounds like a good suggestion. I'll make another patch :)
Comment on attachment 8667679 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8667679 [details] [diff] [review]:
-----------------------------------------------------------------

The formatting and test looks good. Thanks. I'll let you and Blake sort out the icon sizing issue.
Attachment #8667679 - Flags: review?(wmccloskey)
(Assignee)

Comment 13

2 years ago
Created attachment 8669399 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8667679 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Comment on attachment 8669399 [details] [diff] [review]
Support custom icons in Web Extensions

I played around a bit and basically came to this. :)

It's bit simpler than your suggestion, we basically choose the highest resolution up to 2x for both iconURL and icon64URL. It looks good in my tests, what do you think?
Attachment #8669399 - Flags: ui-review?(bwinton)
Just by code-inspection, I still don't think this is right.  If we have an 18-pixel and a 128-pixel image, we should use the 128-pixel image for both the 48x48 and 64x64 sizes, because scaling the 18-pixel image up for 48x48 is going to look much worse than scaling down the 128-pixel image.  Similarly, if we have a 64-pixel and a 256-pixel image, we should use the 256-pixel version, not the 64-pixel version.

(Now, I may be mis-reading the code, in which case I apologize and will find that out when I test it tomorrow, but in general, we should "choose the lowest resolution down to 2x", not "the highest resolution up to 2x".  Hopefully it'll be a fairly small change.)

Thanks!
(Assignee)

Comment 16

2 years ago
Created attachment 8671023 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8669399 - Attachment is obsolete: true
Attachment #8669399 - Flags: ui-review?(bwinton)
(Assignee)

Updated

2 years ago
Attachment #8671023 - Flags: ui-review?(bwinton)
Comment on attachment 8671023 [details] [diff] [review]
Support custom icons in Web Extensions

Looks great to me!  :D

I suspect we'll want to factor out the new code in toolkit/mozapps/extensions/internal/XPIProvider.jsm to its own function, but that can wait for a future patch…  :)

Oh, and another test to see what we do when there is no icon that's big enough would probably be good to add.  (I'm pretty sure we do the right thing, but it would be good to make sure…)

Thanks!
Attachment #8671023 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 18

2 years ago
Created attachment 8671455 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8671023 - Attachment is obsolete: true
(Assignee)

Comment 19

2 years ago
Comment on attachment 8671455 [details] [diff] [review]
Support custom icons in Web Extensions

I made another test to check that even smaller icons are considered if we have no other choice. I hope that's sufficient :)
Attachment #8671455 - Flags: ui-review?(bwinton)
Comment on attachment 8671455 [details] [diff] [review]
Support custom icons in Web Extensions

Looks good to me.  (As a side note, since you didn't change the UI, you could have carried forward my ui-r+…  :)

And I'm going to go ahead and ask Robert for the code review for you, since I'm here.
Attachment #8671455 - Flags: ui-review?(bwinton)
Attachment #8671455 - Flags: ui-review+
Attachment #8671455 - Flags: review?(rhelmer)
(Looking back, BillM might have been a better choice…  Oh well, I'm sure he can steal the review if he feels like it.)
(Reporter)

Comment 22

2 years ago
Comment on attachment 8671455 [details] [diff] [review]
Support custom icons in Web Extensions

Going to steal this as I've been keeping an eye on this already
Attachment #8671455 - Flags: review?(rhelmer) → review?(dtownsend)
(Assignee)

Comment 23

2 years ago
And yes, I'll probably have to factor that logic out anyway in https://bugzilla.mozilla.org/show_bug.cgi?id=1200674
(Reporter)

Comment 24

2 years ago
Comment on attachment 8671455 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8671455 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is attempting to do two different things. First get custom WebExtension icons working and second make us select retina appropriate icons. We really should be separating those two so we can do things properly. In particular there are two problems in this approach:

First the exposed properties are very unexpected. addon.icons[32] is an icon of at least 96px, addon.icons[64] is an icon of at least 128px. Likewise addon.iconURL and addon.icon64URL is much larger than for any other type of extension and I don't really know if everywhere we use them have the appropriate size constraints to make that work.

Because of this the icons used are preferential to users with retina displays. If an extension includes the google recommended set of 16, 48 and 128 then the iconURL will end up being 128px large which for a non-retina display is probably the wrong choice. Until we update everywhere we use icons with proper retina handling we can't have this do the right thing for both non-retina and retina so we should do the right thing for the majority of users and that is non-retina for now.

Instead what we should do here is persist the entire set of (validated) icons from the manifest and make AddonWrapper.iconURL use something as close to 32px as possible, likewise for icon64URL. Then if we want retina to work right file a second bug on doing that. I'd suggest some API like AddonManager.getPreferredIcon(addon.icons, size) which we can use for getting icons for display and can implement the appropriate logic centrally. It might even be useful to make that in this bug for AddonWrapper.iconURL and icon64URL to use.
Attachment #8671455 - Flags: review?(dtownsend) → feedback-
(Assignee)

Comment 25

2 years ago
Yes, I think your approach absolutely makes sense from a software engineering standpoint. We will have to make a followup bug for retina support though, and whoever implements that will have to do some of the hacking that we're trying to avoid right now :)

Maybe it would be easier to implement the "getPreferredIcon" functionality in the AddonWrapper(https://bugzilla.mozilla.org/attachment.cgi?id=8671455&action=diff#a/toolkit/mozapps/extensions/internal/XPIProvider.jsm_sec3), which seems to be widely used already. What do you think?

I would like to give Blake the chance to give his opinion on this before implementing it differently, since he initially suggested this solution.
(In reply to Dave Townsend [:mossop] from comment #24)
> This patch is attempting to do two different things. First get custom
> WebExtension icons working and second make us select retina appropriate
> icons. We really should be separating those two so we can do things
> properly.

That sounds reasonable.

> First the exposed properties are very unexpected. addon.icons[32] is an icon
> of at least 96px,

icons[32] isn't a great choice of variable name, given that https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json says that it "may be up to 48x48 pixels in size" (and has been that way since Firefox 4.0, according to https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#iconURL).  I think that seems like something we might want to address when we add the devicePixelRatio support, but I feel like part of the confusion results from that size being smaller than it should be.

> addon.iconURL and addon.icon64URL is much larger than for any other type of
> extension and I don't really know if everywhere we use them have the
> appropriate size constraints to make that work.

As someone who has been using double-sized icons in his SDK-based add-on for a while now, I can state with a good amount of confidence that we have the appropriate size constraints everywhere we use them.

> Because of this the icons used are preferential to users with retina displays.

Yes.  That was done on purpose for the following reasons: From what I hear, the breakdown is about 1/3 retina vs. 2/3 non-retina, which isn't a huge majority, and given the relative newness of retina screens, I'm going to make the unsupported claim that the percentage of retina screens will increase rather than decrease.  Also, the fallback of scaling a larger icon down for non-retina users is much nicer looking than scaling a small icon up for retina users, as demonstrated when we added High-DPI Windows support to Firefox.  We added 2x icons, and downscaled them for the 1.5x and 1.25x cases, instead of upscaling the 1x icons in that case, because upscaling lower-quality images gives a worse result than downscaling higher-quality images.  Finally the icons will already be there, so it won't incur any extra download cost, which is the usual reason I would recommend against using unnecessarily larger sizes.

> Until we update everywhere we use
> icons with proper retina handling we can't have this do the right thing for
> both non-retina and retina so we should do the right thing for the majority
> of users and that is non-retina for now.

Given the points above, can you agree that looking-for-retina-by-default is the better choice while we wait for the devicePixelRatio support to land?  If not, what would it take to convince you?

> Instead what we should do here is persist the entire set of (validated)
> icons from the manifest and make AddonWrapper.iconURL use something as close
> to 32px as possible, likewise for icon64URL.

See above re: 32 vs 48px.  Other than that, I can live with pushing the devicePixelRatio-supporting changes off to another bug, and the idea of persisting the entire set of icons from the manifest is a much better idea than only pulling out the ones we think we want now.  :)

(Hopefully the devicePixelRatio support can be as easy as checking Window.devicePixelRatio, and multiplying the requested size by that before looking for the image…)

One final thing…  Are you happy with the "smallest icon that's larger than the requested size" algorithm for "as close as possible", Mossop, or did you have a different algorithm in mind?
Flags: needinfo?(dtownsend)
(Reporter)

Comment 27

2 years ago
(In reply to Johann Hofmann from comment #25)
> Maybe it would be easier to implement the "getPreferredIcon" functionality
> in the
> AddonWrapper(https://bugzilla.mozilla.org/attachment.
> cgi?id=8671455&action=diff#a/toolkit/mozapps/extensions/internal/XPIProvider.
> jsm_sec3), which seems to be widely used already. What do you think?

The downside to this is that XPIProvider isn't the only add-on provider so you'd have to replicate the functionality in PluginProvider, LightweightThemeProvider, ... and that's ignoring add-on supplied providers. Using a shared function means we only need to implement this once. and it can also do work to reduce the existing inconsistencies between the providers. After thinking more I think something like this would be sane:

function getPreferredIcon(addon, size, window = undefined) {
  if (window) {
    // multiply size by window's scaling factor
  }

  let icons = addon.icons;
  if (!icons) {
    // build an array from addon's iconURL and icon64URL
  }

  // Find the best matching icon
}

Then for retina support we just have to replace cases where we use addon.iconURL with AddonManager.getPreferredIcon(addon, 48, window)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #26)
> (In reply to Dave Townsend [:mossop] from comment #24)
> > This patch is attempting to do two different things. First get custom
> > WebExtension icons working and second make us select retina appropriate
> > icons. We really should be separating those two so we can do things
> > properly.
> 
> That sounds reasonable.
> 
> > First the exposed properties are very unexpected. addon.icons[32] is an icon
> > of at least 96px,
> 
> icons[32] isn't a great choice of variable name, given that
> https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json says that
> it "may be up to 48x48 pixels in size" (and has been that way since Firefox
> 4.0, according to
> https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#iconURL).  I
> think that seems like something we might want to address when we add the
> devicePixelRatio support, but I feel like part of the confusion results from
> that size being smaller than it should be.

Yes, I'm not entirely sure how that ended up happening and it is a problem I'd like to fix too, it just isn't straightforward. For now we could just stick that icon into icons[32] and icons[48].

> > addon.iconURL and addon.icon64URL is much larger than for any other type of
> > extension and I don't really know if everywhere we use them have the
> > appropriate size constraints to make that work.
> 
> As someone who has been using double-sized icons in his SDK-based add-on for
> a while now, I can state with a good amount of confidence that we have the
> appropriate size constraints everywhere we use them.

Ok that's good to know.

> > Because of this the icons used are preferential to users with retina displays.
> 
> Yes.  That was done on purpose for the following reasons: From what I hear,
> the breakdown is about 1/3 retina vs. 2/3 non-retina, which isn't a huge
> majority, and given the relative newness of retina screens, I'm going to
> make the unsupported claim that the percentage of retina screens will
> increase rather than decrease.

The actual breakdown for release and beta users is more like 1/10 retina vs. 9/10 non-retina. That is a decent majority though I agree it is likely to increase over time.

> > Until we update everywhere we use
> > icons with proper retina handling we can't have this do the right thing for
> > both non-retina and retina so we should do the right thing for the majority
> > of users and that is non-retina for now.
> 
> Given the points above, can you agree that looking-for-retina-by-default is
> the better choice while we wait for the devicePixelRatio support to land? 
> If not, what would it take to convince you?

If it wasn't for it leaving us with a messed up API then I would agree. But APIs are sticky and if we don't end up doing the proper retina fix before the next merge then I don't want to ship an inconsistent API. It might seem small and maybe there would be no consequences in this case but we have a good path to do the right thing everywhere with a little more work, we don't need to take shortcuts to get there.

> > Instead what we should do here is persist the entire set of (validated)
> > icons from the manifest and make AddonWrapper.iconURL use something as close
> > to 32px as possible, likewise for icon64URL.
> 
> See above re: 32 vs 48px.  Other than that, I can live with pushing the
> devicePixelRatio-supporting changes off to another bug, and the idea of
> persisting the entire set of icons from the manifest is a much better idea
> than only pulling out the ones we think we want now.  :)

> (Hopefully the devicePixelRatio support can be as easy as checking
> Window.devicePixelRatio, and multiplying the requested size by that before
> looking for the image…)

I really think it will be pretty straightforward (though if we wanted icons to change when dragging between monitors that would be harder). I would hope that we could get the proper retina support done quickly. With the rough function I sketched out above it is mostly just replacing text in the tree.

> One final thing…  Are you happy with the "smallest icon that's larger than
> the requested size" algorithm for "as close as possible", Mossop, or did you
> have a different algorithm in mind?

larger than or equal to seems fine to me. I don't know if choosing integer multiples of a size in preference to others works better but I'm happy with whatever you want for that as a UI expert.
(Reporter)

Updated

2 years ago
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #27)
> (In reply to Johann Hofmann from comment #25)
> > Maybe it would be easier to implement the "getPreferredIcon" functionality
> > in the
> > AddonWrapper(https://bugzilla.mozilla.org/attachment.
> > cgi?id=8671455&action=diff#a/toolkit/mozapps/extensions/internal/XPIProvider.
> > jsm_sec3), which seems to be widely used already. What do you think?
> 
> The downside to this is that XPIProvider isn't the only add-on provider so
> you'd have to replicate the functionality in PluginProvider,
> LightweightThemeProvider, ... and that's ignoring add-on supplied providers.
> Using a shared function means we only need to implement this once. and it
> can also do work to reduce the existing inconsistencies between the
> providers. After thinking more I think something like this would be sane:
> 
> function getPreferredIcon(addon, size, window = undefined) {
>   if (window) {
>     // multiply size by window's scaling factor
>   }
> 
>   let icons = addon.icons;
>   if (!icons) {
>     // build an array from addon's iconURL and icon64URL
>   }
> 
>   // Find the best matching icon
> }
> 
> Then for retina support we just have to replace cases where we use
> addon.iconURL with AddonManager.getPreferredIcon(addon, 48, window)

This seems great to me.  As a side note, I believe we want surprisingly similar logic for getting icons for the toolbar buttons/menu buttons/urlbar icons, so making the multiply and find steps easy to split out to a utility function is probably a good idea…


> > icons[32] isn't a great choice of variable name
> Yes, I'm not entirely sure how that ended up happening and it is a problem
> I'd like to fix too, it just isn't straightforward. For now we could just
> stick that icon into icons[32] and icons[48].

Sounds like a reasonable short-term fix.


> The actual breakdown for release and beta users is more like 1/10 retina vs.
> 9/10 non-retina. That is a decent majority though I agree it is likely to
> increase over time.

I agree, that's a much larger majority.

> If it wasn't for it leaving us with a messed up API then I would agree. But
> APIs are sticky and if we don't end up doing the proper retina fix before
> the next merge then I don't want to ship an inconsistent API. It might seem
> small and maybe there would be no consequences in this case but we have a
> good path to do the right thing everywhere with a little more work, we don't
> need to take shortcuts to get there.

Okay.  I still worry a little about not getting the retina support in, but I suppose that will just motivate me to write a patch.  ;)

> > One final thing…  Are you happy with the "smallest icon that's larger than
> > the requested size" algorithm for "as close as possible", Mossop, or did you
> > have a different algorithm in mind?
> larger than or equal to seems fine to me. I don't know if choosing integer
> multiples of a size in preference to others works better but I'm happy with
> whatever you want for that as a UI expert.

Integer multiples would be better, but I don't think they would be better enough to justify the algorithmic complexity…  (Like, for a 48px target, would we prefer a 49px image or a 96px image?  I just don't know…  Particularly since add-on authors can pass in whatever they want for images.  :)
(Assignee)

Comment 29

2 years ago
Ok, let me summarize the changes we agree on, so that I'm sure I got this:

1. Keep the parsing and isnumeric checking of icon sizes
2. Persist all valid icons in the addon object
3. Don't put anything into addon.iconURL or addon.icon64URL
4. Implement the method that Mossop described in AddonManager

Something missing?

> Integer multiples would be better, but I don't think they would be better enough to justify the
> algorithmic complexity…  (Like, for a 48px target, would we prefer a 49px image or a 96px image?  I just
> don't know…  Particularly since add-on authors can pass in whatever they want for images.  :)

I think at some point we have to accept that add-on authors are also developers who should be able to experiment with icon sizes and see what looks best for their extension.

As soon as this lands I can also improve the MDN docs on icons in Webextensions.

> > (Hopefully the devicePixelRatio support can be as easy as checking
> > Window.devicePixelRatio, and multiplying the requested size by that before
> > looking for the image…)
> 
> I really think it will be pretty straightforward (though if we wanted icons to change when dragging
> between monitors that would be harder). I would hope that we could get the proper retina support done
> quickly. With the rough function I sketched out above it is mostly just replacing text in the tree.

I agree, this solution sounds very straightforward. Is it ok for me to include these replacements in this patch already or would you like to create a new issue for that?
Flags: needinfo?(dtownsend)
(Reporter)

Comment 30

2 years ago
(In reply to Johann Hofmann from comment #29)
> Ok, let me summarize the changes we agree on, so that I'm sure I got this:
> 
> 1. Keep the parsing and isnumeric checking of icon sizes
> 2. Persist all valid icons in the addon object
> 3. Don't put anything into addon.iconURL or addon.icon64URL
> 4. Implement the method that Mossop described in AddonManager
> 
> Something missing?

Looks ok, a couple of suggestions though:

Move the checks for icon.png and icon64.png from AddonWrapper.icons to loadManifestFromRDF so you build an icons array for old-style extensions too. I think we've agreed on putting icon.png into icons[32] and icons[48] for now since there is some variance there.

AddonWrapper.icons still needs to merge in the icons from repositoryAddon (though they shouldn't overwrite existing icons I think?) and addon.iconURL and addon.icon64URL should still take precedence in icons if the add-on is active

> > > (Hopefully the devicePixelRatio support can be as easy as checking
> > > Window.devicePixelRatio, and multiplying the requested size by that before
> > > looking for the image…)
> > 
> > I really think it will be pretty straightforward (though if we wanted icons to change when dragging
> > between monitors that would be harder). I would hope that we could get the proper retina support done
> > quickly. With the rough function I sketched out above it is mostly just replacing text in the tree.
> 
> I agree, this solution sounds very straightforward. Is it ok for me to
> include these replacements in this patch already or would you like to create
> a new issue for that?

That's fine.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 31

2 years ago
Created attachment 8673530 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8671455 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
I attached a WIP and would love to get some feedback. Some points I'd like to highlight:

1. Because we have to parse and compare icon sizes in AddonManager.getPreferredIconURL, I moved the sanity checks into this method and out of XPIProvider. For me the whole iteration part of getPreferredIconURL doesn't look great, but I'm not sure there is an alternative if we don't want to change the structure of addon.icons, which would increase code complexity.

2.: 
(In reply to Dave Townsend [:mossop] from comment #30) 
> Move the checks for icon.png and icon64.png from AddonWrapper.icons to
> loadManifestFromRDF so you build an icons array for old-style extensions
> too. I think we've agreed on putting icon.png into icons[32] and icons[48]
> for now since there is some variance there.

I found this to be hard to do, since WebExtension icon urls are always relative (which is a really good thing) and we can parse them with this.getResourceURL. But other addons might include absolute paths in their iconURL and icon64URL, which means we can't simply mix them with the relative urls of addon.icons (correct?).

The whole absolute/relative/icon.png situation seems pretty messed up, as I already found out while working on JPM (https://github.com/mozilla-jetpack/jpm/issues/197)

The minimal change to AddonWrapper.icons I did here is least likely to cause regressions, IMO. But I would be very open for ideas, since your suggestion sounds like the cleaner way to do it.

Thanks :)

Oh, I'll also add you to ui-review again, Blake, since it's a completely new patch.
(Assignee)

Updated

2 years ago
Attachment #8673530 - Flags: ui-review?(bwinton)
Attachment #8673530 - Flags: feedback?(dtownsend)
Comment on attachment 8673530 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8673530 [details] [diff] [review]:
-----------------------------------------------------------------

Given the comment below, do you still want me to ui-review this version?

::: toolkit/mozapps/extensions/content/about.js
@@ +15,5 @@
>  
>    document.documentElement.setAttribute("addontype", addon.type);
>  
> +  var iconURL = AddonManager.getPreferredIconURL(addon, 32, window);
> +  if (iconURL) {

So, I kind of expected this call (and the other calls like it) to live in this.__defineGetter__("iconURL" in XPIProvider.jsm, instead of replicating it everywhere.  Also, it should look for the 48px size, not the 32px size, as per previous comments.
(Assignee)

Comment 34

2 years ago
(In reply to Blake Winton (:bwinton) (:☕️) from comment #33)
> Comment on attachment 8673530 [details] [diff] [review]
> Support custom icons in Web Extensions
> 
> Review of attachment 8673530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given the comment below, do you still want me to ui-review this version?

You can hold off until we clarify that :)

> ::: toolkit/mozapps/extensions/content/about.js
> @@ +15,5 @@
> >  
> >    document.documentElement.setAttribute("addontype", addon.type);
> >  
> > +  var iconURL = AddonManager.getPreferredIconURL(addon, 32, window);
> > +  if (iconURL) {
> 
> So, I kind of expected this call (and the other calls like it) to live in
> this.__defineGetter__("iconURL" in XPIProvider.jsm, instead of replicating
> it everywhere.  Also, it should look for the 48px size, not the 32px size,
> as per previous comments.

Yup, let's give that one a 48.

Actually I was thinking of removing the iconURL getters completely. iconURL and icon64URL just don't fit into the idea of flexible icon sizes and calling AddonManager.getPreferredIconURL to get the iconURL seems like too many layers of (unnecessary) abstraction to me. Maybe :mossop can give his feedback on that.

Updated

2 years ago
Blocks: 1214433
Priority: -- → P1
(Reporter)

Comment 35

2 years ago
Comment on attachment 8673530 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8673530 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking really good. We'll need to add some tests for this, other than that there are only a few minor things here.

(In reply to Johann Hofmann from comment #32)
> (In reply to Dave Townsend [:mossop] from comment #30) 
> > Move the checks for icon.png and icon64.png from AddonWrapper.icons to
> > loadManifestFromRDF so you build an icons array for old-style extensions
> > too. I think we've agreed on putting icon.png into icons[32] and icons[48]
> > for now since there is some variance there.
> 
> I found this to be hard to do, since WebExtension icon urls are always
> relative (which is a really good thing) and we can parse them with
> this.getResourceURL. But other addons might include absolute paths in their
> iconURL and icon64URL, which means we can't simply mix them with the
> relative urls of addon.icons (correct?).

I think you're mixing up the iconURL and icon64URL properties that get pulled from install.rdf and the icon.png and icon64.png files that we look for in the root of the add-on. The former is an absolute URL, the latter is relative though and we can store it in the icons property when parsing the RDF add-ons rather than needing to do it everytime the icons property is requested (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6645)

> The whole absolute/relative/icon.png situation seems pretty messed up, as I
> already found out while working on JPM

Yeah iconURL in the install.rdf is a historical thing and in theory allowed for extensions to switch icon based on the theme in use. In practice it's just an annoyance we have to live with now.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +3072,5 @@
> +   * @param  aWindow
> +   *         Optional window object for determining the correct scale.
> +   * @return {String} The absolute URL of the icon
> +   */
> +  getPreferredIconURL: function getPreferredIconURL(aAddon, aSize, aWindow) {

To make it more obvious that aWindow is optional give it a default of undefined.

@@ +3077,5 @@
> +    if (aWindow && aWindow.devicePixelRatio) {
> +      aSize = aSize * aWindow.devicePixelRatio;
> +    }
> +
> +    let icons = aAddon.icons;

aAddon.icons may not exist for some kinds of add-ons in which case you want to create a fake one from aAddon.iconURL and aAddon.icon64URL.

@@ +3080,5 @@
> +
> +    let icons = aAddon.icons;
> +
> +    // quick return if the exact size was found
> +    if(icons[aSize]){

Nit, space after if and the condition.

@@ +3084,5 @@
> +    if(icons[aSize]){
> +      return icons[aSize];
> +    }
> +
> +    let icon;

Declare this to be null by default.

@@ +3093,5 @@
> +    }).filter(function(size) {
> +      return !isNaN(size);
> +    }).sort(function(a, b) {
> +      return b - a;
> +    }).forEach(function(size) {

You should assume that the keys are all integers at this point. Use for ... of instead of forEach and an arrow function for the comparison function should make this much shorter.

@@ +3099,5 @@
> +      // is larger or equal to the required size,
> +      // or any icon if the first condition can't be met
> +      if (!icon || size >= aSize) {
> +        icon = icons[size];
> +      }

I think this ends up looking simpler if you instead iterate from smallest to largest, always assign to icon and return early if size >= aSize. That should give you the icon closest in size to the one requested preferring larger if available.

::: toolkit/mozapps/extensions/content/about.js
@@ +15,5 @@
>  
>    document.documentElement.setAttribute("addontype", addon.type);
>  
> +  var iconURL = AddonManager.getPreferredIconURL(addon, 32, window);
> +  if (iconURL) {

This is what I expected to see. When we use icons we should switch to doing this to get the size we want for the UI. Eventually the iconURL and icon64URL properties could be dropped but for now we'll keep them around in case add-ons rely on them.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +881,4 @@
>    addon.iconURL = null;
>    addon.icon64URL = null;
> +
> +  // XXX: I completely removed the validation here, should I add it again?

Yes, we should validate at parse time then assume the database contains valid data everywhere else.

@@ +6631,5 @@
>      return null;
>    }, this);
>  
>    this.__defineGetter__("iconURL", function AddonWrapper_iconURLGetter() {
> +    return this.icons[32] || this.icons[48] || undefined;

I would switch this to use getPreferredIconURL for 48px.
Attachment #8673530 - Flags: feedback?(dtownsend) → feedback+
(Assignee)

Comment 36

2 years ago
(In reply to Dave Townsend [:mossop] from comment #35)
> Comment on attachment 8673530 [details] [diff] [review]
> Support custom icons in Web Extensions
> 
> Review of attachment 8673530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking really good. We'll need to add some tests for this, other
> than that there are only a few minor things here.

Yup, I wanted to get a round of feedback before testing. I'll add them now. :)

> I think you're mixing up the iconURL and icon64URL properties that get
> pulled from install.rdf and the icon.png and icon64.png files that we look
> for in the root of the add-on. The former is an absolute URL, the latter is
> relative though and we can store it in the icons property when parsing the
> RDF add-ons rather than needing to do it everytime the icons property is
> requested
> (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#6645)
> 

Oh right, I did mix them up. Putting icon.png and icon64.png in there makes absolute sense.
(Assignee)

Comment 37

2 years ago
> @@ +3077,5 @@
> > +    if (aWindow && aWindow.devicePixelRatio) {
> > +      aSize = aSize * aWindow.devicePixelRatio;
> > +    }
> > +
> > +    let icons = aAddon.icons;
> 
> aAddon.icons may not exist for some kinds of add-ons in which case you want
> to create a fake one from aAddon.iconURL and aAddon.icon64URL.
> 

Since aAddon.icons should call the icons getter in AddonWrapper I decided to populate it with iconURL and icon64URL in the AddonWrapper. Could this function get an add-on that is not wrapped by AddonWrapper?
(Reporter)

Comment 38

2 years ago
(In reply to Johann Hofmann from comment #37)
> > @@ +3077,5 @@
> > > +    if (aWindow && aWindow.devicePixelRatio) {
> > > +      aSize = aSize * aWindow.devicePixelRatio;
> > > +    }
> > > +
> > > +    let icons = aAddon.icons;
> > 
> > aAddon.icons may not exist for some kinds of add-ons in which case you want
> > to create a fake one from aAddon.iconURL and aAddon.icon64URL.
> > 
> 
> Since aAddon.icons should call the icons getter in AddonWrapper I decided to
> populate it with iconURL and icon64URL in the AddonWrapper. Could this
> function get an add-on that is not wrapped by AddonWrapper?

Yes, plugins and lightweight themes expose a different add-on object for example.
(Assignee)

Comment 39

2 years ago
(In reply to Dave Townsend [:mossop] from comment #38)
> (In reply to Johann Hofmann from comment #37)
> > > @@ +3077,5 @@
> > > > +    if (aWindow && aWindow.devicePixelRatio) {
> > > > +      aSize = aSize * aWindow.devicePixelRatio;
> > > > +    }
> > > > +
> > > > +    let icons = aAddon.icons;
> > > 
> > > aAddon.icons may not exist for some kinds of add-ons in which case you want
> > > to create a fake one from aAddon.iconURL and aAddon.icon64URL.
> > > 
> > 
> > Since aAddon.icons should call the icons getter in AddonWrapper I decided to
> > populate it with iconURL and icon64URL in the AddonWrapper. Could this
> > function get an add-on that is not wrapped by AddonWrapper?
> 
> Yes, plugins and lightweight themes expose a different add-on object for
> example.

Ok, good to know. Then I'll get iconURL out of the AddonWrapper.

One thing that worries me is creating an infinite recursion when getPreferredIconURL calls AddonWrapper.iconURL(a getter) and that getter calls getPreferredIconURL again.
(Reporter)

Comment 40

2 years ago
(In reply to Johann Hofmann from comment #39)
> (In reply to Dave Townsend [:mossop] from comment #38)
> > (In reply to Johann Hofmann from comment #37)
> > > > @@ +3077,5 @@
> > > > > +    if (aWindow && aWindow.devicePixelRatio) {
> > > > > +      aSize = aSize * aWindow.devicePixelRatio;
> > > > > +    }
> > > > > +
> > > > > +    let icons = aAddon.icons;
> > > > 
> > > > aAddon.icons may not exist for some kinds of add-ons in which case you want
> > > > to create a fake one from aAddon.iconURL and aAddon.icon64URL.
> > > > 
> > > 
> > > Since aAddon.icons should call the icons getter in AddonWrapper I decided to
> > > populate it with iconURL and icon64URL in the AddonWrapper. Could this
> > > function get an add-on that is not wrapped by AddonWrapper?
> > 
> > Yes, plugins and lightweight themes expose a different add-on object for
> > example.
> 
> Ok, good to know. Then I'll get iconURL out of the AddonWrapper.

No I think it's right to leave it there because it is the install.rdf property and differs from AddonWrapper.iconURL which is generated from the final icons array.

> One thing that worries me is creating an infinite recursion when
> getPreferredIconURL calls AddonWrapper.iconURL(a getter) and that getter
> calls getPreferredIconURL again.

That would only happen if the addon object didn't have an icons property and since AddonWrapper does we should be safe.
(Assignee)

Comment 41

2 years ago
(In reply to Dave Townsend [:mossop] from comment #40)
> (In reply to Johann Hofmann from comment #39)
> > (In reply to Dave Townsend [:mossop] from comment #38)
> > > (In reply to Johann Hofmann from comment #37)
> > > > > @@ +3077,5 @@
> > > > > > +    if (aWindow && aWindow.devicePixelRatio) {
> > > > > > +      aSize = aSize * aWindow.devicePixelRatio;
> > > > > > +    }
> > > > > > +
> > > > > > +    let icons = aAddon.icons;
> > > > > 
> > > > > aAddon.icons may not exist for some kinds of add-ons in which case you want
> > > > > to create a fake one from aAddon.iconURL and aAddon.icon64URL.
> > > > > 
> > > > 
> > > > Since aAddon.icons should call the icons getter in AddonWrapper I decided to
> > > > populate it with iconURL and icon64URL in the AddonWrapper. Could this
> > > > function get an add-on that is not wrapped by AddonWrapper?
> > > 
> > > Yes, plugins and lightweight themes expose a different add-on object for
> > > example.
> > 
> > Ok, good to know. Then I'll get iconURL out of the AddonWrapper.
> 
> No I think it's right to leave it there because it is the install.rdf
> property and differs from AddonWrapper.iconURL which is generated from the
> final icons array.
> 
> > One thing that worries me is creating an infinite recursion when
> > getPreferredIconURL calls AddonWrapper.iconURL(a getter) and that getter
> > calls getPreferredIconURL again.
> 
> That would only happen if the addon object didn't have an icons property and
> since AddonWrapper does we should be safe.

Right on both accounts! Sorry, a lot of similarly named things that were confusing me, but I think it's clear now. :D
(Assignee)

Comment 42

2 years ago
Created attachment 8675398 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8673530 - Attachment is obsolete: true
Attachment #8673530 - Flags: ui-review?(bwinton)
(Assignee)

Comment 43

2 years ago
Comment on attachment 8675398 [details] [diff] [review]
Support custom icons in Web Extensions

This is getting quite sophisticated, I hope I didn't forget anything from the feedback. :D

If this works as I expect it to it should make sure that WebExtensions get the ideal icons while old-style addon icons still look the same.

The tests I added only cover new (WebExtension) behavior, I've had several existing tests fail on me during development, so I assume regressions should be covered well enough.

Thanks!
Attachment #8675398 - Flags: ui-review?(bwinton)
Attachment #8675398 - Flags: review?(dtownsend)
Comment on attachment 8675398 [details] [diff] [review]
Support custom icons in Web Extensions

Looks good to me.  Thanks!
Attachment #8675398 - Flags: ui-review?(bwinton) → ui-review+
(Reporter)

Comment 45

2 years ago
Comment on attachment 8675398 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8675398 [details] [diff] [review]:
-----------------------------------------------------------------

You will find that unless you make a change to xpcshell.ini this patch will fail tests when landing for real because of bug 994255. Just add a new blank line to it or something.

This is looking really good ... but I just realised that by moving the checks for icon.png and icon64.png to the manifest parsing code this will break existing users of those add-ons (they won't have an addons property defined). There isn't a particularly nice way to handle that migration either so in order to get this landed I'd suggest that in the case where AddonWrapper.icons can't find an icons property to also create one there checking for the icon.png and icon64.png property. This means doing the check in two places now but at least will start to fill out the DB with correct data as users upgrade and install add-ons. I've filed bug 1216243 to attempt to make this easier in the future at which point we can remove the additional checks.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +3072,5 @@
> +   * @param  aWindow
> +   *         Optional window object for determining the correct scale.
> +   * @return {String} The absolute URL of the icon
> +   */
> +  getPreferredIconURL: function getPreferredIconURL(aAddon, aSize, aWindow=undefined) {

Nit: use a space around operators like =.

@@ +3074,5 @@
> +   * @return {String} The absolute URL of the icon
> +   */
> +  getPreferredIconURL: function getPreferredIconURL(aAddon, aSize, aWindow=undefined) {
> +    if (aWindow && aWindow.devicePixelRatio) {
> +      aSize = aSize * aWindow.devicePixelRatio;

Nit: Use *=

@@ +3098,5 @@
> +    }
> +
> +    // Parse icon set and sort in ascending order
> +    let sizes = Object.keys(icons)
> +    .map((size) => parseInt(size, 10))

We should assume the keys are already sanitized at this point or throw an exception if they aren't.

@@ +3099,5 @@
> +
> +    // Parse icon set and sort in ascending order
> +    let sizes = Object.keys(icons)
> +    .map((size) => parseInt(size, 10))
> +    .sort((a, b) => a - b);

Nit: Line up the periods above.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +887,5 @@
> +    // filter out invalid (non-integer) size keys
> +    Object.keys(icons)
> +    .map((size) => parseInt(size, 10))
> +    .filter((size) => !isNaN(size))
> +    .forEach((size) => addon.icons[size] = icons[size]);

Nit: Line up the periods above.

@@ +6685,5 @@
> +    if (aAddon.icons) {
> +      for (let size in aAddon.icons) {
> +        icons[size] = this.getResourceURI(aAddon.icons[size]).spec;
> +      }
> +    }

If there is no icons property and this isn't a webextension then we haven't gone through the new parsing code so create one and check for the icon.png and icon64.png files.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
@@ +15,5 @@
> +
> +  let addon = yield promiseAddonByID(ID);
> +  addon.uninstall();
> +  yield promiseShutdownManager();
> +});

What is this test testing?
Attachment #8675398 - Flags: review?(dtownsend)
(Assignee)

Comment 46

2 years ago
(In reply to Dave Townsend [:mossop] from comment #45)
> Comment on attachment 8675398 [details] [diff] [review]
> Support custom icons in Web Extensions
> 
> Review of attachment 8675398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You will find that unless you make a change to xpcshell.ini this patch will
> fail tests when landing for real because of bug 994255. Just add a new blank
> line to it or something.
> 
> This is looking really good ... but I just realised that by moving the
> checks for icon.png and icon64.png to the manifest parsing code this will
> break existing users of those add-ons (they won't have an addons property
> defined). There isn't a particularly nice way to handle that migration
> either so in order to get this landed I'd suggest that in the case where
> AddonWrapper.icons can't find an icons property to also create one there
> checking for the icon.png and icon64.png property. This means doing the
> check in two places now but at least will start to fill out the DB with
> correct data as users upgrade and install add-ons. I've filed bug 1216243 to
> attempt to make this easier in the future at which point we can remove the
> additional checks.

Damn, very good catch! I'll add the shim to AW.icons

> 
> @@ +3098,5 @@
> > +    }
> > +
> > +    // Parse icon set and sort in ascending order
> > +    let sizes = Object.keys(icons)
> > +    .map((size) => parseInt(size, 10))
> 
> We should assume the keys are already sanitized at this point or throw an
> exception if they aren't.

I'm not sure I understand that :)

Do you mean that parseInt is unnecessary? We require the sizes to be numbers because we have to sort and compare them.

> ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
> @@ +15,5 @@
> > +
> > +  let addon = yield promiseAddonByID(ID);
> > +  addon.uninstall();
> > +  yield promiseShutdownManager();
> > +});
> 
> What is this test testing?

I couldn't get writeWebManifestForExtension to work without installing an extension first. I'll try to make it more concise. Or is there another way to programmatically overwrite a manifest?
Flags: needinfo?(dtownsend)
(Reporter)

Comment 47

2 years ago
(In reply to Johann Hofmann from comment #46)
> (In reply to Dave Townsend [:mossop] from comment #45)
> > @@ +3098,5 @@
> > > +    }
> > > +
> > > +    // Parse icon set and sort in ascending order
> > > +    let sizes = Object.keys(icons)
> > > +    .map((size) => parseInt(size, 10))
> > 
> > We should assume the keys are already sanitized at this point or throw an
> > exception if they aren't.
> 
> I'm not sure I understand that :)
> 
> Do you mean that parseInt is unnecessary? We require the sizes to be numbers
> because we have to sort and compare them.

I mean that this is an API so rather than sanitizing the input I think we should throw an exception if the passed object contains keys that aren't already numbers. Since we're talking about add-on objects here it would reflect a bug in a provider.

> > ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
> > @@ +15,5 @@
> > > +
> > > +  let addon = yield promiseAddonByID(ID);
> > > +  addon.uninstall();
> > > +  yield promiseShutdownManager();
> > > +});
> > 
> > What is this test testing?
> 
> I couldn't get writeWebManifestForExtension to work without installing an
> extension first. I'll try to make it more concise. Or is there another way
> to programmatically overwrite a manifest?

Looks like writeWebManifestForExtension will fail if the extensions directory doesn't yet exist in the profile. Doing the install then uninstall in this test makes the add-ons manager create it for you but that's unnecessary, just create the directory directly and you should be fine.
Flags: needinfo?(dtownsend)
(Reporter)

Comment 48

2 years ago
(In reply to Dave Townsend [:mossop] from comment #47)
> (In reply to Johann Hofmann from comment #46)
> > (In reply to Dave Townsend [:mossop] from comment #45)
> > > @@ +3098,5 @@
> > > > +    }
> > > > +
> > > > +    // Parse icon set and sort in ascending order
> > > > +    let sizes = Object.keys(icons)
> > > > +    .map((size) => parseInt(size, 10))
> > > 
> > > We should assume the keys are already sanitized at this point or throw an
> > > exception if they aren't.
> > 
> > I'm not sure I understand that :)
> > 
> > Do you mean that parseInt is unnecessary? We require the sizes to be numbers
> > because we have to sort and compare them.
> 
> I mean that this is an API so rather than sanitizing the input I think we
> should throw an exception if the passed object contains keys that aren't
> already numbers. Since we're talking about add-on objects here it would
> reflect a bug in a provider.

I think you can also get away without doing any sorting of the list too. I don't know if this is easier or harder to understand:

let bestSize = null;
for (let size of Object.keys(icons)) {
  if (!Number.isInteger(size))
    throw exception;

  if (!bestSize) {
    bestSize = size;
    continue;
  }

  if (size > aSize && bestSize > aSize) {
    // If both best size and current size are larger than the wanted size then choose
    // the one closest to the wanted size
    bestSize = Math.min(bestSize, size);
  }
  else {
    // Otherwise choose the largest of the two so we'll prefer sizes as close to below aSize
    // or above aSize
    bestSize = Math.max(bestSize, size);
  }
}

if (bestSize)
  return icons[bestSize];
(Assignee)

Comment 49

2 years ago
Unless we decide to use a Map, Object.keys will produce an array of strings.

That means we have to use parseInt, it's not about sanitizing but rather just getting the numeric value out.

If we combine the parseInt with Number.isInteger or isNaN (would be the same in that case) we'd be back at my original patch which you correctly rejected because it's duplicating logic that we already do when originally parsing the values.

I think in this case it would be best for performance and completely sane to trust the XPIProvider, also considering that it's actually impossible for a NaN size to cause damage, since it would be selected last and then icons[NaN] would just return undefined, the correct value if there are no other values in the icons object.

> I think you can also get away without doing any sorting of the list too

I think it looks cleaner when sorted and iteration count should really not matter with no more than a dozen icon sizes. But if you prefer the loop variant, how about this (just added the parseInt):

let bestSize = null;
for (let size of Object.keys(icons)) {
  size = parseInt(size, 10);

  if (!bestSize) {
    bestSize = size;
    continue;
  }

  if (size > aSize && bestSize > aSize) {
    // If both best size and current size are larger than the wanted size then choose
    // the one closest to the wanted size
    bestSize = Math.min(bestSize, size);
  }
  else {
    // Otherwise choose the largest of the two so we'll prefer sizes as close to below aSize
    // or above aSize
    bestSize = Math.max(bestSize, size);
  }
}

if (bestSize)
  return icons[bestSize];
(Reporter)

Comment 50

2 years ago
(In reply to Johann Hofmann from comment #49)
> Unless we decide to use a Map, Object.keys will produce an array of strings.

Oh huh, that's annoying.

> That means we have to use parseInt, it's not about sanitizing but rather
> just getting the numeric value out.

Yeah ok then.

> I think in this case it would be best for performance and completely sane to
> trust the XPIProvider, also considering that it's actually impossible for a
> NaN size to cause damage, since it would be selected last and then
> icons[NaN] would just return undefined, the correct value if there are no
> other values in the icons object.
> 
> > I think you can also get away without doing any sorting of the list too
> 
> I think it looks cleaner when sorted and iteration count should really not
> matter with no more than a dozen icon sizes. But if you prefer the loop
> variant, how about this (just added the parseInt):

Choose whichever you feel reads best.
(Assignee)

Comment 51

2 years ago
Created attachment 8677144 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8675398 - Attachment is obsolete: true
(Assignee)

Comment 52

2 years ago
Comment on attachment 8677144 [details] [diff] [review]
Support custom icons in Web Extensions

So after thinking about it both versions read fine to me. I did a small very unscientific benchmark (http://jsperf.com/webextension-icons) and your version is much faster on my machine, so I chose that one. :)

One more thing, the AddonManager seems to use the pattern of outsourcing complex functions into AddonManagerInternal so I did that too with getPreferredIconURL. I hope that's good.

Cheers
Attachment #8677144 - Flags: review?(dtownsend)
(Reporter)

Comment 53

2 years ago
(In reply to Johann Hofmann from comment #52)
> Comment on attachment 8677144 [details] [diff] [review]
> Support custom icons in Web Extensions
> 
> So after thinking about it both versions read fine to me. I did a small very
> unscientific benchmark (http://jsperf.com/webextension-icons) and your
> version is much faster on my machine, so I chose that one. :)

heh this doesn't surprise me, my version has a single loop while in yours the map is a loop, the sort is at least one loop and then there is the final loop. Of course with the sizes we're dealing with the difference is going to be tiny in reality but if it looks as readable then it's still a win.
(Assignee)

Comment 54

2 years ago
(In reply to Dave Townsend [:mossop] from comment #53)
> (In reply to Johann Hofmann from comment #52)
> > Comment on attachment 8677144 [details] [diff] [review]
> > Support custom icons in Web Extensions
> > 
> > So after thinking about it both versions read fine to me. I did a small very
> > unscientific benchmark (http://jsperf.com/webextension-icons) and your
> > version is much faster on my machine, so I chose that one. :)
> 
> heh this doesn't surprise me, my version has a single loop while in yours
> the map is a loop, the sort is at least one loop and then there is the final
> loop. Of course with the sizes we're dealing with the difference is going to
> be tiny in reality but if it looks as readable then it's still a win.

Yup, it could also be the inefficiency of .map calling a function every time or the added complexity from sorting. Would be interesting to find out.

It's ultra premature optimization but we'll probably use this function in all places where icons need to be displayed, so why not :D
(Reporter)

Comment 55

2 years ago
Comment on attachment 8677144 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8677144 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, there are just two additional checks I'd like to see then this can land.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +2346,5 @@
> +
> +    let bestSize = null;
> +
> +    for (let size of Object.keys(icons)) {
> +      size = parseInt(size, 10);

Throw an exception if this returns NaN, it's broken behaviour.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
@@ +54,5 @@
> +  equal(AddonManager.getPreferredIconURL(addon, 30), uri + "icon32.png");
> +  equal(AddonManager.getPreferredIconURL(addon, 48), uri + "icon48.png");
> +  equal(AddonManager.getPreferredIconURL(addon, 64), uri + "icon64.png");
> +  equal(AddonManager.getPreferredIconURL(addon, 128), uri + "icon64.png");
> +

Do another restart here and then repeat the tests. That will verify that the icons were correctly stored in the disk json file and reloaded properly. Probably easiest to turn the checks into a function you can call twice.
Attachment #8677144 - Flags: review?(dtownsend)
(Assignee)

Comment 56

2 years ago
(In reply to Dave Townsend [:mossop] from comment #55)
> Comment on attachment 8677144 [details] [diff] [review]
> Support custom icons in Web Extensions
> 
> Review of attachment 8677144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, there are just two additional checks I'd like to see then
> this can land.
> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +2346,5 @@
> > +
> > +    let bestSize = null;
> > +
> > +    for (let size of Object.keys(icons)) {
> > +      size = parseInt(size, 10);
> 
> Throw an exception if this returns NaN, it's broken behaviour.

Does this mean consumers of this method (e.g. our addon manager frontend) have to handle this error or should they just ignore it? Handling the error is weird because it's completely out of their domain and there is nothing they can do in case this happens, but not handling a possibly thrown error also sounds incorrect. The problem is that the frontend is not responsible for the contents of the icons object but we'd force it to somehow handle the case when the icons object (or just a single key) is invalid.

Also, what would be the correct error code for that? NS_ERROR_INVALID_ARG? NS_ERROR_ILLEGAL_VALUE?
Flags: needinfo?(dtownsend)
(Assignee)

Comment 57

2 years ago
Created attachment 8678188 [details] [diff] [review]
Support custom icons in Web Extensions
(Assignee)

Updated

2 years ago
Attachment #8677144 - Attachment is obsolete: true
(Assignee)

Comment 58

2 years ago
Comment on attachment 8678188 [details] [diff] [review]
Support custom icons in Web Extensions

Anyway, here's the patch with the suggestions. I'll obviously trust your judgement if you think that's best :)
(Reporter)

Comment 59

2 years ago
Comment on attachment 8678188 [details] [diff] [review]
Support custom icons in Web Extensions

Review of attachment 8678188 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!
Attachment #8678188 - Flags: review+
(Reporter)

Comment 60

2 years ago
(In reply to Johann Hofmann from comment #56)
> (In reply to Dave Townsend [:mossop] from comment #55)
> Also, what would be the correct error code for that? NS_ERROR_INVALID_ARG?
> NS_ERROR_ILLEGAL_VALUE?

Little known fact, NS_ERROR_INVALID_ARG == NS_ERROR_ILLEGAL_VALUE
Flags: needinfo?(dtownsend)

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 61

2 years ago
Great! Thanks a lot for guiding me through this.

> Little known fact, NS_ERROR_INVALID_ARG == NS_ERROR_ILLEGAL_VALUE

Damn :D
(Assignee)

Comment 62

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=379d5fa4a87a
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 63

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4333e54b1ce7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4333e54b1ce7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

4 months ago
Depends on: 1331185
You need to log in before you can comment on or make changes to this bug.