Closed Bug 1330341 Opened 8 years ago Closed 8 years ago

Theming API - implement support for dynamic updates

Categories

(WebExtensions :: Frontend, defect)

defect
Not set
normal

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.
Summary: Theming API - implement support for the LWT properties → Theming API - implement support for background scripts
Depends on: 1330342
Summary: Theming API - implement support for background scripts → Theming API - implement support for dynamic updates
Depends on: 1330346
Assignee: nobody → mwein
Status: NEW → ASSIGNED
Whiteboard: user-story → user-story, triaged
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-
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 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.
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)
(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)
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+
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)
(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.
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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/486e2936e7dd Add support for dynamic updates r=mikedeboer
Keywords: checkin-needed
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)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/34273d4cd0fb Backed out changeset 486e2936e7dd for causing merge conflicts
Thanks, this should merge fine now.
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c15a4cab253 Add support for dynamic updates r=mikedeboer
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
No longer depends on: 1330342
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: