Provide a theme unload function

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Frontend
P5
normal
RESOLVED FIXED
2 months ago
20 days ago

People

(Reporter: andym, Assigned: andym)

Tracking

({dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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)

Updated

2 months ago
Assignee: nobody → amckay
Comment hidden (mozreview-request)

Updated

2 months ago
Priority: -- → P5
Whiteboard: triaged

Comment 2

2 months ago
mozreview-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

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 3

2 months ago
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 4

2 months ago
mozreview-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

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+
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 6

2 months ago
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
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

2 months ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8cfe015562d6
Backed out changeset 005957262022 for eslint failures
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
Wierd, those eslint failures don't show up locally, but I've changed it to match the tree herder log.
Keywords: checkin-needed

Comment 11

2 months ago
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

Comment 12

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/851550a08fc1
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

20 days ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.