Closed Bug 1344926 Opened 5 years ago Closed 5 years ago

Installed static theme does not persist if a new window is launched

Categories

(WebExtensions :: Frontend, defect)

54 Branch
defect
Not set
normal

Tracking

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 disabled, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: krupa--use, Assigned: mikedeboer)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached file test-theme-2.zip
Nightly 54.0a1 (2017-03-06) (64-bit)

steps to reproduce:
1. Launch the latest Nightly
2. Install the attached static theme
3. Open a new window by File -> New Window

expected behavior:
Installed static theme persists when user launches a new window

actual behavior:
Installed static theme does not persist when user launches a new window

Other notes:
LWTs persist across windows as expected.

screencast:

Comparison between LWT and static themes when user launches a new window - https://gfycat.com/VacantMediumDolphin
Note that for the static theme to work, you need to set the pref extensions.webextensions.theme.enabled to true.
Component: Theme → WebExtensions: Frontend
Product: Firefox → Toolkit
I couldn't reproduce this issue using the test theme attached because once I tried to upload it, I was informed that it's a "Duplicate Submission".
I also tried to reproduce it using two already existing themes but with no success, so I couldn't set the flags and to see if it'a a regression.
I can reproduce it with one of the themes attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1343921
Assignee: nobody → jaws
Status: NEW → ASSIGNED
This should have been fixed by bug 1330349. Bug 864562 just changed how themes are rendered, bug bug 1330349 was supposed to integrate webextension themes to act like lightweight themes.

To reproduce this bug, follow these steps:

In about:config, enable the following prefs:
extensions.webextensions.themes.enabled;true
extensions.webextensions.themes.icons.enabled;true
xpinstall.signatures.required;false

Install the attached "face4.zip" through the add-on manager using the Gear icon -> "Install add-on from local file"

Place a breakpoint at LightweightThemeConsumer's constructor where it calls this._update(this.LightweightThemeManager.currentThemeForDisplay). currentThemeForDisplay is null, it seems that the selectedThemeID pref isn't set since it returns an empty string.
Blocks: 1330349
No longer blocks: 864562
Attached file face4.zip (obsolete) —
Attached file face6.zip
Fixing addon-id.
Attachment #8844488 - Attachment is obsolete: true
In extensions.js:1303 `aAddon.userDisabled = false;` causes LightweightThemeManager.jsm:561 to get with traditional lightweight themes. However with the attached face6.zip, that line in extensions.js causes XPIProvider.jsm:7527 to get called.
Thanks for investigating, Jared! I'll be taking this on as a follow-up to bug 1330349.
Assignee: jaws → mdeboer
I reproduce it using the steps provided by Jared Wein on Firefox Nightly 55.0a1, but I still can't set the flags for the other Firefox Nightly versions because I didn't find this pref "extensions.webextensions.themes.icons.enabled" to set its value to "true", and I don't know any other prefs which can do the trick, and if I try to upload the theme any way a pop-up informs me that "This add-on couldn't be installed because it has not been verified".
Comment on attachment 8845423 [details]
Bug 1344926 - make sure that WebExtension theme data is persisted across windows when they open and image data is persisted to disk as well.

https://reviewboard.mozilla.org/r/118612/#review120756

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:139
(Diff revision 1)
>  
>      let data = null;
>      if (selectedThemeID) {
>        data = this.getUsedTheme(selectedThemeID);
> +    } else if (_fallbackThemeData) {
> +      data = _fallbackThemeData;

Can we only do this in currentThemeForDisplay?
Comment on attachment 8845423 [details]
Bug 1344926 - make sure that WebExtension theme data is persisted across windows when they open and image data is persisted to disk as well.

https://reviewboard.mozilla.org/r/118612/#review121098
Attachment #8845423 - Flags: review?(dtownsend) → review+
Comment on attachment 8847092 [details]
Bug 1344926 - add a regression test to make sure themes are persisted across windows using the Theming API.

https://reviewboard.mozilla.org/r/120148/#review122086
Attachment #8847092 - Flags: review?(dtownsend) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3550fcaeae31
make sure that WebExtension theme data is persisted across windows when they open and image data is persisted to disk as well. r=mossop
https://hg.mozilla.org/integration/autoland/rev/5cbd00cc8a95
add a regression test to make sure themes are persisted across windows using the Theming API. r=mossop
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d0cb81121a0
make sure that WebExtension theme data is persisted across windows when they open and image data is persisted to disk as well. r=mossop
https://hg.mozilla.org/integration/autoland/rev/e8dc090888b0
add a regression test to make sure themes are persisted across windows using the Theming API. r=mossop
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(mdeboer)
From https://hg.mozilla.org/mozilla-central/rev/9d0cb81121a0#l2.12,
+// Holds optional fallback theme data that will be returned when no data for an
+// active theme can be found. This the case for WebExtension Themes, for example.
+var _fallbackThemeData = null;

Why can't WebExtension themes get stored in the currentTheme property? Why do we need to introduce another property that sounds basically the same?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> Why can't WebExtension themes get stored in the currentTheme property? Why
> do we need to introduce another property that sounds basically the same?

Because for that to work I'd need to rewire the initialization logic in LightweightThemeManager.jsm, which is already fumbled into submission to just work together with XPIProvider.jsm, and have it instantiate, manage and defer start of WebExtension themes.
I feel that we'd like to keep the interesting, working bits of LWT - the consumer - but gradually make sure we can rid ourselves of the manager counter-part.
You can also look at it this way: we bypass the manager's styling update broadcast of 'lightweight-theme-styling-update' and do it ourselves in ext-theme.js. Same difference, since we'd need to jump through multiple hoops just to be able to use the same flow as LWTs there.

Once there are no 'classic' LWTs left on AMO, we'll be able to retire our use of the LightweightThemeManager.jsm, which is something we'd all feel good about.
Flags: needinfo?(mdeboer)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.