1287748 - Syncing away from lwtheme updates internal state / add-on manager UI, but not the actual browser

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[:Gijs (out for now; he/him)]

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

Comment 1

[:Gijs (out for now; he/him)]

I reproduced on beta, but I bet later versions are affected as well.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1287748 - set the lightweight theme directly when Syncing the theme preference.

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

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8787031 [details]
Bug 1287748 - set the lightweight theme directly when Syncing the theme preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75878/diff/1-2/

Comment 6

[:Gijs (out for now; he/him)]

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

Comment 7

[:Gijs (out for now; he/him)]

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

Comment 8

[Mark Hammond [:markh] [:mhammond]]

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

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8787031 [details]
Bug 1287748 - set the lightweight theme directly when Syncing the theme preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75878/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3524b6a9a75
set the lightweight theme directly when Syncing the theme preference. r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Is it something we want to uplift to 50?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

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