164575 - make nsIPersistentProperties sane

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Alec Flett]

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.

Comment 1

[Alec Flett]

Attachment: clean up interface and consumers

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

Comment 2

[Alec Flett]

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 3

[Doug Turner (:dougt)]

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.

Comment 4

[timeless]

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 5

[Darin Fisher]

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

Comment 6

[Alec Flett]

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 7

[Doug Turner (:dougt)]

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

r=dougt

Comment 8

[Darin Fisher]

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.

Comment 9

[Alec Flett]

fixed.