1465938 - WE static themes need restart to be effective

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Richard Marti (:Paenglab)]

WebExtension themes need a restart to apply.

Last good is 2010527
First bad is 2010528

Comment 1

[Jorg K (CEST = GMT+2)]

Only in TB or also in FF?

So Daily 2010527 is M-C 4e9446f9e8f0a75c7ffe063f1dfb311cc9 and Daily 2010528 is M-C a466172aed4bc2afc21169b749b8068a4b?
You could have pasted those here from the troubleshooting information :-(

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e9446f9e8f0a75c7ffe063f1dfb311cc9&tochange=a466172aed4bc2afc21169b749b8068a4b

That range is mostly dominated by Kris ;-) Also adding Tim who knows this area.

Comment 2

[Richard Marti (:Paenglab)]

FX has also this problems. Switching between WE themes or between built-in themes works but switching from a WE theme to a built-in dark/light theme or vice versa doesn't work.

Comment 3

[Jorg K (CEST = GMT+2)]

So why isn't the bug filed for FF?

Comment 4

[Richard Marti (:Paenglab)]

Because I found during my last comment that it works between WE themes but not by changing between WE theme and built-in theme.  On FX I'm always using WE themes.

Comment 5

[Tim Nguyen :ntim]

Narrowed down the range to: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=95175977ef46cb68ea989411fea514a621af1df9&tochange=0461688b6a091d3088f32206a449d9add7142948

That range unfortunately has a lot of different bugs landing at the same time, which makes it harder to pin-point which exact bug is guilty.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Tim Nguyen :ntim from comment #5)
> Narrowed down the range to: ...
You've done great :-)

> That range unfortunately has a lot of different bugs landing at the same
> time, which makes it harder to pin-point which exact bug is guilty.
That's my Daily bread: Something breaks in TB and I have regression ranges of up to 300 changesets. You've got 12 here, and I guess removing dead/obsolete stuff doesn't do harm. So you could easily bisect this. That said, author and reviewer of those changesets, CC'ed already, might have a hunch why their stuff is affecting WE themes. When I apply a WE theme in TB, I see it flashing up for a split second, and then it disappears again. Maybe that's a hint.

Comment 7

[Andrew Swan [:aswan]]

Argh there appear to be multiple problems created by the addons async patches.  Capturing my thoughts here before I get distracted:

One problem is here:
https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/toolkit/mozapps/extensions/LightweightThemeManager.jsm#358-368

In particular, the `themeChanged(null)` line.  Before all the recent async changes that Kris landed this code ran before the webextension theme code that broadcast the new theme changes, now it runs after (so I actually see the browser flash briefly into the new theme style and then switch back to the default).

Even dating back to pre-webextension-theme days, it seems that changing from one theme to another always entails calling themeChanged(null) (to un-apply the old theme?) and then themeChanged(newTheme).  But from what I can tell, the code in LightweightThemeConsumer.jsm that actually applies the update should work just fine if we skip that first call.

A second problem is that this code was never updated to use enable()
https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/toolkit/mozapps/extensions/AddonManager.jsm#1830-1836

However, fixing these is causing an additional regression in the Themes panel of about:addons.  I'll keep looking at this.

Comment 8

[Andrew Swan [:aswan]]

(In reply to Andrew Swan [:aswan] from comment #7)
> However, fixing these is causing an additional regression in the Themes
> panel of about:addons.  I'll keep looking at this.

Eh, never mind I was thrown off by the default theme not having any enabled buttons when it is active but of course that is the expected behavior.  Patch coming shortly.  This stuff remains woefully under-tested but that's not something I think we can address right now.

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1465938 Fix a pair of WE theme issues

1. Stop broadcasting an extra theme update event when changing themes
2. Use enable() to enable themes upon install

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

Comment 10

[Tim Nguyen :ntim]

Can we have a test for this ?

Comment 11

[Andrew Swan [:aswan]]

(In reply to Tim Nguyen :ntim from comment #10)
> Can we have a test for this ?

I would welcome one but I don't have time to create one.

Comment 12

[Andrew Swan [:aswan]]

(hit submit too fast)

We'll also be removing LWT support in the not-too-distant future so the current LWT implementation is in basic maintenance mode...

Comment 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8982633 [details]
Bug 1465938 Fix a pair of WE theme issues

https://reviewboard.mozilla.org/r/248598/#review254858

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa67de9bfa1c
Fix a pair of WE theme issues r=zombie

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/aa67de9bfa1c

Comment 16

[marius.santa]

Attachment: theme changing with no restart.gif

Reproduced in Firefox 62.0a1 (20180530100110).
Retested and verified in Firefox 62.0a1 (20180607100059) on Windows 10 64Bit and MacOS 10.13.4.