1373850 - Provide a theme unload function

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Andy McKay]

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.

Comment 1

[Andy McKay]

Attachment: bug 1373850 add browser.theme.reset which allows unloading changes caused by browser.theme.update and resets to the default theme

Review commit: https://reviewboard.mozilla.org/r/150050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150050/

Comment 2

[Matthew Wein [:mattw]]

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);

Comment 3

[Matthew Wein [:mattw]]

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.

Comment 4

[Mike de Boer [:mikedeboer]]

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.

Comment 5

[Andy McKay]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150050/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=109939543&repo=autoland for the last four failures in https://treeherder.mozilla.org/logviewer.html#?job_id=109939543&repo=autoland (the first two are something else).

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cfe015562d6
Backed out changeset 005957262022 for eslint failures

Comment 9

[Andy McKay]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150050/diff/2-3/

Comment 10

[Andy McKay]

Wierd, those eslint failures don't show up locally, but I've changed it to match the tree herder log.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/851550a08fc1

Comment 13

[Will Bamberg [:wbamberg]]

Docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/reset

Please let me know if this covers it.

Comment 14

[Andy McKay]

lgtm, thanks