Closed Bug 1343921 Opened 3 years ago Closed 3 years ago

Implement support for custom icons through the Theming API

Categories

(WebExtensions :: Frontend, enhancement)

enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

(Whiteboard: triaged)

User Story

Replacing the current icon set with custom glyphs is not possible in any other browser currently available through a standardized API, thus a unique feature once the following user stories have been implemented:

- As a theme author I’d like to use SVG glyphs as icons for buttons present anywhere in the browser interface. Bitmap graphics have serious downsides which make it hard for both browser engineers and theme authors to make them look good across operating systems, and screen DPI settings. To prevent a low quality look and feel due to lossy up- and/ or downscaling of bitmap images, only vector images in the SVG format will be supported.

- As a theme author I’d like my theme with custom icons to be future-compatible if the browser ever decides to change icon sizes or introduce new ways to look at buttons (such as when the Firefox menu panel got introduced). Using SVG glyphs provides enough functionality to fulfill this story.

Note that the successful implementation of this feature is at risk, because the performance of using SVG icons is not guaranteed to be optimal. There is work underway to optimize this throughout the browser, eventually switching all icons in the default browser theme to using SVG glyphs, but it is unknown at this time if that’s finished before we start work here.

Attachments

(2 files, 1 obsolete file)

No description provided.
Still need to get the test finished to cover only providing some of the icons.
Attached file webext-theme-icon.zip
These are the two webextensions I've been testing with. One defines only one icon and the other defines all.
Comment on attachment 8842979 [details]
Bug 1343921 - Implement support for custom icons through the Theming API.

https://reviewboard.mozilla.org/r/116708/#review118482

This looks great!

