Closed
Bug 1176703
Opened 9 years ago
Closed 9 years ago
Theme overrides don't work in safe mode and cause missing/wrong images
Categories
(Toolkit :: Themes, defect)
Toolkit
Themes
Tracking
()
VERIFIED
FIXED
mozilla42
People
(Reporter: elbart, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(2 files)
44.51 KB,
image/png
|
Details | |
40 bytes,
text/x-review-board-request
|
benjamin
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
m-c: 2:40.59 LOG: MainThread Bisector INFO Last good revision: 7d4ab4a9febd (2015-06-07) 2:40.59 LOG: MainThread Bisector INFO First bad revision: 4700d1cdf489 (2015-06-08) 2:40.59 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7d4ab4a9febd&tochange=4700d1cdf489 m-i: 1:47.54 LOG: MainThread Bisector INFO Last good revision: ab9471275a90 1:47.55 LOG: MainThread Bisector INFO First bad revision: 9ef529a9a02b 1:47.55 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab9471275a90&tochange=9ef529a9a02b fx-team: 5:34.95 LOG: MainThread Bisector INFO Last good revision: 653299500d3c 5:34.96 LOG: MainThread Bisector INFO First bad revision: 39821d952adc 5:34.96 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=653299500d3c&tochange=39821d952adc 39821d952adc Gijs Kruitbosch — Bug 1150417 - use theme manifest for overrides, r=glandium,dao 264fb3d4d4c6 Gijs Kruitbosch — Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r=bsmedberg
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Navbar-icons in safemode have different color to normal mode → Theme overrides don't work in safe mode and cause missing/wrong images
Assignee | ||
Comment 1•9 years ago
|
||
[Tracking Requested - why for this release]: This will cause missing icons in e.g. about:addons
Blocks: 1150417
status-firefox40:
--- → affected
status-firefox41:
--- → affected
tracking-firefox40:
--- → ?
tracking-firefox41:
--- → ?
Assignee | ||
Comment 2•9 years ago
|
||
I don't really understand why the default theme manifest is no longer read in this case, when it is clearly still the theme that's in use... Dave, do you know? As a minimal fix for 40 we could move the overrides for images that do not otherwise exist back to the regular manifest file, or back the change out if we can't find a way of fixing this.
Flags: needinfo?(dtownsend)
Comment 3•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > I don't really understand why the default theme manifest is no longer read > in this case, when it is clearly still the theme that's in use... Dave, do > you know? Ugh right, because we don't load extension manifests when in safe mode.
Flags: needinfo?(dtownsend)
Adding a tracking flag. We should not be messing up the icons in safe mode.
Assignee | ||
Comment 5•9 years ago
|
||
Benjamin, where does the code live that adds these manifests and checks for safe mode? I've been looking for this last week but I couldn't find it. Would changing those to make an exception for the default theme be doable, or should we consider reverting the decision to move the overrides from the main manifest into the extension manifest? (if we leave the overrides-allowed-in-themes thing in, at least themes can workaround by using overrides themselves rather than packaging extra files in the right places, I guess...)
Flags: needinfo?(benjamin)
Comment 6•9 years ago
|
||
The code which loads theme directories is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#644 I don't know how this interacts with safe mode. I suspect it might be through this check: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7106
Flags: needinfo?(benjamin)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #6) > I don't know how this interacts with safe mode. I suspect it might be > through this check: > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProvider.jsm#7106 Nope, right here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#623
Assignee | ||
Comment 8•9 years ago
|
||
Thanks to both of you!
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 9•9 years ago
|
||
Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg
Attachment #8630369 -
Flags: review?(benjamin)
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/12711/#review11383 ::: toolkit/xre/nsXREDirProvider.cpp:657 (Diff revision 1) > + } This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #10) > https://reviewboard.mozilla.org/r/12711/#review11383 > > ::: toolkit/xre/nsXREDirProvider.cpp:657 > (Diff revision 1) > > + } > > This makes assumptions about the default theme's ID that I think we'll > forget down the road. FWIW, I thought about this, but we make this assumption in a lot of places, including automated tests and our packaging manifests, which is why this seemed acceptable... > I think it would work to just only load the ThemeDirs > from the ini and make sure that the value of general.skins.selectedSkin is > ignored in safe mode (I assume it is already but let's check) ... but this does sound neater. I can look at this later today, I hope. Setting ni so I don't forget...
Flags: needinfo?(gijskruitbosch+bugs)
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/12711/#review11383 > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check) I'm similarly concerned, although I'll accept this if we can't find a more elegant solution. Waiting on your investigations later today before marking anything.
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #12) > https://reviewboard.mozilla.org/r/12711/#review11383 > > > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check) > > I'm similarly concerned, although I'll accept this if we can't find a more > elegant solution. Waiting on your investigations later today before marking > anything. So, if I do this: 1) lightweight themes remain applied in safe mode 2) full themes don't remain applied *but* the same problem we're trying to fix here reoccurs, in that the overrides from the default theme aren't being applied, if your selected theme isn't the default theme (yeah... not sure what's happening there). So it seems like if we want to go this route we need more work in the add-on manager / lwtheme code. I can do that, but it will take more time and I'm worried about running out of time for 40. Benjamin, thoughts? (my test patch: https://pastebin.mozilla.org/8839005 )
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #13) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #12) > > https://reviewboard.mozilla.org/r/12711/#review11383 > > > > > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check) > > > > I'm similarly concerned, although I'll accept this if we can't find a more > > elegant solution. Waiting on your investigations later today before marking > > anything. > > So, if I do this: > > 1) lightweight themes remain applied in safe mode > 2) full themes don't remain applied *but* the same problem we're trying to > fix here reoccurs, in that the overrides from the default theme aren't being > applied, if your selected theme isn't the default theme (yeah... not sure > what's happening there). > > So it seems like if we want to go this route we need more work in the add-on > manager / lwtheme code. I can do that, but it will take more time and I'm > worried about running out of time for 40. > > Benjamin, thoughts? > > (my test patch: https://pastebin.mozilla.org/8839005 )
Flags: needinfo?(benjamin)
Comment 15•9 years ago
|
||
Comment on attachment 8630369 [details] MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg This patch is simpler, and simple is better here. I'm hoping to get rid of heavy themes anyway since that is a necessary part of kill-XUL, and then the problem will just go away.
Flags: needinfo?(benjamin)
Attachment #8630369 -
Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/d9402c81ffdb https://hg.mozilla.org/mozilla-central/rev/5995a3f88211
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8630369 [details] MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg Approval Request Comment [Feature/regressing bug #]: bug 1150417 / windows theme work [User impact if declined]: missing/wrong images in safe mode [Describe test coverage new/current, TreeHerder]: nope... [Risks and why]: reasonably low, change specific to how we deal with the default theme's manifest in safe mode [String/UUID change made/needed]: nope
Attachment #8630369 -
Flags: approval-mozilla-beta?
Attachment #8630369 -
Flags: approval-mozilla-aurora?
Comment 20•9 years ago
|
||
Comment on attachment 8630369 [details] MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg We want the safe mode to be safe! Taking it.
Attachment #8630369 -
Flags: approval-mozilla-beta?
Attachment #8630369 -
Flags: approval-mozilla-beta+
Attachment #8630369 -
Flags: approval-mozilla-aurora?
Attachment #8630369 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Flags: qe-verify+
Comment 23•9 years ago
|
||
Confirming the fix for: - latest 42.0a1 Nightly, build ID: 20150715095506; - latest 41.0a2 Aurora, build ID: 20150715095519; - Firefox 40.0b4, build ID: 20150713153304.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•