190174 - Migrate profile, unable to switch from Classic to Modern or Modern to Classic

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect)

Description

[Ninoschka Baca]

Trunk build 2003-01-21: Mac 10.1.5 (trunk 2002-12-18 and 7.01 builds)

Overview: Migrate a profile and the current skin is Classic. Change to Modern
but it never changes.

Steps to reproduce:
1. Migrate a profile (currently, it's classic by default)
2. Select View|Apply Theme|Modern and it prompts that the changes will not take
effect until after a restart.
3. Exit/Restart

Actual Results: It still displays the classic theme. I tried deleting the "XUL
Fast Load File" and "localstore.rdf" but the problem still occured.

Interesting...New profiles don't have this problem, can switch between classic
and modern.

Expected Results: Switching from the Classic to Modern theme should be possilbe.

Additional Information:
1. 2003-01-21: WinMe, ok.

Comment 1

[Ninoschka Baca]

Marking nsbeta1 because in a migrated profile you should be able to switch from
a Classic skin to Modern. This is very inconvenient for users with accessibility
issues since there are so many regressions in the classic skin.

If Modern became the default skin for the Mac then the accessibility issue would
be of less concern.

Comment 2

[sairuh (rarely reading bugmail)]

esther found that this was a problem with 12/16 trunk builds.

patty / grace, do you know if there's a bug filed for this already? nbaca and i
couldn't find one.

Comment 3

[Conrad Carlen (not reading bugmail)]

> Interesting...New profiles don't have this problem, can switch between classic
and modern.

That is interesting - Does this bug exist if the profile was migrated long ago,
or is it only with freshly migrated profiles?

Reason I mention freshly migrated is that the Mach-O build doesn't migrate 4.x
profiles.

Comment 4

[sairuh (rarely reading bugmail)]

see also bug 190336.

Comment 5

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 6

[Grace Bush]

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

Comment 7

[Ninoschka Baca]

Renominating since this is a regression and a basic feature that should work. 
Switching themes on the Mac appears to work but Windows continues to have the
problem.

Comment 8

[Grace Bush]

Attachment: testing for migrated profiles/ changing themes

Comment 9

[Grace Bush]

Did some testing with migrated profiles - some going through activation process,
some not- see attached


from comment #3
Does this bug exist if the profile was migrated long ago,
or is it only with freshly migrated profiles?

I have an previously migrated profile (which I keep backed up) which was
activated and which was changed to classic theme 

Comment 10

[sairuh (rarely reading bugmail)]

i was able to repro this with a freshly migrated profile on win2k, but unable to
do so with an old migrated profile.

Comment 11

[Samir Gehani]

adt: nsbeta1+/adt2

Bug has morphed to cover windows now.

Comment 12

[Grace Bush]

updating summary

Comment 13

[shliang]

Attachment: patch

add defaults/profile/chrome to package list

Comment 14

[Sean Su]

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

this patch is necessary to get the chrome files packaged up.  It has a side
effect of fixing this bug, but if we ever stopped packaging this chrome dir,
the bug will come back.

r=ssu for this patch, but you should still try to find out what's causing this
bug.

ps. I just realized that you shoulnd't need to do the same patch to the ns tree
because the same moz packages- files will be run on the ns tree.

Comment 15

[jag (Peter Annema)]

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

sr=jag if you take the ns parts out (not needed).

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How does this patch fix the bug?  It looks like the only files in that directory
are examples.

Comment 17

[jag (Peter Annema)]

Note: it looks like the Mac install packager already packages up those two css
files (don't know how), but that's why we don't see this bug on Mac.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

packages-mac is defunct, and the Mach-O build just packages everything, IIRC.

Comment 19

[jag (Peter Annema)]

Ah, that would explain.

Comment 20

[shliang]

nsChromeRegistry::GetProfileRoot fails if the chrome directory doesn't exist in
the user profile directory and defaults/profile/chrome isn't in the install
directory

Comment 21

[jag (Peter Annema)]

So nsProfileDirSericeProvider::GetFile shouldn't call EnsureProfileFileExists
for sApp_UserChromeDirectory (since that's not what the caller expects), or the
caller should be fixed to assume the directory will exist after a successful
call to NS_GetSpecialDirectory (less robust, only valid if the installer
packages the defaults/profile/chrome directory, see patch), and should instead
check for the existence of these two files before copying them over.

Though it turns out these two files get copied over anyway, and not by this code.

Comment 22

[(not reading, please use [e-mail] instead)]

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

which package did these get added to?  

I'm thinking about the browser only scenario.  if these got added to [mail],
that would be a problem, right?

Comment 23

[(not reading, please use [e-mail] instead)]

theme switching needs to be fixed for 1.4 final, marking as a blocker.

cc asa, but I'm confident he will agree.

Comment 24

[(not reading, please use [e-mail] instead)]

shuehan tells me that she's added these lines to the "[deflenus]" section.

what about non en-US builds?

why not under "; chrome stuff"?

shuehan, can you discuss with ssu, who knows these packaging issues best?

Comment 25

[(not reading, please use [e-mail] instead)]

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

hold off on this.  comments coming.

Comment 26

[(not reading, please use [e-mail] instead)]

of course, I agree, we have to fix skin switching.

but maybe adding these two files back isn't what we want.

on a tip from samir/shuehan, I queried bugzilla and found that they were removed
on purpose.

see http://bugzilla.mozilla.org/show_bug.cgi?id=110555

instead of relying on the installer to copy over these files, why can't we fix
the code to either ensure that defaults/profile/chrome directory exists, or that
we handle it better when it doesn't?  (I vote for the latter).

shuehan wrote:

"nsChromeRegistry::GetProfileRoot fails if the chrome directory doesn't exist in
the user profile directory and defaults/profile/chrome isn't in the install
directory"

let's fix that instead

Comment 27

[(not reading, please use [e-mail] instead)]

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

no go on this.

Comment 28

[Sean Su]

Attachment: potential trivial quick fix

Comment 29

[Conrad Carlen (not reading bugmail)]

  All nsChromeRegistry::GetProfileRoot() is really required to do is to get a
file which points to the profile chrome dir and make a URL from that. It should
only fail if the directory itself does not exist after calling
NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR).
  The problem is that EnsureProfileFileExists() is being used here on a
directory. nsProfileDirSericeProvider::GetFile() will fail if the directory is
there but copying the defaults fails. Really, NS_APP_USER_CHROME_DIR should
refer only to the directory, not its contents, too. Then, we define two more
directory service props, for each of the two files. GetProfileRoot() could then
just call NS_GetSpecialDirectory for each of those, after succeeding on
NS_APP_USER_CHROME_DIR. It would also remove some bloat. See bug 37642, comment
57. That bug was back in the days before you could '-' a patch ;-)
  A simpler fix would be to have nsProfileDirSericeProvider::GetFile() not call
EnsureProfileFileExists() and make sure nsChromeRegistry::GetProfileRoot() can't
fail on copying the default files. 

Comment 30

[shliang]

Attachment: remove EnsureProfileFileExists()

remove EnsureProfileFileExists() from
NS_GetSpecialDirectory(NS_APP_USER_CHROME_DIR) so that chrome directory will be
created. i'm not sure any other changes are needed, the default css files don't
need to be there for skin switching to work.

Comment 31

[Conrad Carlen (not reading bugmail)]

Comment on attachment 124298 [details] [diff] [review]
remove EnsureProfileFileExists()

r=ccarlen. BTW, is the code in	nsChromeRegistry::GetProfileRoot() which copies
these files from the defaults still useful since those files  are no longer
present in the defaults/profile dir?

Comment 32

[shliang]

Attachment: also fix file names in GetProfileRoot

the code to copy the default files is still useful for non-installed builds -
just realized that wasn't working anyway though because the filenames are wrong
so changed those.

Comment 33

[Conrad Carlen (not reading bugmail)]

If you're going to touch GetProfileRoot(), how about this, while you're there:

defaultUserContentFile->AppendNative(NS_LITERAL_CSTRING("chrome"));
defaultUserContentFile->Clone(getter_AddRefs(defaultUserChromeFile));
defaultUserContentFile->AppendNative(NS_LITERAL_CSTRING("userContent-example.css"));
defaultUserChromeFile->AppendNative(NS_LITERAL_CSTRING("userChrome-example.css"));

Using Clone() allows you to get rid of the 2nd set of calls to
NS_GetSpecialDirectory() and a call to Append().
Also, there shouldn't be any "return rv" in that block - it's all optional.

r=ccarlen on what you have since the GetProfileRoot redundancy isn't yours.

Comment 34

[jag (Peter Annema)]

Comment on attachment 124469 [details] [diff] [review]
also fix file names in GetProfileRoot

sr=jag

Comment 35

[shliang]

checked in

Comment 36

[(not reading, please use [e-mail] instead)]

is this fixed on trunk and branch, or just trunk?

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 124469 [details] [diff] [review]
also fix file names in GetProfileRoot

conrad r='d this in a comment.

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

This has been approved for 1.4.  When it's checked into 1.4, please add the
keyword 'fixed1.4'
.

Comment 39

[Grace Bush]

on XP I am seeing this fixed on trunk (2003060305) but not branch (2003060305)
looks ok on Mac OS X branch

Comment 40

[Samir Gehani]

a=adt to land this fix on the 1.4 branch.  Please add the fixed1.4 keyword once
you land this.

Comment 41

[(not reading, please use [e-mail] instead)]

clearing my status whiteboard comment, the fix I rejected was not checked in.
(another fix was done.)

Comment 42

[Grace Bush]

verified on XP branch 2003060905
(and also on Mac/Linux branches/trunks)