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

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: markh)

Tracking

48 Branch
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

(Whiteboard: [sync-quality])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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).
Reporter

Comment 1

3 years ago
I reproduced on beta, but I bet later versions are affected as well.
Version: 50 Branch → 48 Branch
Priority: -- → P2
Whiteboard: [sync-quality]
Assignee

Comment 2

3 years ago
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
Assignee

Comment 3

3 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 6

3 years ago
mozreview-review
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+
Reporter

Comment 7

3 years ago
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...
Priority: P2 → P1
Assignee

Comment 8

3 years ago
(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 hidden (mozreview-request)

Comment 10

3 years ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/d3524b6a9a75
set the lightweight theme directly when Syncing the theme preference. r=Gijs

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d3524b6a9a75
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Is it something we want to uplift to 50?
Assignee

Comment 13

3 years ago
(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.
Assignee

Updated

3 years ago
Duplicate of this bug: 1238955
Reporter

Updated

3 years ago
Duplicate of this bug: 1196161
You need to log in before you can comment on or make changes to this bug.