simon sees this on a recent mac debug build from today --and it occurs regardless of theme, and on both an exisiting/migrated profile and a new profile. he cannot repro this on a recent win32 build, however. i'm currently using a 4/25 comm verif builds for some other testing, so won't be able to test this till tomorrow's verif bits are out.

note to self: also check linux builds, to see if this is really mac only.

Maybe color prefs got broken by bnesse?

I see this in a current linux build too.

Link colors also changed in the last day or so, and now look lighter, and somewhat washed-out. What changed?

It's most likely mine...

Ugh. This is a result of the decision to remove the "color pref" methods. Even though GetColorDWordPref returned an uint, it was fetching a char string and sprinting into the uint. There's a couple of ways we could quick fix this, but what we should do is determine how we really want color prefs handled. I believe conrad and possibly valeski had thoughts on this. cc'ing them.

Text and Background color prefs are broken as well. tested on winNT [2001.04.27.09] and linux [2001.04.27.05].

I think we should have a color pref accessor. We used to and it wrote the color in the #RGB format which is nice and XP. Color prefs were changed to just use an int and there is no migration from the old format to the new, hence the breakage. I'd stick with the previous #RGB string format so we can still read existing pref files and add a color pref accessor.

I'm not opposed to conrad's suggestion, I just recall being told that it should go away. If we add it back in, do we go with the GetColorPref() version which returns r, g, and b parameters or the GetColorDWordPref version which returns an NS_RGB format value?

How many users of GetColorDWordPref did there used to be? Couldn't we end up with copies of MakeColorPref all over the place? I'd think that the RGB color is a pretty common pref type. As far as whether it's as one ARGB long or r, g, and b params, hmm. The 2nd, while a little more laborious to use, is more clear and less dependent on whether it's ARGB, RGBA, or whatever.

nsPresContect is the only consumer who was using it. And it only uses the Getter, not the Setter. If indeed this functionality is desirable, it certainly belongs in prefs to avoid just the situation you describe. It could be implemented as a complex value instead of as a standalone method if that's more desirable. We could even return an object that could contain the information in multiple formats I suppose.

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

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

patch looks reasonable to me - as I recall, nsPresContext was the only consumer of color prefs. Another option is to use a ComplexPrefs system, but I think that's overkill here. sr=alecf on the patch

True, nsPresContext is the only consumer of color prefs in our code, but embeddors need to access color prefs in order to get/set them with their own UI. I know that they were unclear as to how to do this - what the format was, whether they had to convert it to a string and use SetCharPref, etc. If there was such a thing as an nsISupportsColor and they could use it with get/setComplexPref, it might be easier for them. That might be good at some point - nothing against the current patch.

we could make a nsIColor interface (or maybe there's one already?) interface nsIColor { attribute short red; attribute short green; attribute short blue; }

Do we really want to force heap allocations and refCounting just to track colors?

nope, I think it's overkill...consumers are already dealing with heap allocations however, since they were using strings. quite frankly, I think they should just be using 32-bit signed long integers, but NOO.. we had to come up with some dorky sscanf trick. Here's a thought...what if we fixed pref migration to switch the strings into integer values, thus changing the pref type. old consumers (i.e. the ones addressed in this patch) should handle both types in the function that bnesse created... then we can tell people (embedders) that they can add pref defaults by saying user_pref("some.color.pref", 0xff00ff); to define their colors, and use GetIntPref() to retrieve them.

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

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

> user_pref("some.color.pref", 0xff00ff); That would reduce the human-readability of prefs.js, which -- especially because of the number of GUI-less prefs we have -- I think is kinda important. (Cf. the lengthy discussions in other bugs about the names for some prefs, when the names only matter to humans.) Feel free to ignore me if you think I'm being too trivial ...

you're being too trivial. this is prefs.js. it's a internal file. if you're mucking with it then you better understand enough to know that 0x<numbers> is a hex number.

i would personally be fine with a solution involving prefs.js. my impression is that the average user would not be mucking around in there anyway. something informative like user_pref(browser.document.foreground,"#ffffff"); would be fine for probably anyone who knows html. 0xffffff is of course the standard, but if you're worrying about the users... i would think that anyone who doesn't even know basic html would probably not be manually editing prefs anyway. hasn't been any activity here in a few days. what's up? =) it's getting somewhat tedious having to select text in mail when i want to read it... ;-)

personally, I'm fine w/ hex values for colors. the more convenient we can make the values for colors the better, but allocating new mem should be avoided.

It seems that, though we may not know exactly how we want to support color preferences, we don't want to create a color object. The current scheme is essentially converting a integer hex value into a string value and back (behind the scenes), which in my mind isn't really a gain in any way. The question is, should we go with the patch as it stands (I'll add some comments to it in way of explaination) or should we, as Alec suggested, change prefs migration to convert it to an integer value? If we go with the second option it would probably make sense to rewrite the patch to support both string and int types (or to convert strings to ints) as we would have backward compatibility issues with old profiles.

I think we should check in the patch as is, leaving this bug open.. then we come up with a second patch which: - supports both strings and integers - fixes the migration code to change the strings to integers when migrating a 4.x profile - change the defaults in all.js to use integers

Ok, in that case, can I get an r= from someone?

+static void MakeColorPref(const char *colstr, nscolor *colorref); You don't need a prototype for a routine that is declared |static|. Also, since an nscolor is just a PRUint32, it would be clearer if MakeColorPref just returned an nscolor. With those changes, r=sfraser

Ah yes, I returned in the param because my original implementation returned a nsresult. I'll change it.

r/sr=alecf

Fix checked in. Leaving bug open as per Alec's suggestion but downgrading severity and clearing keywords.

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

Closing this bug as per my managers request. Bug 79724 has been opened to track any additional work.

while trying to vrfy this, i encountered bug 81553. however, i can change the colors, so am marking this one verified [we can track other issues in other bugs]. winnt - 2001.05.17.10 mac and linux - 2001.05.17.08