133666 - Skin switch on restart not completely performed

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect)

Description

[John Morrison]

Skin switch on restart not completely performed

Steps to reproduce:

1) create a new profile 
2) start browser (comes up in default skin, assume Classic is default for the 
   discussion below.)
3) 'View->Apply Themes->Modern' and say OK to the alert
4) Shut down. Start again.

Actual results:
  Buttons and menu-backgrounds are still in 'Classic'

5) Shut down. Start again.

Actual results:
  Everything is now fully 'Modern'

6) 'View->Apply Themes->Classic' and say OK to the alert
7) Shut down. Start again.

Actual results:
  Buttons and menu-backgrounds are still in 'Modern'

8) Shut down. Start again.

Actual results:
  Everything is now fully 'Classic'

Tested with 2002-03-26-10-trunk comm. and with mozilla trunk build on win2k.
Reproducible 100%

nsbeta1, mozilla1.0

Comment 1

[Asa Dotzler [:asa]]

critical to mozilla1.0. adding keyword.

Comment 2

[Peter Trudelle]

nsbeta1+/adt3 per Nav triage team

Comment 3

[John Morrison]

*** Bug 134946 has been marked as a duplicate of this bug. ***

Comment 4

[Vadim Berezniker]

os -> all per dupe

Comment 5

[Pete Boyd]

this bug is related to bug 115940

Comment 6

[Aaron Kaluszka]

I don't think that's directly related.. dynamic switching has always done that,
while switching after restart has worked.

Comment 7

[André Dahlqvist]

WFM with 2002-04-05-10 on Linux. Both Modern -> Classic and the other way around
are completely done switching theme after a restart for me.

Comment 8

[André Dahlqvist]

John, have you been able to reproduce this bug with newer builds than 03-26?

Comment 9

[John Morrison]

Sure. This is reproducible as described above with mozilla builds pulled in the 
last few hours on both Linux (rh6.1) and win32(win2k).

Comment 10

[Boris Zbarsky [:bzbarsky]]

ccing people who may have an idea of what's up here.

Comment 11

[Tracy Walker [:tracy]]

note:  restarting a second time after switching themes makes the mixed theme 
problem go away.

Comment 12

[André Dahlqvist]

Tracy, when the themes are mixed does it looks the same as the bug on dynamic
theme switch? Can you attach a screenshot?

Comment 13

[Patty Mac]

Andre, I can attach the screenshot for you. It seems happen on all platforms
changing from PC to all platforms.

Comment 14

[Patty Mac]

using this build (2002-04-11-08-TRUNK)

Comment 15

[Patty Mac]

Attachment: Look at Search button

Comment 16

[André Dahlqvist]

Strange that I cannot see it. I have tested on several Linux machines at school
(latest build) as well as on my Linux box at home, and all widgets are changed
to the correct theme.

Comment 17

[Sören 'Chucker' Kuklau (gone)]

Confirming pmac's issues. Saw the same behaviour multiple times on recent Win32
trunk builds.

Comment 18

[Asa Dotzler [:asa]]

removing from the RC1 tracking bug.

Comment 19

[Jonadab the Unsightly One (Nathan Eady)]

I've seen this when switching between modern and lo-fi on 
Linux, but I'm still mostly using 0.9.9 (but note that not
needing to restart was not the issue; first restart did
not change everything).  Note that if you have extra 
toolbars (e.g., prefs toolbar, googlebar, uabar) these 
are least likely to have the correct theme applied
the first time.  No idea why.  

This only happens occasionally, and a subsequent additional
restart of Mozilla invariably fixes it.  It is not 100% 
reproducable for me.

I'll start using a more recent build and see if I can catch
this happening again.  I switch themes fairly often (because
I'm in the process of creating one, but at the moment it's 
half done and looks like it has mange, so I don't use it for
regular browsing) so if anyone can reproduce this I probably can.  

Adding self to Cc.  

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

