Closed
Bug 1435191
Opened 7 years ago
Closed 6 years ago
Enforce limit in additional_backgrounds field
Categories
(WebExtensions :: Themes, defect, P2)
WebExtensions
Themes
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: mikedeboer, Assigned: ntim)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good-next-bug])
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [good-next-bug]
Updated•7 years ago
|
Component: WebExtensions: Frontend → WebExtensions: Themes
Assignee | ||
Comment 1•7 years ago
|
||
I'm not sure, but we may be able to use schema validation for that.
Summary: Theming API Performance tracking → Enforce limit in additional_backgrounds field
Comment hidden (mozreview-request) |
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•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
While your approach also works, what I meant by schema validation is changing schemas/theme.json to use a maxItems property (maxItems: 15).
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ntim.bugs
Flags: needinfo?(vivek3zero)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8967923 -
Attachment is obsolete: true
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8993158 [details]
Bug 1435191 - Enforce limit in additional_backgrounds field.
https://reviewboard.mozilla.org/r/257968/#review264894
Attachment #8993158 -
Flags: review?(jaws) → review+
Comment 10•6 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd213fedfae9
Enforce limit in additional_backgrounds field. r=jaws
Comment 11•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Comment 13•6 years ago
|
||
Covered by schema validation, which is itself covered by automated tests.
Flags: needinfo?(ntim.bugs) → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•