117685 - nsChomeRegistry::isSkinSelected not returning correct result

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Blake Ross]

isSkinSelected is not returning the correct result when you're in Classic and 
pass in modern/1.0.  It returns true.  I haven't yet tested in a commercial 
build to see if it's related to the default skin.  Works ok in all other cases.

Comment 1

[Blake Ross]

The problem would appear to be that in my profile's chrome datasource, the 
selectedSkin arc for venkman and chatzilla is urn:mozilla:skin:modern/1.0 
instead of classic/1.0.  Indeed, when I manually change those to be classic, 
this works.  I don't quite see why venkman and chatzilla have modern.  Hyatt, 
how do I fix this?

(This is not just my profile. Others on irc tested, and it happens with a new 
profile.)

Comment 2

[Blake Ross]

-> rginda

He's using the same files for both skins; needs to split them up.

Comment 3

[Robert Ginda]

I'm not going to ship two skins.  There is no reason for that.  Both skins are
the same, and are likely to say the same for the foreseeable future.  Is there
really some bug out there that requires that I duplicate my skin files for
Venkman, Chatzilla, and the Compont Viewer (don't forget about that one now)? 
Is every app installed going to have to have two ship with two skins?

What is this bug about?  The summary doesn't mean anything to me.

You're going to have to convince me that there is some reason I have to suddenly
start maintaining two skins before I accept this bug.  "That's the way it is",
and "every other app does it that way" arent going to cut it.  Pointing me to
bug 44032 isn't either.  What does bug 44032 have to do with this, anyway?

Comment 4

[Blake Ross]

I spent 20 minutes on irc explaining the bug to you, no need to ask "what is
this bug about" in public.  Bug 44032 is an important usability bug. Your apps
are preventing the fix because it's making doing what Todd said in
http://bugzilla.mozilla.org/show_bug.cgi?id=44032#c17 impossible.  I suggested
you read the bug, I guess you didn't do that. It's irrelevant that you don't
consider that bug important (it is); your apps are breaking the isSkinSelected
api everywhere it's currently used and everywhere it will be used in the future.

I already explained all of this, offered to split the skin files up for you,
expressed desire for a better solution, and said I'd talk to hyatt about
possibilities.  Stop grandstanding in bugs and put your shields down so we can
figure it out.

Comment 5

[Hixie (not reading bugmail)]

One solution might be to just copy the file twice during the build process. I
don't know if that is a good solution or not.

Comment 6

[Robert Ginda]

Grandstanding?  Come off it Blake.

You didn't explain *why* this is a problem, and you didn't answer my questions,
on IRC, or here.  You said something along the lines of "it blocks 44032".  I'd
like to know *why* having only one skin blocks the bug.  What is the problem? 
Point me to some code.

I'm sorry you are taking this personal, it is not.  I object, in principal, to
having to ship two skins.  The two skins are *exactly the same*.  Even if I had
plans to make them different in the future, they are the same now.

Pehaps you don't understand me fully.  I do not object to doing the work due to
lack of time on my part.  Your offer to "do it for me" is of no consequence. 
Similarly, your offer to do it "one way or another" is not well recieved.  I
object to the requirement that all apps ship with two skins, and am only
interested in *why* this is suddenly a requirement.

You're not asking me to change some small thing here, these application have
survived with only one skin for quite some time now.  As I recall it, the idea
has come up before, and we have found other solutions.  I expect we can do the
same with this situation.

Comment 7

[Blake Ross]

I understand and agree with you that it's not an ideal solution.  I am not
pushing for you to do this anymore, as I said, I want to see what else can be
done. I'm sure this is all unnecessary, hyatt probably has the answer if he'd
get his lazy ass into work ;) Hixie's idea also sounds reasonable.

Comment 8

[Blake Ross]

Sorry, and to clarify, my "one way or another" comment referred to bug 44032. 
As in, whatever course of action we decide here, that has to be fixed at some point.

Comment 9

[David Hyatt]