John, are you using QuickLaunch?

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

could be a chrome registry problem

Comment 22

[John Morrison]

No. I am not using quickbrunch.

Comment 23

[Peter Trudelle]

Mass moving nsbeta1+/adt3 bugs assigned to Navigator team engineers out to
target milestone 1.0.1.  Please confine your attentions to driving down our list
of TM 1.0 bugs for beta.  Better to help, debug, or test one of them than fix
one of these.

Comment 24

[Jacob Hallén]

What actually happens is that when Mozilla shuts down with the
Modern theme active, the line
user_pref("general.skins.selectedSkin", "modern/1.0");
is struck from the prefs.js file.
It happens even if you have selected Modern in the prefs and
it does not happen if you shut down with the Classic theme
active, but Modern selected in the prefs.

Comment 25

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 26

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 27

[John Morrison]

Good grief. Skin switching DOES NOT WORK, either in fact, or in the perception 
of end users (who will point this out to us ad infinitum). Need I say more 
about why a BETA release should not ship with this defect?

Comment 28

[Peter Trudelle]

->blaker

Comment 29

[John Morrison]

*** Bug 141358 has been marked as a duplicate of this bug. ***

Comment 30

[R.K.Aa.]

*** Bug 141728 has been marked as a duplicate of this bug. ***

Comment 31

[John Morrison]

Here comes the wah-mbulance .... Wah! This defect is going to be mozilla 1.0?

Comment 32

[Blake Ross]

Attachment: patch

Comment 33

[Blake Ross]

What happens is FlushCaches() reloads the scoped stylesheets, which (because of
the profile manager) include listbox.css, checkbox.css, popup.css, and
button.css.  But since it does so before we SelectSkin(), which updates all the
selectedSkin arcs, we load the old stylesheets (of the skin we're about to
switch out of).  This patch simply reorders the calls to ensure that we refetch
the new skin's scoped stylesheets.

Comment 34

[David Hyatt]

sr=hyatt

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 82365 [details] [diff] [review]
patch

r=[e-mail]

(adding sr checkmark for hyatt)

Comment 36

[Blake Ross]

Checked into trunk, marking fixed.

But I strongly urge adt to accept this for beta, it's an easy patch and this is
an obvious problem, especially for reviewers trying out theme switching.  I also
see that drivers want this since it's mozilla1.0+...

Comment 37

[andreww]

Yes I would second that. This is imperative to get into the Beta as we are going
to be updating the three skins from the themepark (toyfactory, fogcity, american
theme) for RTM and need to be able to test these out using the beta.

Althought theme switching might seem like a minor issue right now, it is
something that I suspect would be an easy target for a magazine reviewer to ding
us on, and developers of themes to get put off by this defect.  

Comment 38

[grylchan]

*** Bug 144815 has been marked as a duplicate of this bug. ***

Comment 39

[Patty Mac]

Skin switching works ok on the trunk build (2002-05-17-08-TRUNK). Marking 
verified.

Comment 40

[Mike Shaver (:shaver emeritus)]

I want for rc3, adding to the no-suck-list.

Comment 41

[John Morrison]

(I now realize that this condition applies if you have >1 profile, which means 
the profile manager appears (comment #33). Nonetheless, still something that 
must be fixed in mozilla1.0 final (with adt considerations being mostly moot)).

Comment 42

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch, pending
Drivers' approval.  After, checking in, please add the fixed1.0 keyword.

Comment 43

[Scott Collins]

Comment on attachment 82365 [details] [diff] [review]
patch

a=scc (on behalf of drivers) for checkin to the mozilla1.0 branch

Comment 44

[timeless]

mozilla/rdf/chrome/src/nsChromeRegistry.cpp,v  <--  
nsChromeRegistry.cpp
new revision: 1.240.2.3; previous revision: 1.240.2.2
fix checked into 1.0.0 branch

Comment 45

[Christine Wozniak]

Adding custrtm- to comment field.