58 bytes, text/x-review-board-request
STR: 0. have machines (a) and (b) set up to sync add-ons and themes, both using the default theme 1. use one of the builtin lwthemes on machine (a). 2. wait for this change to sync to machine (b) (if you like, help it along with some manual syncs. 3. on machine (a), deselect the lwtheme 4. wait for syncs ER on machine (b): i) the theme changes back to default ii) the add-on manager UI shows the default theme to be enabled AR: i) the theme is not the default in existing windows (but newly opened windows have the right theme!) ii) the add-on manager UI shows the default theme to be enabled (meaning I can't manually correct this situation).
I reproduced on beta, but I bet later versions are affected as well.
Best I can tell this bug is caused by Sync first setting the lightweightThemes.selectedThemeID preference, then trying to trick the theme manager into updating to reflect the pref, but this seems to confuse the theme manager - it seems stuck in a place where it thinks the pref itself must be correct even though it doesn't reflect its internal state. The fix I came up with is to special case this preference value, then instead of setting the pref then asking the theme manager to use it, just tell the theme manager to change the theme and *it* updates the pref to match.
Assignee: nobody → markh
(alternatively, I guess we could consider it a bug in the theme manager that just setting the pref does the wrong thing, but that seems a yak...)
Comment on attachment 8787031 [details] Bug 1287748 - set the lightweight theme directly when Syncing the theme preference. https://reviewboard.mozilla.org/r/75878/#review73974 I agree this looks like the simplest solution. Updating the lwt code to be pref-change-based would be... challenging. Not sure it's even possible given other constraints (at the least, that the add-on manager is *also* doing its own admin of all of these items... case of too many cooks in the kitchen, me thinks.)
Attachment #8787031 - Flags: review?(gijskruitbosch+bugs) → review+
I don't suppose this is testable at a high level? That might be nice to give us some more test coverage for this kind of thing...
(In reply to :Gijs Kruitbosch from comment #7) > I don't suppose this is testable at a high level? That might be nice to give > us some more test coverage for this kind of thing... Thanks! It's somewhat testable at a low level, but not at a high-level using the theme manager itself - we don't really have a sane test story for Sync tests in the browser. I refactored things a little to put the theme manager interactions in their own helper function and had our xpcshell tests verify that help is called (or not called) at the appropriate times. I'll push and land that.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/d3524b6a9a75 set the lightweight theme directly when Syncing the theme preference. r=Gijs
(In reply to Sylvestre Ledru [:sylvestre] from comment #12) > Is it something we want to uplift to 50? Not a regression, impact is relatively low and doesn't persist across restarts, so I think it's fine to let this ride the trains.
You need to log in before you can comment on or make changes to this bug.