Seems like the easiest thing to do is to rename ChatZilla's skin to something
else, besides Modern, i.e., "rgindaskin".  Technically you're skin-independent,
so you really shouldn't be claiming to be Modern (which implies you work with
Modern but not with Classic).

Comment 10

[Robert Ginda]

I vaguely recall there being some reason I had to specify a valid skin name.  Is
this not the case?

Comment 11

[David Hyatt]

You should be able to make up your own skin name.

Comment 12

[Robert Ginda]

Attachment: patch (not for check in)

The attached patch makes up it's own skin name for chatzilla.  If you apply
this patch and check Edit->Prefs->Appnc->Themes, you'll now be offered the
option to apply "ChatZilla Default Theme" to Mozilla (*not* a good thing.)

Is there a way to opt out of the theme selection dialog?

Comment 13

[basic]

how about moving chatzilla's "skin" to content?

Comment 14

[David Hyatt]

Why does this even matter?  It is a perfectly valid state to have multiple skins
partially selected at the same time.  Blake, could you explain why you need to
have Modern be fully unselected when you're in Classic?

Comment 15

[Blake Ross]

Because then we'll keep reapplying the theme even if the user doesn't change it
when he goes to the prefs.  How many users are going to have different themes
for different apps...?

Comment 16

[David Hyatt]

What you really need is for isSkinSelected to indicate 3 possible values: full,
partial, or none, and then you can treat partial and none as the same.  Would
that work for you?

Comment 17

[Blake Ross]

Works for me!

Comment 18

[Blake Ross]

I take that back, that won't work.  After all, most people are still going to 
be in the "partial" category.  So we're still going to select the wrong theme 
when we select that panel, and so we'll apply the wrong theme, etc.

However, I have a patch which fixes the problem in this case.  IsSkinSelected 
was returning true for modern when in Classic because we'd hit venkman, 
selectedSkin would be true, so we'd return true.  The fix is to change it so 
that we bail the first time the selected skin _isn't_ the skin passed in.  This 
will work because venkman and chatzilla aren't in the packages list for classic 
(see their contents.rdf files).

See the results below to understand better.  The first section is when passing 
in modern, the second is passing in classic, while in the classic skin:

---
selected for package urn:mozilla:package:inspector: 0
selected for package urn:mozilla:package:chatzilla: 1
selected for package urn:mozilla:package:venkman: 1
selected for package urn:mozilla:package:communicator: 0
selected for package urn:mozilla:package:editor: 0
selected for package urn:mozilla:package:global: 0
selected for package urn:mozilla:package:messenger: 0
selected for package urn:mozilla:package:navigator: 0
---
selected for package urn:mozilla:package:inspector: 1
selected for package urn:mozilla:package:communicator: 1
selected for package urn:mozilla:package:editor: 1
selected for package urn:mozilla:package:global: 1
selected for package urn:mozilla:package:messenger: 1
selected for package urn:mozilla:package:navigator: 1

Comment 19

[Blake Ross]

Attachment: patch for debugging

Comment 20

[Blake Ross]

Attachment: patch

Comment 21

[John Morrison]

Please don't remove the "general.skins.selectedSkin" hack (as your patch does).
Or, if you're going to remove it, give mcafee and I some time to make some 
changes on the various tinderbox tests, since we use that to force the build 
into the modern skin. Last I heard from hyatt, he said that he was just 
planning to leave this in place, though.

Comment 22

[Blake Ross]

Attachment: patch, improved

doesn't remove the pref (hyatt also spanked me for doing that)

Comment 23

[David Hyatt]

sr=hyatt

Comment 24

[Blake Ross]

stealing.

Comment 25

[Blake Ross]

fixed.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 27

[Michael Gabriel]

on current trunk 2003011713 when going to prefs -> appearance -> themes im getting

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004003 (NS_ERROR_INVALID_POINTER) [nsIXULChromeRegistry.isSkinSelected]" 
nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
chrome://communicator/content/pref/pref-themes.js :: Startup :: line 76"  data: no]

in any way related ?