the pref name is extensions.webextensions.themes.icons.enabled. changing it to true will allow icons to be specified by webextension themes. the bug that introduced the theming:icon code and pref is bug 1343921 right now bug 1343921 will accept any image format, but we should restrict it to SVG before preffing on (since it's new for lwt) - unless it is decided that SVG icons are not being used in firefox toolbar. The bugs this depends upon are the ones needed to make sure SVG icons are performant enough. not using SVG leaves scaling issues between themes - shorlander is the UX person on this and explains best in https://bugzilla.mozilla.org/show_bug.cgi?id=1054016#c40

(In reply to :shell escalante from comment #0) > right now bug 1343921 will accept any image format, but we should restrict > it to SVG before preffing on (since it's new for lwt) - unless it is decided > that SVG icons are not being used in firefox toolbar. Even if the default icons use SVG, I'm not sure requiring SVG for icons provided via the theming API is a good idea. SVG can be quite close to raster formats in terms of performance _IF_ the SVG doesn't contain expensive effects. On the other hand, if the SVG needs to contain CPU intensive effects such as gradients or filters then raster image equivalents can perform much better (since all the expensive work of computing the gradients, filters, etc. is done while the raster images are being created instead of when they're loaded).

removing depends on bug 1347543 and bug 1054016. Follow bug 1347543 for results of patch and perf. The SVG toolbar icons perf tests (uses different SVGs) looks good enough to try experiment with our icon SVG pref to true based on https://bugzilla.mozilla.org/show_bug.cgi?id=1347543#c16 ... there is a test SVG icon theme included with jareds' patch in bug 1343921

(In reply to Jonathan Watt [:jwatt] from comment #1) > (In reply to :shell escalante from comment #0) > > right now bug 1343921 will accept any image format, but we should restrict > > it to SVG before preffing on (since it's new for lwt) - unless it is decided > > that SVG icons are not being used in firefox toolbar. > > Even if the default icons use SVG, I'm not sure requiring SVG for icons > provided via the theming API is a good idea. SVG can be quite close to > raster formats in terms of performance _IF_ the SVG doesn't contain > expensive effects. On the other hand, if the SVG needs to contain CPU > intensive effects such as gradients or filters then raster image equivalents > can perform much better (since all the expensive work of computing the > gradients, filters, etc. is done while the raster images are being created > instead of when they're loaded). Yes, while this is true we also get many benefits from using scalable graphics in that we will not be locked in to one size for the future of our UI. We could implement some rules that block SVGs that use expensive functions if that is our only worry.

Comment on attachment 8862158 [details] Bug 1348039 - Enable the support for icons in webextension themes. https://reviewboard.mozilla.org/r/134134/#review137036 Thanks, Jared!

(In reply to Mike de Boer [:mikedeboer] from comment #5) > Comment on attachment 8862158 [details] > Bug 1348039 - Enable the support for icons in webextension themes. > > https://reviewboard.mozilla.org/r/134134/#review137036 > > Thanks, Jared! What does this do?

(In reply to Stephen Horlander [:shorlander] from comment #6) > (In reply to Mike de Boer [:mikedeboer] from comment #5) > > Comment on attachment 8862158 [details] > > Bug 1348039 - Enable the support for icons in webextension themes. > > > > https://reviewboard.mozilla.org/r/134134/#review137036 > > > > Thanks, Jared! > > What does this do? This allows themes written through the new theming API to replace icons with their own version. Themes would have to use the following in their manifest: > { > "manifest_version": 2, > "name": "webext-lwt-icons", > "version": "1.0", > > "description": "Loads a lwt and icons using the webext format", > "icons": { > "96": "face.png" > }, > "theme": { > "colors": { > "frame": [71, 105, 91], > "tab_text": [207, 221, 192, 0.9] > }, > "images": { > "theme_frame": "face.png" > }, > "icons": { > "back": "fox.svg", > "forward": "fox.svg", > "reload": "fox.svg", > "stop": "fox.svg", > } > } > } Creating a theme like this, with a 'fox.svg' file, would replace the back, forward, reload, and stop icons with the fox SVG icon.

Sorry, I mean does this specifically turn that on with the intent to ship? I discussed with Kev that we had never talked about this functionality and no one from UX had input here that I am aware of.

(In reply to Stephen Horlander [:shorlander] from comment #8) > Sorry, I mean does this specifically turn that on with the intent to ship? I > discussed with Kev that we had never talked about this functionality and no > one from UX had input here that I am aware of. Oh, I was out of the loop on that. I thought we were just trying to tie this together with switching our icons from PNG to SVG. I think we should do this, it is a nice way for theme devs to customize the browser and create a rich/full theme with little work and low risk of side-effects. What are the issues that you or Kev see with this?

(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #9) > (In reply to Stephen Horlander [:shorlander] from comment #8) > > Sorry, I mean does this specifically turn that on with the intent to ship? I > > discussed with Kev that we had never talked about this functionality and no > > one from UX had input here that I am aware of. > > Oh, I was out of the loop on that. I thought we were just trying to tie this > together with switching our icons from PNG to SVG. I think we should do > this, it is a nice way for theme devs to customize the browser and create a > rich/full theme with little work and low risk of side-effects. What are the > issues that you or Kev see with this? Sorry this bug was filed to track the work to do, but not to land pre-57. As this isn't a blocker area for 57, and the risk/time isn't something that is planned pre-57. The issues Kev brought up were around defining and constraining the SVGs accepted to avoid any security risk of including more than intended with the SVG file and keep the perf on par with what was tested with toolbar icons. In addition to getting further UX input.

(In reply to :shell escalante from comment #10) > Sorry this bug was filed to track the work to do, but not to land pre-57. > As this isn't a blocker area for 57, and the risk/time isn't something that > is planned pre-57. The issues Kev brought up were around defining and > constraining the SVGs accepted to avoid any security risk of including more > than intended with the SVG file and keep the perf on par with what was > tested with toolbar icons. In addition to getting further UX input. Yes. We need to have a bigger discussion around the constraints and also the design risks this introduces. Can we please make sure this does not get enabled until we have those discussions? Thanks!

Hi JAWS, Can you not land that patch to enable? I'll check with Shorlander/Kev on timing, I imagine to avoid introducing any risk - this will be post 57.

Yeah, I'm holding off from landing and watching the conversation here. Thanks for double-checking.

Bringing back up, adding shorlander on needinfo and scheduling a conversation so we get resolution.

Clearing my needinfo, pinging shorlander for comment by email, because we do need to rekindle this for 58/59 if this is going to be a thing.