Closed
Bug 1330341
Opened 8 years ago
Closed 8 years ago
Theming API - implement support for dynamic updates
Categories
(WebExtensions :: Frontend, defect)
WebExtensions
Frontend
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: mikedeboer, Assigned: mattw)
References
Details
(Whiteboard: user-story, triaged)
User Story
As a user I’d like to programmatically alter the appearance of the browser, thus complementing the static theme definition in the manifest. I’d like a single chrome.theme.update(propertyBag) function to be exposed to my WebExtension background scripts. I’d like the property-bag to be a simple object which may only contain properties that are defined in the schema as well. Example: ```js chrome.theme.update({ colors: { background_tab: [255, 170, 255] } }); ``` Example uses of this API would be a background script which fetches data from weather.com and shows specific background images that fit the current weather type, or a background script checks the time of year and shows specific background images that fit the current meteorological season.
Attachments
(1 file)
No description provided.
Reporter | ||
Updated•8 years ago
|
Summary: Theming API - implement support for the LWT properties → Theming API - implement support for background scripts
Reporter | ||
Updated•8 years ago
|
Summary: Theming API - implement support for background scripts → Theming API - implement support for dynamic updates
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Reporter | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Whiteboard: user-story → user-story, triaged
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8830602 [details]
Bug 1330341 - Add support for dynamic updates
https://reviewboard.mozilla.org/r/107344/#review108520
I like the changes you made, but when you refactor out all the things, I guess I hoped you'd go with the full-monty class setup we used on the cedar branch. Can you do that, please? So have theme class instances mapped to extension instances. Thanks!
Attachment #8830602 -
Flags: review?(mdeboer) → review-
Reporter | ||
Comment 4•8 years ago
|
||
Oh, and please don't forget to document the code! We missed that the previous round, but it's not too much work at this point still ;-)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830602 [details]
Bug 1330341 - Add support for dynamic updates
https://reviewboard.mozilla.org/r/107344/#review108520
Yep, happy to do that here :) I also added some documentation in areas that I thought could use explanation. I think most of the code that we have currently is self explanatory on its own, so I'm a little hesistant to add any more documentation.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
Proposing an addition to the user story - As a theme developer, if I have a theme loaded in the manifest and I want to make minor updates to it, I should be able to obtain the properties of my theme to avoid overwriting them. I may also want to make changes to my theme based on the current values of each property. To do this, I propose adding a method which returns an object containing all of the supported properties and their values in the same format as the Schema's ThemeType object. Thoughts?
Flags: needinfo?(mdeboer)
Flags: needinfo?(jaws)
Reporter | ||
Comment 9•8 years ago
|
||
(In reply to Matthew Wein [:mattw] from comment #8)
> Proposing an addition to the user story - As a theme developer, if I have a
> theme loaded in the manifest and I want to make minor updates to it, I
> should be able to obtain the properties of my theme to avoid overwriting
> them. I may also want to make changes to my theme based on the current
> values of each property. To do this, I propose adding a method which returns
> an object containing all of the supported properties and their values in the
> same format as the Schema's ThemeType object. Thoughts?
I like the idea, but we'll need a broader discussion about it, I think, to go through the technical details. We can talk about this coming Wednesday during our team meeting or would you like to schedule something separately on Monday?
Flags: needinfo?(mdeboer)
Reporter | ||
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8830602 [details]
Bug 1330341 - Add support for dynamic updates
https://reviewboard.mozilla.org/r/107344/#review108862
Excellent, looking really nice. r=me with comments addressed.
::: browser/components/extensions/ext-theme.js:17
(Diff revision 4)
> - // Since values are optional, they may be `null`.
> - if (val === null) {
> - continue;
> - }
> + }
>
> - if (color == "accentcolor") {
> + load(details) {
I understand what you're saying about the API being rather self-explanatory, but I was hinting more at us adopting the rule to add docblock comments atop each class and function definition we write, in the most modern style:
```js
/**
* Loads a theme by reading the properties from the
* extension's manifest.
*
* @param {Object} details Theme part of the manifest
*/
```
::: browser/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:7
(Diff revision 4)
> +
> +const BACKGROUND_1 = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==";
> +const ACCENT_COLOR_1 = "#a14040";
> +const TEXT_COLOR_1 = "#fac96e";
> +
> +const BACKGROUND_2 = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAABGdBTUEAAK/INwWK6QAAABl0RVh0U29mdHdhcmUAQWRvYmUgSW1hZ2VSZWFkeXHJZTwAAAHWSURBVHjaYvz//z8DJQAggJiQOe/fv2fv7Oz8rays/N+VkfG/iYnJfyD/1+rVq7ffu3dPFpsBAAHEAHIBCJ85c8bN2Nj4vwsDw/8zQLwKiO8CcRoQu0DxqlWrdsHUwzBAAIGJmTNnPgYa9j8UqhFElwPxf2MIDeIrKSn9FwSJoRkAEEAM0DD4DzMAyPi/G+QKY4hh5WAXGf8PDQ0FGwJ22d27CjADAAIIrLmjo+MXA9R2kAHvGBA2wwx6B8W7od6CeQcggKCmCEL8bgwxYCbUIGTDVkHDBia+CuotgACCueD3TDQN75D4xmAvCoK9ARMHBzAw0AECiBHkAlC0Mdy7x9ABNA3obAZXIAa6iKEcGlMVQHwWyjYuL2d4v2cPg8vZswx7gHyAAAK7AOif7SAbOqCmn4Ha3AHFsIDtgPq/vLz8P4MSkJ2W9h8ggBjevXvHDo4FQUQg/kdypqCg4H8lUIACnQ/SOBMYI8bAsAJFPcj1AAEEjwVQqLpAbXmH5BJjqI0gi9DTAAgDBBCcAVLkgmQ7yKCZxpCQxqUZhAECCJ4XgMl493ug21ZD+aDAXH0WLM4A9MZPXJkJIIAwTAR5pQMalaCABQUULttBGCCAGCnNzgABBgAMJ5THwGvJLAAAAABJRU5ErkJggg==";
Can you tell me what this background is? Perhaps fun to mention in a comment above it.
Attachment #8830602 -
Flags: review?(mdeboer) → review+
Comment 11•8 years ago
|
||
BACKGROUND_1 is a red dot, and BACKGROUND_2 is the Mozilla dino head.
In response to comment #8, yeah I can see how this could be useful, but I think we should handle that in a different bug.
Flags: needinfo?(jaws)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Matthew Wein [:mattw] from comment #8)
> > Proposing an addition to the user story - As a theme developer, if I have a
> > theme loaded in the manifest and I want to make minor updates to it, I
> > should be able to obtain the properties of my theme to avoid overwriting
> > them. I may also want to make changes to my theme based on the current
> > values of each property. To do this, I propose adding a method which returns
> > an object containing all of the supported properties and their values in the
> > same format as the Schema's ThemeType object. Thoughts?
>
> I like the idea, but we'll need a broader discussion about it, I think, to
> go through the technical details. We can talk about this coming Wednesday
> during our team meeting or would you like to schedule something separately
> on Monday?
Discussing on Wednesday sounds good to me, and I agree with Jared about handling it in a separate bug if we decide to go forward with it.
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830602 [details]
Bug 1330341 - Add support for dynamic updates
https://reviewboard.mozilla.org/r/107344/#review108862
> I understand what you're saying about the API being rather self-explanatory, but I was hinting more at us adopting the rule to add docblock comments atop each class and function definition we write, in the most modern style:
> ```js
> /**
> * Loads a theme by reading the properties from the
> * extension's manifest.
> *
> * @param {Object} details Theme part of the manifest
> */
> ```
Yeah, I think it would be good for us to start doing this early on.
> Can you tell me what this background is? Perhaps fun to mention in a comment above it.
Jared beat me to it, but I added comments to the patch.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/486e2936e7dd
Add support for dynamic updates r=mikedeboer
Keywords: checkin-needed
Comment 17•8 years ago
|
||
sorry had to back this out since this conflicted with merging m-c to autoland in browser/components/extensions/ext-theme.js
Flags: needinfo?(mwein)
Comment 18•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34273d4cd0fb
Backed out changeset 486e2936e7dd for causing merge conflicts
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years ago
|
||
Thanks, this should merge fine now.
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 21•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c15a4cab253
Add support for dynamic updates r=mikedeboer
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•