99122 - Remove XPCOM dependency on string/obsolete

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P2)

Description

[Jon Smirl]

This is a first pass at removing XPCOM's dependency on the string/obsolete 
directory. I could fix everything except nsRegistry.cpp which needs UTF8 
conversions which are not available in the new string code.

This code compiles on Windows but I have not tried testing it yet. It's a 2,200 
line diff which is going to need to be carefully checked. There were also a few 
places where APIs needed to be changed to remove usage of the old strings. No 
attempt was made to check other platforms, they probably won't even compile.

95% of the changes were straight forward but a few were more complicated like 
Trim() which does not exist in the new string code.

Comment 1

[Jon Smirl]

Attachment: patches to remove string/obsolete dependency from XPCOM

Comment 2

[Doug Turner (:dougt)]

kool!  Lets ask Jag to take a look at this patch.  He is much more skilled with
our string classes than I am.

scott, jag, is there any way to do  UTF8 conversions without string/obsolete?

Comment 3

[jag (Peter Annema)]

Jon, could you attach a diff -u version of this patch for context and
readability?

Comment 4

[Jon Smirl]

Attachment: -u version

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm surprised that nsAFlatString doesn't have any pure virtual methods. 
However, I think it was still intended as an abstract type, whereas
nsDependentString and nsSharableString are not.

What is the goal of removing this dependency?  I think at some point in the
future |nsString| will have a new implementation (based on the current
nsSharableString?) that lives in string/public and string/src (although probably
with a bunch of methods removed / modified).

I also thought scc was rewriting the converters as generators a while ago...

scc?

Comment 6

[jag (Peter Annema)]

Collided with dbaron who said what I wanted to say, but here's my comment
anyway:

This won't work. Even though the code compiles, the |A| in nsAFlatString and
nsAString stands for abstract, meaning that you can't directly use the class.
What's lacking in this case is a mechanism for allocating memory when assigning
into it.

You should be able to use nsSharableString for this purpose though.

Comment 7

[Jon Smirl]

Does nsSharableString incur a locking/refcount overhead? I can switch if this 
isn't going to be a performance issue. If I remember the changes right a global 
replace should work.

I just had a bad experience with Xalan doing multi-thread locks on strings and 
ruining performance.

I wish there was better documentation for the string code. I find it entirely 
confusing as to what to use.  I've got another entire project that can't even 
compile because of string changes.

Comment 8

[Jon Smirl]

Attachment: -u with Flat changed to Sharable

Comment 9

[Jon Smirl]

Attachment: Made it build debug/non-debug on Windows

Comment 10

[jag (Peter Annema)]

Let me first comment that ns[C][Auto]String are not obsolete, even though they
currently are in that directory. They will become versions of
ns[C]SharableString. The changes you are making don't really make sense unless
there's an urgent need to get rid of some build dependency that can't wait till
the strings have been cleaned up.

I also see that in a lot of places you have converted nsString& parameters to
nsAString& parameters. While this is laudable, is it what we really want? In
some cases we might want nsAFlatString& for performance reasons (where we know
the string will be flat, i.e. single fragment, zero-terminated, and most or all
usage of that code will be from flat strings, and we can hand flatten the few
non flat strings).

Some comments on your patch as it is:

+    nsSharableCString str; 
+       CopyUCS2toASCII(nsDependentString(message), str);

Fix your editor to not insert hard tabs.

+          status = (*_retval = nsCRT::strdup((*entry).get())) ? NS_OK :
NS_ERROR_OUT_OF_MEMORY;

Perhaps you could use ToNewCString(*entry) there.

-    return AddSubtree( baseKey, NS_ConvertUCS2toUTF8(keyname).get(), _retval );
+       nsSharableCString name;
+       CopyUCS2toASCII(nsDependentString(keyname), name);

This is not the same thing. There is no way to currently do what you want.

-    nsAutoString tmp;
+    nsSharableString tmp;

This is not the same thing. nsAutoString provides a 64 unit buffer on the stack,
which generally makes it cheaper than an nsString for use with short strings.
nsString will always need to allocate memory, where nsAutoString will only need
to allocate when it needs more than 64 units.

-[ref] native nsStringRef(nsAWritableString);
+[ref] native nsStringRef(nsAString);

Hmmm... The other part of this is:

[noscript] void ToString(in nsStringRef aString); 
 
Do you think people would mind if that were changed to:

AString ToString();


------------


+                       for (int i = key.Length(); i > 0; i++) {
+                               PRUnichar ch = key.CharAt(i);
+                               if ((ch == L' ') || (ch == L'\t'))
+                                       key.Truncate(i - 1);
+                               else
+                                       break;
+                       }

I don't think L'x' is portable, use PRUnichar('x');
That should be i--, not i++, and CharAt is 0 based, so you'll not get the char
you want there.

How about something like:
PRInt32 i;
for (i = key.Length() - 1; i >= 0; --i) {
  PRUnichar ch = key.CharAt(i);
  if (ch != PRUnichar(' ') && ch != PRUnichar('\t'))
    break;
}
if (i != key.Length() - 1)
  key.Truncate(i);

---

and then a bit later, you do:


-      value.Trim(trimThese, PR_TRUE, PR_TRUE);
-      nsAutoString oldValue;
+                       for (i = value.Length(); i > 0; i++) {
+                               PRUnichar ch = value.CharAt(i);
+                               if ((ch == L' ') || (ch == L'\t'))
+                                       value.Truncate(i - 1);
+                               else
+                                       break;
+                       }
+      nsSharableString oldValue;
       mSubclass->SetStringProperty(key, value, oldValue);

The same mistakes as above, and this code was trimming from both ends.
This should work:

nsAString::const_iterator begin, end;
key.BeginReading(begin);
key.EndReading(end);
while (begin != end && (*begin == ' ' || *begin == '\t'))
  ++begin;

while (end != begin) {
  --end;
  if (*end != ' ' && *end != '\t')
    break;
}

nsSharableString oldValue;
mSubclass->SetStringProperty(key, Substring(begin, end), oldValue);

Btw, something similar to the above while loop could also be used for the first
case.

        PRUnichar test[]={0x0041,0x0051,0x0052,0x0000};
-       nsString T(test);
+       nsAString T(test);

This code should really fail to compile, since again nsAString is abstract and
should not be instantiated. Use |nsDependentString T(test);|. You use this
replacement pattern in a few places, btw, which will all need fixing.

Comment 11

[Doug Turner (:dougt)]

alec is doing some work here.  Alec, care to own this one too.  

Comment 12

[Jon Smirl]

Who responsibile for finishing sorting out string vs string/obsolete? That work 
has to be finished before XPCOM can be adjusted to not use string/obsolete.

For example autostring is in string/obsolete when it should be in string. 
USC support is incomplete in string
The classes in string are missing some commonly needed helper methods.
The documentation for string is partial and needs a lot of help.
.....

Comment 13

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 14

[Alec Flett]

not sure this is ever going to happen. string/obsolete isn't really obsolete -
its just the internal implementations of nsString and friends that need to be
cleaned up. marking WONTFIX.