Closed Bug 1373850 Opened 3 years ago Closed 3 years ago

Provide a theme unload function

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: andy+bugzilla, Assigned: andy+bugzilla)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

Using browser.theme.update() I can change the theme of the browser. But you can't undo that change and reset the browser to the way it was before. This would be useful if you want to change the theme temporarily.

This is simple enough to do by calling the already existing theme.unload() method, it would just be a matter of wrapping that in the schema.

The only problem is that we haven't got a stack for the themes. So if I set the theme to be the Dark Theme, then call theme.update(), then theme.unload() we get back to the default theme. 

But this is true of themes right now. If in about:addons I enable Compact Light, then Compact Dark, when I disable Compact Dark I go back to the Default theme. So its probably not worth worrying about.
Assignee: nobody → amckay
Priority: -- → P5
Whiteboard: triaged
Comment on attachment 8878730 [details]
bug 1373850 add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme

https://reviewboard.mozilla.org/r/150050/#review155756

Thanks! I just have some nits to consider.

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:22
(Diff revision 1)
>  
> -function validateTheme(backgroundImage, accentColor, textColor) {
> +function validateTheme(backgroundImage, accentColor, textColor, checkLWT) {
>    let docEl = window.document.documentElement;
>    let style = window.getComputedStyle(docEl);
>  
> +  if (checkLWT) {

nit: Could you rename this to `isLWT` instead?

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:104
(Diff revision 1)
> +
> +  extension.sendMessage("reset-theme");
> +
> +  await extension.awaitMessage("theme-reset");
> +
> +  validateTheme(

nit: I think this would be easier to read if it was kept on one line. I think one option to simplify it would be:

let {backgroundImage, backgroundColor, color} = defaultStyle;
validateTheme(backgroundImage, backgroundColor, color, false);
Attachment #8878730 - Flags: review?(mwein) → review+
Comment on attachment 8878730 [details]
bug 1373850 add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme

Adding Mike as an additional reviewer for the stamp of approval.
Attachment #8878730 - Flags: review?(mdeboer)
Comment on attachment 8878730 [details]
bug 1373850 add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme

https://reviewboard.mozilla.org/r/150050/#review156220

LGTM, Thanks!!

::: commit-message-2d20b:1
(Diff revision 1)
> +bug 1373850 add theme.reset r?mattw

I vote for a wee bit more elaborate commit message.
Attachment #8878730 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/005957262022
add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme r=mattw,mikedeboer
Keywords: checkin-needed
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8cfe015562d6
Backed out changeset 005957262022 for eslint failures
Wierd, those eslint failures don't show up locally, but I've changed it to match the tree herder log.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/851550a08fc1
add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme r=mattw,mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/851550a08fc1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: dev-doc-needed
Docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/reset

Please let me know if this covers it.
Flags: needinfo?(amckay)
lgtm, thanks
Flags: needinfo?(amckay)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.