Closed Bug 1330341 Opened 7 years ago Closed 7 years ago

Theming API - implement support for dynamic updates

Categories

(WebExtensions :: Frontend, defect)

Product:
WebExtensions
Component:
Frontend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
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)

Bug 1330341 - Add support for dynamic updates
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
      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
https://hg.mozilla.org/mozilla-central/rev/8c15a4cab253
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
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

Creator:
Created:
Updated:
Size: