Tim Nguyen :ntim Assignee
	Description • 7 years ago

Making them mandatory only made sense when we only supported those 3 properties. Now we support many more properties, so these shouldn't be mandatory.

	Tim Nguyen :ntim Assignee
	Comment 1 • 7 years ago

What do you think about this?

	Tim Nguyen :ntim Assignee
	Comment 2 • 7 years ago

Some investigation into this: removing the check that makes the 3 properties mandatory doesn't work well.

There is a theme existence check in LightweightThemeConsumer.jsm based on headerURL, and not specifying that makes the check falsy.


Also, a lot of lightweight theme CSS is based on the assumption that accentcolor/textcolor/headerURL are always defined.

	Tim Nguyen :ntim Assignee
	Comment 3 • 7 years ago

A very simple fix would be adding:

if (!details.images || !details.images.headerURL) {
  details.images.headerURL = "";
}

to ext-theme.js and removing the checks making accentcolor/textcolor/headerURL mandatory.

A better fix would be refactoring LightweightThemeConsumer.jsm to stop using `active = !!aData.headerURL` as "theme is active" check, but that's quite complicated to do with the current setup unfortunately.

	Mike de Boer [:mikedeboer]
	Comment 4 • 7 years ago

These properties are mandatory, because we don't want theme authors to create invalid Light-Weight Themes.
If you want the properties to become optional, then we need to move this validation to ext-theme.js:

 1. If only LWT properties are present in the manifest, kick off validation that none of them is missing.
    Ex.: When only 'accentcolor' is defined in the manifest, then we should assume that it's trying to 'behave' like a LWT,
         thus we should fail validation, saying 'textcolor' and 'headerURL' are also required in this case.
 2. Exempt the theme.update() function from this requirement & skip the validation step. LWTs are manifest-only (near future, I know), so we needn't impose this restriction to other theme API consumers.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 7 years ago

I am fine with removing the requirements once we can confirm that themes without these will not look broken on our tier 1 platforms (linux, osx, and windows). We should also confirm that AMO can handle and display them fine.

	Tim Nguyen :ntim Assignee
	Comment 6 • 6 years ago

This is actually an essential part for chrome compatibility.

	Tim Nguyen :ntim Assignee
	Comment 7 • 6 years ago

headerURL was taken care of in bug 1404688

	Tim Nguyen :ntim Assignee
	Comment 8 • 6 years ago

Both the patch in bug 1404688 and the refactoring in bug 1417883 made this much easier. The accentcolor/textcolor is decoupled from the rest of LWTConsumer.jsm.

	Code Review Bot [:reviewbot]
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8991000 [details]
Bug 1413144 - Make accentcolor and textcolor optional.

https://reviewboard.mozilla.org/r/255962/#review262842


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:91
(Diff revision 2)
> -        defaultTheme = this;
> +      defaultTheme = this;
> -      }
> +    }
> -      onUpdatedEmitter.emit("theme-updated", this.details, this.windowId);
> +    onUpdatedEmitter.emit("theme-updated", this.details, this.windowId);
>  
> +    let lwtData = {
> +      theme: this.lwtStyles

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/extensions/parent/ext-theme.js:297
(Diff revision 2)
>      }
>    }
>  
>    static unload(windowId) {
> -    let lwtStyles = {
> -      headerURL: "",
> +    let lwtData = {
> +      theme: null

Error: Missing trailing comma. [eslint: comma-dangle]

	Code Review Bot [:reviewbot]
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8991000 [details]
Bug 1413144 - Make accentcolor and textcolor optional.

https://reviewboard.mozilla.org/r/255962/#review262850


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:89
(Diff revision 1)
> -        defaultTheme = this;
> +      defaultTheme = this;
> -      }
> +    }
> -      onUpdatedEmitter.emit("theme-updated", this.details, this.windowId);
> +    onUpdatedEmitter.emit("theme-updated", this.details, this.windowId);
>  
> +    let lwtData = {
> +      theme: this.lwtStyles

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/extensions/parent/ext-theme.js:295
(Diff revision 1)
>      }
>    }
>  
>    static unload(windowId) {
> -    let lwtStyles = {
> -      headerURL: "",
> +    let lwtData = {
> +      theme: null

Error: Missing trailing comma. [eslint: comma-dangle]

	Jared Wein [:jaws] (please needinfo? me)
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8991000 [details]
Bug 1413144 - Make accentcolor and textcolor optional.

https://reviewboard.mozilla.org/r/255962/#review262946

::: toolkit/components/extensions/parent/ext-theme.js:94
(Diff revision 3)
> +    if (this.windowId) {
> +      lwtData.window =
> +        getWinUtils(windowTracker.getWindow(this.windowId)).outerWindowID;

Can you combine this with the block at line 83?

::: toolkit/modules/LightweightThemeConsumer.jsm:211
(Diff revision 3)
> +    for (let icon of ICONS) {
> +      let value = aData.icons ? aData.icons[`--${icon}--icon`] : null;

Thanks, it looks like you did this to fix a bug with _update in which we weren't removing an icon that was set in a previous update.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf951826537f
Make accentcolor and textcolor optional. r=jaws

	Raul Gurzau (:RaulG)
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bf951826537f

	Vlad Jiman
	Comment 20 • 6 years ago

Hello, is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

	Will Bamberg [:wbamberg]
	Comment 21 • 6 years ago

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#colors, and also the the compat data: https://github.com/mdn/browser-compat-data/pull/2772, although the compat data isn't deployed to the Wiki yet.

Marking dev-doc-complete, but please let me know if we need anything else.