::: browser/components/extensions/ext-theme.js:151
(Diff revision 2)
> +    for (let icon of Object.keys(icons)) {
> +      let val = icons[icon];
> +      if (!val) {
> +        continue;
> +      }
> +      if (Object.keys(ICONS_TO_VARIABLE_MAP).includes(icon)) {

Would it be better to store the call to Object.keys(ICONS_TO_VARIABLE_MAP) outside of the loop?

::: browser/components/extensions/ext-theme.js:153
(Diff revision 2)
> +      if (!val) {
> +        continue;
> +      }
> +      if (Object.keys(ICONS_TO_VARIABLE_MAP).includes(icon)) {
> +        if (!this.lwtStyles.icons) {
> +          this.lwtStyles.icons = {};

Could we just move this to the constructor?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:5
(Diff revision 2)
> +"use strict";
> +
> +const ENCODED_IMAGE_DATA = "PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCA2NCA2NCIgZW5hYmxlLWJhY2tncm91bmQ9Im5ldyAwIDAgNjQgNjQiPjxwYXRoIGQ9Im01NS45IDMyLjFsLTIyLjctMTQuOWMwIDAgMTIuOS0xNy40IDE5LjQtMTQuOSAzLjEgMS4xIDUuNCAyNS4xIDMuMyAyOS44IiBmaWxsPSIjM2U0MzQ3Ii8+PHBhdGggZD0ibTU0LjkgMzMuOWwtOS00LjFjMCAwLTUuMy0xNCA2LjEtMjQuMSAyLjQgMiA1LjEgMjUgMi45IDI4LjIiIGZpbGw9IiNmZmYiLz48cGF0aCBkPSJtOC4xIDMyLjFsMjIuNi0xNC45YzAgMC0xMi45LTE3LjQtMTkuNC0xNC45LTMgMS4xLTUuMyAyNS4xLTMuMiAyOS44IiBmaWxsPSIjM2U0MzQ3Ii8+PHBhdGggZD0ibTkuMSAzMy45bDktNC4xYzAgMCA1LjMtMTQtNi4xLTI0LjEtMi40IDItNS4xIDI1LTIuOSAyOC4yIiBmaWxsPSIjZmZmIi8+PHBhdGggZD0iTTMyLDEzQzE4LjksMTMsMiwzMy42LDIsNDUuNEMyMC41LDQ1LjQsMTkuNyw2MiwzMiw2MnMxMS41LTE2LjYsMzAtMTYuNkM2MiwzMy42LDQ1LjEsMTMsMzIsMTN6IiBmaWxsPSIjZmY4NzM2Ii8+PGcgZmlsbD0iI2ZmZiI+PHBhdGggZD0iTTMyLDU2LjJjMCw1LjEsOS42LDQuMiw5LjUtMi45YzYuNy05LjQsMTkuOS04LjcsMTkuOS04LjdDMzkuNiwzMi40LDMyLDU2LjIsMzIsNTYuMnoiLz48cGF0aCBkPSJNMzIsNTYuMmMwLDUuMS05LjYsNC4yLTkuNS0yLjlDMTUuOCw0NCwyLjYsNDQuNywyLjYsNDQuN0MyNC40LDMyLjQsMzIsNTYuMiwzMiw1Ni4yeiIvPjwvZz48ZyBmaWxsPSIjZmY4NzM2Ij48cGF0aCBkPSJtNTMuNCAxOC41Yy00IC43LTQuOSA2LjMtNC45IDYuM2w2IDUuM2MtMi4zLTUuOS0xLjEtMTEuNi0xLjEtMTEuNiIvPjxwYXRoIGQ9Im01MS4xIDEzLjVjLTQuNCAzLjktNS4xIDguNy01LjEgOC43bDYgNS4zYy0yLjQtNS44LS45LTE0LS45LTE0Ii8+PHBhdGggZD0ibTEwLjYgMTguNWM0IC43IDQuOSA2LjMgNC45IDYuM2wtNiA1LjNjMi4zLTUuOSAxLjEtMTEuNiAxLjEtMTEuNiIvPjxwYXRoIGQ9Im0xMi45IDEzLjVjNC40IDMuOSA1LjEgOC43IDUuMSA4LjdsLTYgNS4zYzIuNC01LjguOS0xNCAuOS0xNCIvPjwvZz48cGF0aCBkPSJtNTIuOCAzMS4xYy01LjctMS44LTEwLjktMy40LTEzLjguOS0yLjQgMy43LjcgOS40LjcgOS40IDExLjIgMS4yIDEzLjEtMTAuMyAxMy4xLTEwLjMiIGZpbGw9IiMzZTQzNDciLz48ZWxsaXBzZSBjeD0iNDMiIGN5PSIzNi4zIiByeD0iNC4yIiByeT0iNC4xIiBmaWxsPSIjZDVmZjgzIi8+PGcgZmlsbD0iIzNlNDM0NyI+PGVsbGlwc2UgY3g9IjQzIiBjeT0iMzYuMyIgcng9IjIuNyIgcnk9IjIuNyIvPjxwYXRoIGQ9Im0xMS4yIDMxLjFjNS43LTEuOCAxMC45LTMuNCAxMy43LjkgMi40IDMuNy0uNyA5LjQtLjcgOS40LTExLjEgMS4yLTEzLTEwLjMtMTMtMTAuMyIvPjwvZz48ZWxsaXBzZSBjeD0iMjEiIGN5PSIzNi4zIiByeD0iNC4yIiByeT0iNC4xIiBmaWxsPSIjZDVmZjgzIi8+PGcgZmlsbD0iIzNlNDM0NyI+PGVsbGlwc2UgY3g9IjIxIiBjeT0iMzYuMyIgcng9IjIuNyIgcnk9IjIuNyIvPjxwYXRoIGQ9Im00MS4yIDQ3LjljLS43LTIuMy0xLjgtNC40LTMtNi41IDEuMSAyLjEgMiA0LjMgMi41IDYuNi41IDIuMy43IDQuNyAwIDYuOC0uNCAxLTEgMi0xLjggMi42LS44LjYtMS44IDEtMi43IDEtLjkgMC0xLjktLjMtMi41LTEtLjYtLjctLjktMS42LS44LTIuNmwtLjkuMmgtLjljMCAxLS4yIDEuOS0uOCAyLjYtLjYuNy0xLjUgMS0yLjUgMS0uOSAwLTEuOS0uNC0yLjctMS0uOC0uNi0xLjQtMS42LTEuOC0yLjYtLjgtMi4xLS42LTQuNiAwLTYuOC41LTIuMyAxLjUtNC41IDIuNS02LjYtMS4yIDItMi4zIDQuMS0zIDYuNS0uNyAyLjMtMS4xIDQuOC0uNCA3LjMuMyAxLjIgMSAyLjQgMS45IDMuMy45LjkgMi4xIDEuNCAzLjQgMS41IDEuMi4xIDIuNi0uMiAzLjctMS4yLjMtLjIuNS0uNS43LS44LjIuMy40LjYuNy44IDEgMSAyLjQgMS4zIDMuNyAxLjIgMS4zLS4xIDIuNC0uNyAzLjQtMS41LjktLjkgMS42LTIgMS45LTMuMy41LTIuNi4xLTUuMi0uNi03LjUiLz48cGF0aCBkPSJtMzcuNiA1MC4zYy0xLjEtMS4xLTQuNS0xLjItNS42LTEuMi0xIDAtNC41LjEtNS42IDEuMi0uOC44LS4yIDIuOCAxLjkgNC41IDEuMyAxLjEgMi42IDEuNCAzLjYgMS40IDEgMCAyLjMtLjMgMy42LTEuNCAyLjMtMS43IDIuOS0zLjcgMi4xLTQuNSIvPjwvZz48L3N2Zz4=";
> +
> +function shouldButtonHaveCustomStyling(selector, verifyFn, icon, message) {

The title of this function seems to suggest it should return true if the button should have custom styling and false otherwise. Could you maybe rename it to something like `verifyButtonProperties` and add jsdoc?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:8
(Diff revision 2)
> +const ENCODED_IMAGE_DATA = "PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCA2NCA2NCIgZW5hYmxlLWJhY2tncm91bmQ9Im5ldyAwIDAgNjQgNjQiPjxwYXRoIGQ9Im01NS45IDMyLjFsLTIyLjctMTQuOWMwIDAgMTIuOS0xNy40IDE5LjQtMTQuOSAzLjEgMS4xIDUuNCAyNS4xIDMuMyAyOS44IiBmaWxsPSIjM2U0MzQ3Ii8+PHBhdGggZD0ibTU0LjkgMzMuOWwtOS00LjFjMCAwLTUuMy0xNCA2LjEtMjQuMSAyLjQgMiA1LjEgMjUgMi45IDI4LjIiIGZpbGw9IiNmZmYiLz48cGF0aCBkPSJtOC4xIDMyLjFsMjIuNi0xNC45YzAgMC0xMi45LTE3LjQtMTkuNC0xNC45LTMgMS4xLTUuMyAyNS4xLTMuMiAyOS44IiBmaWxsPSIjM2U0MzQ3Ii8+PHBhdGggZD0ibTkuMSAzMy45bDktNC4xYzAgMCA1LjMtMTQtNi4xLTI0LjEtMi40IDItNS4xIDI1LTIuOSAyOC4yIiBmaWxsPSIjZmZmIi8+PHBhdGggZD0iTTMyLDEzQzE4LjksMTMsMiwzMy42LDIsNDUuNEMyMC41LDQ1LjQsMTkuNyw2MiwzMiw2MnMxMS41LTE2LjYsMzAtMTYuNkM2MiwzMy42LDQ1LjEsMTMsMzIsMTN6IiBmaWxsPSIjZmY4NzM2Ii8+PGcgZmlsbD0iI2ZmZiI+PHBhdGggZD0iTTMyLDU2LjJjMCw1LjEsOS42LDQuMiw5LjUtMi45YzYuNy05LjQsMTkuOS04LjcsMTkuOS04LjdDMzkuNiwzMi40LDMyLDU2LjIsMzIsNTYuMnoiLz48cGF0aCBkPSJNMzIsNTYuMmMwLDUuMS05LjYsNC4yLTkuNS0yLjlDMTUuOCw0NCwyLjYsNDQuNywyLjYsNDQuN0MyNC40LDMyLjQsMzIsNTYuMiwzMiw1Ni4yeiIvPjwvZz48ZyBmaWxsPSIjZmY4NzM2Ij48cGF0aCBkPSJtNTMuNCAxOC41Yy00IC43LTQuOSA2LjMtNC45IDYuM2w2IDUuM2MtMi4zLTUuOS0xLjEtMTEuNi0xLjEtMTEuNiIvPjxwYXRoIGQ9Im01MS4xIDEzLjVjLTQuNCAzLjktNS4xIDguNy01LjEgOC43bDYgNS4zYy0yLjQtNS44LS45LTE0LS45LTE0Ii8+PHBhdGggZD0ibTEwLjYgMTguNWM0IC43IDQuOSA2LjMgNC45IDYuM2wtNiA1LjNjMi4zLTUuOSAxLjEtMTEuNiAxLjEtMTEuNiIvPjxwYXRoIGQ9Im0xMi45IDEzLjVjNC40IDMuOSA1LjEgOC43IDUuMSA4LjdsLTYgNS4zYzIuNC01LjguOS0xNCAuOS0xNCIvPjwvZz48cGF0aCBkPSJtNTIuOCAzMS4xYy01LjctMS44LTEwLjktMy40LTEzLjguOS0yLjQgMy43LjcgOS40LjcgOS40IDExLjIgMS4yIDEzLjEtMTAuMyAxMy4xLTEwLjMiIGZpbGw9IiMzZTQzNDciLz48ZWxsaXBzZSBjeD0iNDMiIGN5PSIzNi4zIiByeD0iNC4yIiByeT0iNC4xIiBmaWxsPSIjZDVmZjgzIi8+PGcgZmlsbD0iIzNlNDM0NyI+PGVsbGlwc2UgY3g9IjQzIiBjeT0iMzYuMyIgcng9IjIuNyIgcnk9IjIuNyIvPjxwYXRoIGQ9Im0xMS4yIDMxLjFjNS43LTEuOCAxMC45LTMuNCAxMy43LjkgMi40IDMuNy0uNyA5LjQtLjcgOS40LTExLjEgMS4yLTEzLTEwLjMtMTMtMTAuMyIvPjwvZz48ZWxsaXBzZSBjeD0iMjEiIGN5PSIzNi4zIiByeD0iNC4yIiByeT0iNC4xIiBmaWxsPSIjZDVmZjgzIi8+PGcgZmlsbD0iIzNlNDM0NyI+PGVsbGlwc2UgY3g9IjIxIiBjeT0iMzYuMyIgcng9IjIuNyIgcnk9IjIuNyIvPjxwYXRoIGQ9Im00MS4yIDQ3LjljLS43LTIuMy0xLjgtNC40LTMtNi41IDEuMSAyLjEgMiA0LjMgMi41IDYuNi41IDIuMy43IDQuNyAwIDYuOC0uNCAxLTEgMi0xLjggMi42LS44LjYtMS44IDEtMi43IDEtLjkgMC0xLjktLjMtMi41LTEtLjYtLjctLjktMS42LS44LTIuNmwtLjkuMmgtLjljMCAxLS4yIDEuOS0uOCAyLjYtLjYuNy0xLjUgMS0yLjUgMS0uOSAwLTEuOS0uNC0yLjctMS0uOC0uNi0xLjQtMS42LTEuOC0yLjYtLjgtMi4xLS42LTQuNiAwLTYuOC41LTIuMyAxLjUtNC41IDIuNS02LjYtMS4yIDItMi4zIDQuMS0zIDYuNS0uNyAyLjMtMS4xIDQuOC0uNCA3LjMuMyAxLjIgMSAyLjQgMS45IDMuMy45LjkgMi4xIDEuNCAzLjQgMS41IDEuMi4xIDIuNi0uMiAzLjctMS4yLjMtLjIuNS0uNS43LS44LjIuMy40LjYuNy44IDEgMSAyLjQgMS4zIDMuNyAxLjIgMS4zLS4xIDIuNC0uNyAzLjQtMS41LjktLjkgMS42LTIgMS45LTMuMy41LTIuNi4xLTUuMi0uNi03LjUiLz48cGF0aCBkPSJtMzcuNiA1MC4zYy0xLjEtMS4xLTQuNS0xLjItNS42LTEuMi0xIDAtNC41LjEtNS42IDEuMi0uOC44LS4yIDIuOCAxLjkgNC41IDEuMyAxLjEgMi42IDEuNCAzLjYgMS40IDEgMCAyLjMtLjMgMy42LTEuNCAyLjMtMS43IDIuOS0zLjcgMi4xLTQuNSIvPjwvZz48L3N2Zz4=";
> +
> +function shouldButtonHaveCustomStyling(selector, verifyFn, icon, message) {
> +  try {
> +    let element;
> +    if (selector == "#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon") {

Could you add a comment explaining why this selector needs to be different from all the others?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:28
(Diff revision 2)
> +function verifyButtonWithoutCustomStyling(selector, icon, message) {
> +  shouldButtonHaveCustomStyling(selector, (result, message) => ok(!result, message), icon, message);
> +}

I think it's a bit confusing having the same parameter name `message` for the function and the callback you are passing in. You might not need to pass the message to `shouldButtonHaveCustomStyling`. 

function verifyButtonWithoutCustomStyling(selector, icon, message) {
  shouldButtonHaveCustomStyling(selector, result => ok(!result, message), icon);
}

And for the function below:

function verifyButtonWithCustomStyling(selector, icon, message) {
  shouldButtonHaveCustomStyling(selector, result => ok(result, message), icon);
}

Would it work to rewrite functions like this?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:61
(Diff revision 2)
> +  };
> +  let files = {
> +    "fox.svg": imageBufferFromDataURI(ENCODED_IMAGE_DATA),
> +  };
> +
> +  let buttons = [

Perhaps something like `const ICON_INFO`?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:67
(Diff revision 2)
> +    ["bookmark_menu", "#bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon"],
> +    ["downloads", "#downloads-button", "downloads-button"],
> +    ["home", "#home-button", "home-button"],
> +    ["app_menu", "#PanelUI-menu-button"],
> +    ["cut", "#cut-button", "edit-controls"],
> +    ["copy", "#copy-button"],

Could you add a comment explaining how this array is structured?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:139
(Diff revision 2)
> +  } finally {
> +    CustomizableUI.reset();
> +    window.restore();
> +  }

Would it work to have a teardown task at the end instead of wrapping all of this in a try/catch? If so I think that would be a little cleaner.

Something like:

add_task(function* teardown() {
  CustomizableUI.reset();
  window.restore();
});
Comment on attachment 8842979 [details]
Bug 1343921 - Implement support for custom icons through the Theming API.

https://reviewboard.mozilla.org/r/116708/#review118686

This is looking great so far :-) I'm curious to hear what you think about the suggestion I made below.

Something else is to think about is the rebase you'll have to do today: where will the icons and their definitions live? Ideally somewhere in browser/ but where and how? Perhaps Matt knows?

::: browser/components/extensions/ext-theme.js:43
(Diff revision 3)
> +  "subscribe": "--subscribe-icon",
> +  "text_encoding": "--text_encoding-icon",
> +  "email_link": "--email_link-icon",
> +  "forget": "--forget-icon",
> +  "pocket": "--pocket-icon",
> +};

It looks like you don't need this map at all - just an array (or `Set([...])`) called `ICON_NAMES`

::: browser/components/extensions/ext-theme.js:149
(Diff revision 3)
>      }
>    }
>  
> +  loadIcons(icons) {
> +    let iconNames = Object.keys(ICONS_TO_VARIABLE_MAP);
> +    for (let icon of Object.keys(icons)) {

nit: `Object.getOwnPropertyNames(icons)`

::: browser/components/extensions/ext-theme.js:154
(Diff revision 3)
> +    for (let icon of Object.keys(icons)) {
> +      let val = icons[icon];
> +      if (!val) {
> +        continue;
> +      }
> +      if (iconNames.includes(icon)) {

```js
if (!val || !ICON_NAMES.has(icon))
  continue;
```

::: browser/components/extensions/ext-theme.js:155
(Diff revision 3)
> +      let val = icons[icon];
> +      if (!val) {
> +        continue;
> +      }
> +      if (iconNames.includes(icon)) {
> +        let variableName = ICONS_TO_VARIABLE_MAP[icon];

```js
let cssVar = `--${icon}-icon`;
```
Attachment #8842979 - Flags: review?(mdeboer) → review-
Comment on attachment 8843057 [details]
Bug 1343921 - Put the icon support behind a pref so it can be enabled once we are satisfied with the performance.

https://reviewboard.mozilla.org/r/116808/#review118698

Not a - per sé, but MozReview will get the least unhappy this way...
Attachment #8843057 - Flags: review?(mdeboer) → review-
Attachment #8843057 - Attachment is obsolete: true
Attachment #8843057 - Flags: review?(mwein)
Comment on attachment 8842979 [details]
Bug 1343921 - Implement support for custom icons through the Theming API.

https://reviewboard.mozilla.org/r/116708/#review119212

At first I was a bit on edge about stuffing the icon setting logic in LightweightThemeConsumer.jsm, but now I'm actually really happy about it! We'll investigate the Category Manager approach later on, if that becomes necessary.
I am interested in the performance implications of using CSS string matching operators, but that's also for later.

::: toolkit/components/extensions/ext-theme.js:11
(Diff revision 4)
>                                    "resource://gre/modules/Preferences.jsm");
>  
>  // WeakMap[Extension -> Theme]
>  let themeMap = new WeakMap();
>  
> +const ICONS = Preferences.get("extensions.webextensions.themes.icons.buttons",

nit: I think you can leave this one a single line.

::: toolkit/components/extensions/ext-theme.js:117
(Diff revision 4)
>        }
>      }
>    }
>  
> +  loadIcons(icons) {
> +    if (!Preferences.get("extensions.webextensions.themes.icons.enabled")) {

I forgot why we needed this to be behind a pref?
Attachment #8842979 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> > +  loadIcons(icons) {
> > +    if (!Preferences.get("extensions.webextensions.themes.icons.enabled")) {
> 
> I forgot why we needed this to be behind a pref?

We moved it behind a pref while we research the performance implications. This way we can enable the API before we enable the icon support, only enabling the icon support once we are satisfied with the performance characteristics.
Whiteboard: triaged
Comment on attachment 8842979 [details]
Bug 1343921 - Implement support for custom icons through the Theming API.

https://reviewboard.mozilla.org/r/116708/#review119280

Nice!

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:14
(Diff revision 4)
> +   *   within the DOM.
> +   * @param {function} resultModifierFn Used to invert the result, if needed.
> +   * @param {string} message The message that is printed to the console
> +   *   by the verifyFn.
> +   */
> +function verifyButtonProperties(selector, resultModifierFn, message) {

Nit: instead of taking a `resultModifierFn`, could you just have this take a boolean `shouldHaveCustomStyling` and use that to modify how you call `ok`?

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:140
(Diff revision 4)
> +  for (let button of ICON_INFO) {
> +    let iconInfo = icons.find(arr => arr[0] == button[0]);
> +    if (iconInfo[1]) {
> +      verifyButtonWithCustomStyling(button[1],
> +        `The ${button[1]} should have it's icon customized in the toolbar`);
> +    } else {
> +      verifyButtonWithoutCustomStyling(button[1],
> +        `The ${button[1]} should not have it's icon customized in the toolbar`);
> +    }
> +  }

Could you move this to a separate function since it's basically repeated below.

::: browser/components/extensions/test/browser/browser_ext_themes_icons.js:140
(Diff revision 4)
> +  for (let button of ICON_INFO) {
> +    let iconInfo = icons.find(arr => arr[0] == button[0]);
> +    if (iconInfo[1]) {
> +      verifyButtonWithCustomStyling(button[1],
> +        `The ${button[1]} should have it's icon customized in the toolbar`);
> +    } else {
> +      verifyButtonWithoutCustomStyling(button[1],
> +        `The ${button[1]} should not have it's icon customized in the toolbar`);
> +    }
> +  }

Could you move this to a separate function? It looks like it's repeated again below and the for loop above could also potentially be moved.
Attachment #8842979 - Flags: review?(mwein) → review+
Gr, sorry about the redundant comment.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1666512ef081
Implement support for custom icons through the Theming API. r=mattw,mikedeboer
Backed out for failing browser_ext_themes_chromeparity.js and browser_ext_themes_icons.js:

https://hg.mozilla.org/integration/autoland/rev/25f5d0648983899de6f0bed21f3c3c72e09e4867

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1666512ef0813b231b82a713075e09b614143f6f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log browser_ext_themes_chromeparity.js: https://treeherder.mozilla.org/logviewer.html#?job_id=81975878&repo=autoland

[task 2017-03-06T21:00:17.531310Z] 21:00:17     INFO - TEST-START | toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js
[task 2017-03-06T21:00:18.090502Z] 21:00:18     INFO - TEST-INFO | started process screentopng
[task 2017-03-06T21:00:19.242000Z] 21:00:19     INFO - TEST-INFO | screentopng: exit 0
[task 2017-03-06T21:00:19.242337Z] 21:00:19     INFO - Buffered messages logged at 21:00:17
[task 2017-03-06T21:00:19.242532Z] 21:00:19     INFO - Entering test bound setup
[task 2017-03-06T21:00:19.242721Z] 21:00:19     INFO - Leaving test bound setup
[task 2017-03-06T21:00:19.242929Z] 21:00:19     INFO - Entering test bound test_support_theme_frame
[task 2017-03-06T21:00:19.243097Z] 21:00:19     INFO - Extension loaded
[task 2017-03-06T21:00:19.243294Z] 21:00:19     INFO - Buffered messages logged at 21:00:18
[task 2017-03-06T21:00:19.245934Z] 21:00:19     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-image’.  Falling back to ‘initial’." {file: "chrome://browser/content/browser.css" line: 20 column: 664 source: " var(--lwt-header-image) "}]
[task 2017-03-06T21:00:19.249308Z] 21:00:19     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-image’.  Falling back to ‘initial’." {file: "chrome://browser/skin/browser.css" line: 2972 column: 95855 source: " linear-gradient(transparent 2px, rgba(255,255,255,.4) 2px, rgba(255,255,255,.4)), var(--lwt-header-image)"}]
[task 2017-03-06T21:00:19.256097Z] 21:00:19     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-image’.  Falling back to ‘initial’." {file: "chrome://browser/skin/browser.css" line: 2980 column: 96285 source: "                    var(--lwt-header-image)"}]
[task 2017-03-06T21:00:19.258493Z] 21:00:19     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-image’.  Falling back to ‘initial’." {file: "chrome://browser/skin/browser.css" line: 2972 column: 95855 source: " linear-gradient(transparent 2px, rgba(255,255,255,.4) 2px, rgba(255,255,255,.4)), var(--lwt-header-image)"}]
[task 2017-03-06T21:00:19.260669Z] 21:00:19     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js | LWT attribute should be set - true == true - 
[task 2017-03-06T21:00:19.268149Z] 21:00:19     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js | LWT text color attribute should be set - "bright" == "bright" - 
[task 2017-03-06T21:00:19.269792Z] 21:00:19     INFO - Buffered messages finished
[task 2017-03-06T21:00:19.274656Z] 21:00:19     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js | The backgroundImage should use face.png. Actual value is: none - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js :: test_support_theme_frame :: line 45
[task 2017-03-06T21:00:19.278517Z] 21:00:19     INFO - Stack trace:
[task 2017-03-06T21:00:19.279460Z] 21:00:19     INFO -     chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:test_support_theme_frame:45
[task 2017-03-06T21:00:19.280312Z] 21:00:19     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-06T21:00:19.283674Z] 21:00:19     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-06T21:00:19.285313Z] 21:00:19     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-06T21:00:19.287047Z] 21:00:19     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-06T21:00:19.289722Z] 21:00:19     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-03-06T21:00:19.291206Z] 21:00:19     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-03-06T21:00:19.292806Z] 21:00:19     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-03-06T21:00:19.294454Z] 21:00:19     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-03-06T21:00:19.296238Z] 21:00:19     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-06T21:00:19.299052Z] 21:00:19     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-06T21:00:19.300537Z] 21:00:19     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-06T21:00:19.302018Z] 21:00:19     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
[task 2017-03-06T21:00:19.303695Z] 21:00:19     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-03-06T21:00:19.307543Z] 21:00:19     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-06T21:00:19.309148Z] 21:00:19     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-06T21:00:19.310720Z] 21:00:19     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-06T21:00:19.312391Z] 21:00:19     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-03-06T21:00:19.313930Z] 21:00:19     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-03-06T21:00:19.315501Z] 21:00:19     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Failure log browser_ext_themes_icons.js: https://treeherder.mozilla.org/logviewer.html#?job_id=81976291&repo=autoland

