1330340 - Theming API - implement support for the LWT properties

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1330340 - Implement support for lightweight theme properties (headerURL, accentcolor, textcolor) for Theming API.

Review commit: https://reviewboard.mozilla.org/r/106874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106874/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I didn't use the transparent GIF approach that was used on cedar because it isn't necessary at this point. Lightweight themes required that all properties be defined, so for support we merely need to require that all are present (excluding footerURL which we can leave out).

Later when we extend the API to allow various colors/images, we can remove the requirement that they are all specified.

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> I didn't use the transparent GIF approach that was used on cedar because it
> isn't necessary at this point. Lightweight themes required that all
> properties be defined, so for support we merely need to require that all are
> present (excluding footerURL which we can leave out).

I wasn't particularly fond of that code too btw, so I vote to keep the hacky transparent GIF approach out this implementation.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8829913 [details]
Bug 1330340 - Implement support for lightweight theme properties (headerURL, accentcolor, textcolor) for Theming API.

https://reviewboard.mozilla.org/r/106874/#review107930

\o/ second piece of the puzzle

::: browser/components/extensions/ext-theme.js:12
(Diff revision 1)
>  /* eslint-disable mozilla/balanced-listeners */
>  extensions.on("manifest_theme", (type, directive, extension, manifest) => {
>    let enabled = Preferences.get("extensions.webextensions.themes.enabled");
> -  extension.emit("test-message", "themes-enabled", enabled);
>  
> -  if (enabled) {
> +  if (enabled && manifest && manifest.theme) {

Shall we just bail-out early here?
```js
if (!enabled || !manifest || !manifest.theme) {
  return;
}
```

::: browser/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:62
(Diff revision 1)
> +      "theme": {
> +        "images": {
> +          "headerURL": kBackground,
> +        },
> +      },
> +    },

<rant>this trailing comma rule makes me cringe to no end</rant>

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829913 [details]
Bug 1330340 - Implement support for lightweight theme properties (headerURL, accentcolor, textcolor) for Theming API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106874/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90f3467f8b8c
Implement support for lightweight theme properties (headerURL, accentcolor, textcolor) for Theming API. r=mikedeboer

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8829913 [details]
Bug 1330340 - Implement support for lightweight theme properties (headerURL, accentcolor, textcolor) for Theming API.

https://reviewboard.mozilla.org/r/106874/#review108076

::: browser/components/extensions/ext-theme.js:12
(Diff revision 2)
>  /* eslint-disable mozilla/balanced-listeners */
>  extensions.on("manifest_theme", (type, directive, extension, manifest) => {
>    let enabled = Preferences.get("extensions.webextensions.themes.enabled");
> -  extension.emit("test-message", "themes-enabled", enabled);
>  
> -  if (enabled) {
> +  if (!enabled || !manifest || !manifest.theme) {

nit: `manifest` and `manifest.theme` will always be truthy so we can remove them from this check.

::: browser/components/extensions/ext-theme.js:22
(Diff revision 2)
> +  if (manifest.theme.colors) {
> +    let colors = manifest.theme.colors;
> +    for (let color of Object.getOwnPropertyNames(colors)) {
> +      let val = colors[color];
> +      // Since values are optional, they may be `null`.
> +      if (val === null) {

nit: the schema parser won't accept `null` as a value for a color, so this will never happen.

::: browser/components/extensions/ext-theme.js:26
(Diff revision 2)
> +      // Since values are optional, they may be `null`.
> +      if (val === null) {
> +        continue;
> +      }
> +
> +      if (color == "accentcolor") {

nit: maybe you use a switch here?

::: browser/components/extensions/ext-theme.js:40
(Diff revision 2)
> +
> +  if (manifest.theme.images) {
> +    let images = manifest.theme.images;
> +    for (let image of Object.getOwnPropertyNames(images)) {
> +      let val = images[image];
> +      if (val === null) {

nit: the schema parser won't accept `null` as a value for a image, so this will never happen.

::: browser/components/extensions/ext-theme.js:50
(Diff revision 2)
> +        lwtStyles.headerURL = val;
> +      }
> +    }
> +  }
> +
> +  if (lwtStyles.headerURL &&

Just curious - why do you require all three properties to be set here?

::: browser/components/extensions/schemas/theme.json:28
(Diff revision 2)
> +          "colors": {
> +            "type": "object",
> +            "optional": true,
> +            "properties": {
> +              "accentcolor": {
> +                "type": "string",

I think it would be nice if we did some better schema validation on the color instead of accepting arbitrary length strings. I'd suggest creating a new Color type that allows only hex or rgb(a) strings, and having each color property reference it.

See the commands schema for an example: http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/commands.json#13.

::: browser/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:3
(Diff revision 2)
> +"use strict";
> +
> +const kBackground = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==";

nit: please use `g` instead of `k` for webextension globals.

::: browser/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:13
(Diff revision 2)
> +  hex = parseInt((hex.indexOf("#") > -1 ? hex.substring(1) : hex), 16);
> +  return [hex >> 16, (hex & 0x00FF00) >> 8, (hex & 0x0000FF)];
> +}
> +
> +add_task(function* setup() {
> +  yield SpecialPowers.pushPrefEnv({

Do you need to set the pref back to false after the test shuts down?

::: browser/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:73
(Diff revision 2)
> +  Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
> +  yield extension.unload();
> +  Assert.ok(!docEl.hasAttribute("lwtheme"), "LWT attribute should not be set");
> +});
> +
> +add_task(function* testLWTRequiresAllPropertiesDefinedColorsOnly() {

nit: could you consider using underscores here and above to make the names read better? (e.g testLWT_RequiresAllPropertiesDefined_ColorsOnly)

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/90f3467f8b8c

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Matthew Wein [:mattw] from comment #7)
> nit: `manifest` and `manifest.theme` will always be truthy so we can remove
> them from this check.

We'll include it in the next update to ext-theme.js.

> nit: the schema parser won't accept `null` as a value for a color, so this
> will never happen.

The value may be `null` for optional types, right? At least, that's what I'm seeing when I dump the schema parser result to the console. We don't have those right now, but we did on the cedar branch.

> nit: maybe you use a switch here?

I think I mentioned or thought the same, but ultimately concluded it's not important for this patch. We'll re-organize/ micro-refactor with subsequent patches.


> nit: the schema parser won't accept `null` as a value for a image, so this
> will never happen.

Same as above. The loop is generic for all future `image` properties, so this filters out omitted optional ones in the future.

> Just curious - why do you require all three properties to be set here?

See comment 2.

> I think it would be nice if we did some better schema validation on the
> color instead of accepting arbitrary length strings. I'd suggest creating a
> new Color type that allows only hex or rgb(a) strings, and having each color
> property reference it.

Can you file a bug? The engineering plan mentions a wide range of color values that we'll support, so this deserves its own bug, methinks. I can totally see you volunteering to take it ;-)

> nit: please use `g` instead of `k` for webextension globals.

This isn't a mutable global, but an immutable constant.

> Do you need to set the pref back to false after the test shuts down?

No, `pushPrefEnv` does that for you. Neat, eh?

> nit: could you consider using underscores here and above to make the names
> read better? (e.g testLWT_RequiresAllPropertiesDefined_ColorsOnly)

FWIW, I don't feel that your example reads much better, but that's a matter of taste.

Comment 10

[Matthew Wein [:mattw]]

Thanks for following up. My intention is to bring awareness to these (minor) issues and have them resolved either here or in subsequent patches, so I'm happy that you responded that way for several of them already.

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Matthew Wein [:mattw] from comment #7)
> > nit: `manifest` and `manifest.theme` will always be truthy so we can remove
> > them from this check.
> 
> We'll include it in the next update to ext-theme.js.

Sgtm.

> 
> > nit: the schema parser won't accept `null` as a value for a color, so this
> > will never happen.
> 
> The value may be `null` for optional types, right? At least, that's what I'm
> seeing when I dump the schema parser result to the console. We don't have
> those right now, but we did on the cedar branch.

Sorry, I should've made this an open question instead of asserting how I think it works.. I think the parser excludes these values if they aren't included or null, but I'll have to double check and could be wrong about this. In either case, it doesn't really hurt to have these checks.

> 
> > nit: maybe you use a switch here?
> 
> I think I mentioned or thought the same, but ultimately concluded it's not
> important for this patch. We'll re-organize/ micro-refactor with subsequent
> patches.

Sgtm.

> 
> > Just curious - why do you require all three properties to be set here?
> 
> See comment 2.

Thanks!

> 
> > I think it would be nice if we did some better schema validation on the
> > color instead of accepting arbitrary length strings. I'd suggest creating a
> > new Color type that allows only hex or rgb(a) strings, and having each color
> > property reference it.
> 
> Can you file a bug? The engineering plan mentions a wide range of color
> values that we'll support, so this deserves its own bug, methinks. I can
> totally see you volunteering to take it ;-)

Will do :)

> 
> > nit: please use `g` instead of `k` for webextension globals.
> 
> This isn't a mutable global, but an immutable constant.

Again, I should've made this an open question instead of asserting how I thought it should be done. I think it's still a global, but you're right, we do treat them differently. I'm pretty sure in the webextensions code we try to use caps with underscores for immutable constants. Just thought it was worth bringing up for consistency's sake. Like you've mentioned above, this can obviously be updated in a later patch.

> 
> > Do you need to set the pref back to false after the test shuts down?
> 
> No, `pushPrefEnv` does that for you. Neat, eh?

Yeah, it really is :) 

I wasn't able to find any good documentation for it unfortunately :/

> 
> > nit: could you consider using underscores here and above to make the names
> > read better? (e.g testLWT_RequiresAllPropertiesDefined_ColorsOnly)
> 
> FWIW, I don't feel that your example reads much better, but that's a matter
> of taste.

That's fair. It looks like most of our test names use underscores, like `test_LWT_requires_all_properties_defined_colors_only`, which is different from what I had suggested. I think it would be good for us to try to be consistent with this if we can, even though it might not necessary read better. This can be changed in a later patch of course if we agree to.

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Matthew Wein [:mattw] from comment #10)
> Will do :)

\o/

> Again, I should've made this an open question instead of asserting how I
> thought it should be done. I think it's still a global, but you're right, we
> do treat them differently. I'm pretty sure in the webextensions code we try
> to use caps with underscores for immutable constants. Just thought it was
> worth bringing up for consistency's sake. Like you've mentioned above, this
> can obviously be updated in a later patch.

I'd like us to follow the WebExtensions style wherever possible, so ALL_CAPS works for me.

> Yeah, it really is :) 
> 
> I wasn't able to find any good documentation for it unfortunately :/

O yes, documentation and hidden 'features' are what we are most proud of, it sometimes seems. However, this is exactly where we need to do better. Hence me writing the 'Definition of Complete' in the final Engineering Plan to include _good docs_!

> That's fair. It looks like most of our test names use underscores, like
> `test_LWT_requires_all_properties_defined_colors_only`, which is different
> from what I had suggested. I think it would be good for us to try to be
> consistent with this if we can, even though it might not necessary read
> better. This can be changed in a later patch of course if we agree to.

Alright, seems to me that us converting to the lowercase test function names is a good idiom to follow here. Let's.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Hence me writing the 'Definition of Complete' in the final Engineering
> Plan to include _good docs_!

Whoops, I added it just now :o)

Comment 13

[Tomislav Jovanovic :zombie]

Attachment: Bug 1330340 - Check host permissions for webRequest listeners

Review commit: https://reviewboard.mozilla.org/r/108314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108314/