Richard Marti (:Paenglab) Reporter
	Description • 6 years ago

WebExtension themes need a restart to apply.

Last good is 2010527
First bad is 2010528

	Jorg K (CEST = GMT+2)
	Comment 1 • 6 years ago

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.

	Richard Marti (:Paenglab) Reporter
	Comment 2 • 6 years ago

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.

	Jorg K (CEST = GMT+2)
	Comment 3 • 6 years ago

So why isn't the bug filed for FF?

	Richard Marti (:Paenglab) Reporter
	Comment 4 • 6 years ago

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.

	Tim Nguyen :ntim
	Comment 5 • 6 years ago

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.

	Jorg K (CEST = GMT+2)
	Comment 6 • 6 years ago

(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.

	Andrew Swan [:aswan] Assignee
	Comment 7 • 6 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 8 • 6 years ago

(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.

	Tim Nguyen :ntim
	Comment 10 • 6 years ago

Can we have a test for this ?

	Andrew Swan [:aswan] Assignee
	Comment 11 • 6 years ago

(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.

	Andrew Swan [:aswan] Assignee
	Comment 12 • 6 years ago

(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...

	Tomislav Jovanovic :zombie
	Comment 13 • 6 years ago
mozreview-review

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

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

	Pulsebot
	Comment 14 • 6 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa67de9bfa1c
Fix a pair of WE theme issues r=zombie

	Dorel Luca [:dluca]
	Comment 15 • 6 years ago
bugherder

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

	marius.santa
	Comment 16 • 6 years ago

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