[task 2017-03-06T21:10:30.943022Z] 21:10:30     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-image’.  Falling back to ‘initial’." {file: "chrome://browser/skin/browser.css" line: 2972 column: 95855 source: " linear-gradient(transparent 2px, rgba(255,255,255,.4) 2px, rgba(255,255,255,.4)), var(--lwt-header-image)"}]
[task 2017-03-06T21:10:30.946082Z] 21:10:30     INFO - listStyleImage for fox.svg is none
[task 2017-03-06T21:10:30.947924Z] 21:10:30     INFO - Buffered messages finished
[task 2017-03-06T21:10:30.949988Z] 21:10:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_themes_icons.js | The #back-button should have it's icon customized in the toolbar - Got false, expected true
[task 2017-03-06T21:10:30.955515Z] 21:10:30     INFO - Stack trace:
[task 2017-03-06T21:10:30.956890Z] 21:10:30     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
[task 2017-03-06T21:10:30.958625Z] 21:10:30     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_icons.js:verifyButtonProperties:35
[task 2017-03-06T21:10:30.960265Z] 21:10:30     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_icons.js:verifyButtonWithCustomStyling:62
[task 2017-03-06T21:10:30.962025Z] 21:10:30     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_icons.js:checkButtons:79
[task 2017-03-06T21:10:30.964945Z] 21:10:30     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_icons.js:runTestWithIcons:164
[task 2017-03-06T21:10:30.971131Z] 21:10:30     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-06T21:10:30.972776Z] 21:10:30     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-06T21:10:30.974314Z] 21:10:30     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-06T21:10:30.978779Z] 21:10:30     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-06T21:10:30.980746Z] 21:10:30     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-03-06T21:10:30.983979Z] 21:10:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-03-06T21:10:30.988853Z] 21:10:30     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-03-06T21:10:30.989234Z] 21:10:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-03-06T21:10:30.989577Z] 21:10:30     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-06T21:10:30.991392Z] 21:10:30     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-06T21:10:30.994038Z] 21:10:30     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-06T21:10:30.996885Z] 21:10:30     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
[task 2017-03-06T21:10:30.999552Z] 21:10:30     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-03-06T21:10:31.002141Z] 21:10:30     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-06T21:10:31.003777Z] 21:10:30     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-06T21:10:31.008549Z] 21:10:30     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-06T21:10:31.011478Z] 21:10:30     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-03-06T21:10:31.014379Z] 21:10:30     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-03-06T21:10:31.018119Z] 21:10:30     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4f9dfc7fee5
Implement support for custom icons through the Theming API. r=mattw,mikedeboer
https://hg.mozilla.org/mozilla-central/rev/e4f9dfc7fee5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1348039
Blocks: themingapi-more-ui
No longer blocks: themingapi
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.