Ok, dougt's work on nsIProperty inspired me to go fix up nsIPersistentProperties. here's what I've changed: 1) fixed up enumerator stuff to just use nsISimpleEnumerator 2) switched over to AString/AUTF8String and friends 3) made it scriptable 4) made the interface's "key" parameter be a UTF8 string, because 99 times out of 100, the key is actually ASCII. This was actually implemented at a lower level a while back, I'm just pushing it out further to the interface level now. This also will pave the way for us converting the "key" parameter of the string bundle's GetStringFromName to use a UTF8-based key...but that's another bug. patch forthcoming.

and here's the patch. darin/doug, you want to review?

oh, and in the patch I #if 0'ed out EnumerateProperties but in my tree I've now removed it completely. as you can see from the patch, this removes a bunch of WithConversion stuff, nsXPIDLStrings, casting, and all sorts of other goodness.

Comment on attachment 96654 [details] [diff] [review] clean up interface and consumers I am not so sure about using AUTF8String's here since the nsIProperties uses plain string. Maybe we should stay consistent between the nsIPersistentProperties and nsIProperties? Maybe we should be using the nsISerializable.idl interface here? Just some thoughts. I am okay with either way we go since this interfaces is not intended for public consumption yet.

can you add some doxygen comments to this: +interface nsIPersistentProperties : nsIProperties +{ + void load(in nsIInputStream input); + void save(in nsIOutputStream output, in AUTF8String header); + void subclass(in nsIPersistentProperties superclass); ... i'm especially curious about subclass although i can guess.

Comment on attachment 96654 [details] [diff] [review] clean up interface and consumers >Index: xpcom/ds/nsPersistentProperties.cpp > nsAutoString key; > while ((c >= 0) && (c != '=') && (c != ':')) { >- key.Append((PRUnichar) c); >+ key.Append(PRUnichar(c)); > c = Read(); >+ mSubclass->SetStringProperty(NS_ConvertUCS2toUTF8(key), value, oldValue); Too bad you can't read the file as UTF-8, and then expand only the values. As is, we're expanding the whole file and then compressing just the keys. >Index: gfx/src/windows/nsFontMetricsWin.cpp >+ nsCAutoString name(NS_LITERAL_CSTRING("encoding.")); >+ name += aFontName; >+ name += NS_LITERAL_CSTRING(".ttf"); nit: use operator+() instead. >Index: layout/mathml/content/src/nsMathMLOperators.cpp > SetOperator(OperatorData* aOperatorData, > nsOperatorFlags aForm, >+ nsCString aOperator, > nsString aAttributes) passing string objects by value?? that can't be good. >+ nsCAutoString key(NS_LITERAL_CSTRING("mathvariant.")); >+ key.Append(kMathVariant_name[i]); another case for operator+(). overall, the patch looks great... sr=darin

doug: I'd like to leave it as UTF8 for the time being because nsIPersistentProperties is the meat of string bundles, which are used everywhere. My concern is that there are a number of places in the code, in both string bundles and elsewhere, where we retrieve UCS2-encoded values from bundles or properties, and then try to reuse the value as a key back into nsIPersistentProperties. I'm 99% certain that they are all just ASCII values, but I don't think its worth the risk at this point. Of course, difference between AUTF8String and ACString is only an xpconnect issue at this point though - they both become nsACString in C++ anyway, and up until now this interface wasn't even scriptable. So can I get a r=dougt, and then I can cover the UTF8 issue when I clean up string bundles. darin: nice catch on the objects-by-value. As for inflating and then deflating, I agree its pretty lame. It was an unfortunate side effect of bug 153754, which had the secondary effect of storing keys in UTF8. I'd like to move this whole interface over to UTF8 but I think that would have pretty negative impacts on some asian character sets.

Comment on attachment 96654 [details] [diff] [review] clean up interface and consumers r=dougt

i think the property values need to remain as UCS-2 for performance reasons, but the way in which the data is read from disk does not necessarily have to go directly to UCS-2 before getting parsed. anyways, not a big deal.

fixed.