Closed Bug 190174 Opened 17 years ago Closed 17 years ago

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

Categories

(Core Graveyard :: Skinability, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4final

People

(Reporter: nbaca, Assigned: shliang)

References

Details

(Keywords: access, regression, Whiteboard: [adt2])

Attachments

(4 files, 1 obsolete file)

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.
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.
Assignee: andreww → varga
Keywords: nsbeta1
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.
Keywords: access
> 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.
Depends on: 190336
QA Contact: pmac → gbush
Nav triage team: nsbeta1-
Keywords: nsbeta1nsbeta1-
*** Bug 202816 has been marked as a duplicate of this bug. ***
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.
Keywords: nsbeta1-nsbeta1, regression
OS: MacOS X → Windows XP
Hardware: Macintosh → PC
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 
i was able to repro this with a freshly migrated profile on win2k, but unable to
do so with an old migrated profile.
adt: nsbeta1+/adt2

Bug has morphed to cover windows now.
Assignee: varga → shliang
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2]
updating summary
Summary: Migrate profile, unable to switch from Classic to Modern → Migrate profile, unable to switch from Classic to Modern or Modern to Classic
Attached patch patch (obsolete) — Splinter Review
add defaults/profile/chrome to package list
Attachment #123940 - Flags: review?(ssu)
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.
Attachment #123940 - Flags: review?(ssu) → review+
Comment on attachment 123940 [details] [diff] [review]
patch

sr=jag if you take the ns parts out (not needed).
Attachment #123940 - Flags: superreview+
Attachment #123940 - Flags: approval1.4?
How does this patch fix the bug?  It looks like the only files in that directory
are examples.
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.
packages-mac is defunct, and the Mach-O build just packages everything, IIRC.
Ah, that would explain.
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
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 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?
theme switching needs to be fixed for 1.4 final, marking as a blocker.

cc asa, but I'm confident he will agree.
Flags: blocking1.4+
Target Milestone: --- → mozilla1.4final
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 on attachment 123940 [details] [diff] [review]
patch

hold off on this.  comments coming.
Attachment #123940 - Flags: approval1.4?
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
Whiteboard: [adt2] → [adt2] [need new fix]
Comment on attachment 123940 [details] [diff] [review]
patch

no go on this.
Attachment #123940 - Attachment is obsolete: true
  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. 
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 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?
Attachment #124298 - Flags: review+
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.
Attachment #124469 - Flags: superreview?(jaggernaut)
Attachment #124469 - Flags: review?(ccarlen)
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 on attachment 124469 [details] [diff] [review]
also fix file names in GetProfileRoot

sr=jag
Attachment #124469 - Flags: superreview?(jaggernaut) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
is this fixed on trunk and branch, or just trunk?
Attachment #124469 - Flags: approval1.4?
Comment on attachment 124469 [details] [diff] [review]
also fix file names in GetProfileRoot

conrad r='d this in a comment.
Attachment #124469 - Flags: review?(ccarlen) → review+
Attachment #124469 - Flags: approval1.4? → approval1.4+
This has been approved for 1.4.  When it's checked into 1.4, please add the
keyword 'fixed1.4'
.
on XP I am seeing this fixed on trunk (2003060305) but not branch (2003060305)
looks ok on Mac OS X branch
a=adt to land this fix on the 1.4 branch.  Please add the fixed1.4 keyword once
you land this.
Keywords: fixed1.4
clearing my status whiteboard comment, the fix I rejected was not checked in.
(another fix was done.)
Whiteboard: [adt2] [need new fix] → [adt2]
verified on XP branch 2003060905
(and also on Mac/Linux branches/trunks)
Status: RESOLVED → VERIFIED
Keywords: fixed1.4verified1.4
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.