1435191 - Enforce limit in additional_backgrounds field

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect, P2)

Description

[Mike de Boer [:mikedeboer]]

Enforce a limit to the amount of background images that a theme can provide; too many may end up causing performance issues, because it may increase the time necessary to paint a window.

I'm thinking of a new pref, something like 'extensions.webextensions.themes.backgrounds.limit' and an initial value of '15' - that's the 14 alignment (as noted at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#properties) options plus 1 to round it up.

Comment 1

[Tim Nguyen :ntim]

I'm not sure, but we may be able to use schema validation for that.

Comment 2

[Vivek]

Attachment: Bug 1435191 - Enforce limit in additional_backgrounds field

This patch adds a check to make sure themes do not load more than 15 images, when making use of via additional_backgrounds property. If there are more than 15 images, theme will not be loaded.

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

Comment 3

[Vivek]

Hey I'd like to work on this if thats fine. I'm not sure I completely understand adding the new pref part, but I've put together something really simple to start. It won't load the theme if there are more than 15 additional_backgrounds.

If I can work on this, please let me know if what I did is similar to what is expected in comment 1. Or if not, what should I be looking to do instead?

If there is something else planned for this or it's just harder than it looks, thats fine too. I can look for something else :)

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8967923 [details]
Bug 1435191 - Enforce limit in additional_backgrounds field

https://reviewboard.mozilla.org/r/236624/#review242400


Code analysis found 1 defect in this patch:
 - 1 defect 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 1)
>  
>      // Lightweight themes require accentcolor and textcolor to be defined.
> -    if (this.lwtStyles.accentcolor &&
> +    // If lightweight themes have additionalBackgrounds, it should have no more than 15 images
> +    if (this.lwtStyles.additionalBackgrounds && this.lwtStyles.additionalBackgrounds.length > 15) {
> +      this.logger.warn("Your theme includes more than 15 images in images: 'additional_backgrounds'");
> +    }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

Comment 5

[Vivek]

Comment on attachment 8967923 [details]
Bug 1435191 - Enforce limit in additional_backgrounds field

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

Comment 6

[Tim Nguyen :ntim]

While your approach also works, what I meant by schema validation is changing schemas/theme.json to use a maxItems property (maxItems: 15).

Comment 7

[Caitlin Neiman [:caitmuenster]]

Hey Vivek, how's it going with this bug?

Comment 8

[Tim Nguyen :ntim]

Attachment: Bug 1435191 - Enforce limit in additional_backgrounds field.

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

Comment 9

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

Comment on attachment 8993158 [details]
Bug 1435191 - Enforce limit in additional_backgrounds field.

https://reviewboard.mozilla.org/r/257968/#review264894

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd213fedfae9
Enforce limit in additional_backgrounds field. r=jaws

Comment 11

[Natalia Csoregi [:nataliaCs]]

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

Comment 12

[Victor Carciu]

Can you please add some STRs on this issue ?

Comment 13

[Tim Nguyen :ntim]

Covered by schema validation, which is itself covered by automated tests.