141090 - New profiles do not display current theme w/ check flag

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jerry]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    20020425

according to bug 127993, theme has one checked flag when mozilla startup.
but if start mozilla with new profile, theme doesnot have it.


Reproducible: Always
Steps to Reproduce:
1.startup mozilla with create new profile
2.chose the new created profile
3.View->Apply Theme


Actual Results:  No checked flag for the current theme

Expected Results:  There is one check flag for the current theme

if select one theme, restart mozilla, 
View->Apply Theme, you will see that there is a checked flag for the current
used theme.

Comment 1

[R.K.Aa.]

related: bug 54681

Comment 2

[Jerry]

I dont agree.
this bug is not dup of 54681.
this bug only happens when new profile is created.

if using existed profile, it is ok.

Comment 3

[Jerry]

take it

Comment 4

[Jerry]

Attachment: patch

Comment 5

[Jerry]

Attachment: patch (correct some format error)

Comment 6

[Jerry]

Comment on attachment 133551 [details] [diff] [review]
patch (correct some format error)

bryner, can you give r?

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 133551 [details] [diff] [review]
patch (correct some format error)

After talking with bsmedberg, I think a better solution is to fix
isSkinSelected() to always work correctly.

Comment 8

[Jerry]

Attachment: patch 0.2 (fix in nsChromeRegistry::LoadProfileDataSource)

when mozilla start with new profile, the new created prefs doesnot contain
selected skin info.

but for nsChromeRegistry::LoadProfileDataSource, it will check user prefs, 
if it contain selected skin info, mozilla will selectSkin for all package,
if not, it will just skip.

so patch 0.2 hack here, if prefs doesnot contain selected skin info, it will
query "global"'s skin, and selectSkin for all packages.

Comment 9

[Jerry]

Attachment: patch 0.3 (hacking in nsProfile and nsChromeRegistry)

it goes in another way.

when mozilla create new profile, it will create chrome.rdf for the new profile,

it contains locale info, but lack skin info.

so I make patch 0.3, just handle skin like locale, 
when new profile, add selected skin info into chrome.rdf.

Comment 10

[Jerry]

bryner and blaker, can you take a look at patch 0.2 and patch 0.3?

I prefer patch 0.3.
but for a existed profile, if user never use View->Apply Theme to set theme,
there will be still no check flag for all availble themes.

and I dont want to hack in nsChromeRegistry:IsSkinSelected(), 
because it can return FULL, partical, NONE, 
it could be used in the future.

Comment 11

[Benjamin Smedberg]

From quick perusal, I think version 2 is better (and we should probably be doing
something similar for locale). The 'birds are moving towards
--enable-single-profile, which means that none of the code in nsProfile will be
present. That might be a red herring, though.

Comment 12

[Jerry]

Comment on attachment 134467 [details] [diff] [review]
patch 0.2 (fix in nsChromeRegistry::LoadProfileDataSource)

blake, can you review it?

Comment 13

[Jerry]

Comment on attachment 134467 [details] [diff] [review]
patch 0.2 (fix in nsChromeRegistry::LoadProfileDataSource)

bryner, can you review it?

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 134467 [details] [diff] [review]
patch 0.2 (fix in nsChromeRegistry::LoadProfileDataSource)

I like patch 0.3 better... this one will impact startup time by calling
SelectSkin on every startup.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 134473 [details] [diff] [review]
patch 0.3 (hacking in nsProfile and nsChromeRegistry)

r/sr=bryner for this patch.

Comment 16

[Benjamin Smedberg]

Comment on attachment 134473 [details] [diff] [review]
patch 0.3 (hacking in nsProfile and nsChromeRegistry)

If you alter an interface you *must* change the IID of the interface: generate
a new IID and insert it into the nsIXULChromeRegistry interface and r=me

Please don't check this in yet... I need to move the nsIChromeRegistry.idl to
content/base/public, so I will check this in once that is done. (see bug
219233)

Comment 17

[Kyle Yuan]

(In reply to comment #16)
> Please don't check this in yet... I need to move the nsIChromeRegistry.idl to
> content/base/public, so I will check this in once that is done. (see bug
> 219233)
> 

bug 219233 has been fixed, have you checked this in?

Comment 18

[benc]

adding depends per bsmedberg, flagging for 1.7f, allplats

I verified the previous bug, which fixes the problem in everything except the
default state of a newly created profile.


I'd really like to see this fixed for 1.7.

Comment 19

[Benjamin Smedberg]

Comment on attachment 134473 [details] [diff] [review]
patch 0.3 (hacking in nsProfile and nsChromeRegistry)

I'm vaguely worried about the possible regressions here, but with several
release candidates I think this should probably go in.

Comment 20

[benc]

Jerry: just so I understand...

This patch has the effect of forcing new profiles to have the same theme
selection state that they get after you select a theme via the menu?

I have seen some theme weirdness in a new Mac profile that goes away after you
re-select the theme...

Comment 21

[Asa Dotzler [:asa]]

bsmedberg, if we're going to consider this for 1.7, can you help us identify the
things that might be impacted -- what areas of the app we should pay particular
testing attention to after it lands? Has it landed on the trunk yet? Any fallout
there?

Comment 22

[benc]

Jerry: Is this patch bitrotted? I had someone try to build w/ the patch today,
and they said that the idl file had moved.

Comment 23

[Jerry]

the patch does not in trunk.

Due to patch for  bug 219233
nsIChromeRegistry.idl has been moved into content, 

I think just modify the patch 0.3, set nsIChromeRegistry.idl to the currently
place, the patch still work.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 134473 [details] [diff] [review]
patch 0.3 (hacking in nsProfile and nsChromeRegistry)

a=asa (on behalf of drivers) for checkin to 1.7

Comment 25

[Benjamin Smedberg]

Checked in on trunk and 1.7 branch.

The kinds of regressions we're looking for are:
1) If you install a non-default theme, and use the profile manager, the correct
theme is applied
2) If you uninstall a theme, it reverts to some other theme sanely.
3) Other similar theme+profile combinations.

Comment 26

[benc]

Thanks.