63923 - [API] Fix NS_LITERAL_STRING

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Peter "jag" Annema]

NS_(NAMED_)LITERAL_STRING currently is defined as either a nsLiteralString
(nsAReadableString) or a NS_ConvertASCIItoUCS2 (nsAutoString), depending on
whether your platform supports short wchar or not (in that order).

The problem here is that code which compiles fine on a system which doesn't
support short wchar, may not compile on systems which do, because "suddenly" a
nsAReadableString is passed into something which was expecting a ns(Auto)String.

To prevent people being bitten by this (yes, they should know better, but it's
an easy mistake, and needlessly wastes time) NS_(NAMED_LITERAL_STRING should
cast the NS_ConvertASCIItoUCS2 to a nsAReadableString.

Patch will follow.

Comment 1

[Peter "jag" Annema]

Patch will take a little longer than I expected. I overlooked the fact that
nsAReadableString doesn't have a |.get()| (nor can have one, it's not guaranteed
to be flat). I don't immediately see a way around this, other than adapting
nsLiteralString to interally do something like ASCIItoUCS2 (in which case it
will probably need to be untemplatized), or add a new class which does for
nsAReadableString what NS_ConvertASCIItoUCS2 does for nsAutoString.

I hope someone else sees a better way :-)

Comment 2

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

Can you put a simple, entirely inlined wrapper class (inheriting from
nsAReadableString) around NS_ConvertASCIItoUCS2?

Comment 3

[Scott Collins]

Yes, as part of the string release work, I'll make sure this casts appropriately
to a common base class so the same behavior is statically expected on all platforms.

Comment 4

[Scott Collins]

want to base the solution to this on the flat abstract class that will be
inserted into the hierarchy in 0.9.  So, moving this out as a dependency.

Comment 5

[Scott Collins]

(mass change) didn't get these in for target milestone mozilla0.8.1 but they are
very close.  Moving all to mozilla0.9.

Comment 6

[Scott Collins]

this didn't get fixed in the branch

Comment 7

[Scott Collins]

These are the 0.9 bugs that didn't make it, and that I intend to fix in 0.9.1

Comment 8

[Scott Collins]

re-targeting milestones, starting from a clean slate

Comment 9

[Scott Collins]

All planned string API fixes need to be in by mozilla1.0.

Comment 10

[Scott Collins]

Attachment: first attempt at unifying the type yielded by the literal macros

Comment 11

[Scott Collins]

Attachment: previous patch + fix callers impacted by the change

Comment 12

[Scott Collins]

This latest patch builds, but there's trouble when you run ... so we're not
ready for prime time yet.  A look in the patch, though, shows where some really
funky stuff was already going on, particularly in history.

Comment 13

[Scott Collins]

I think the original patch is basically sound ... but only for
|NS_LITERAL_STRING|.  The same fixes in the rest of the tree (from the second
patch: 05/16/01 05:57) still apply.  I'll attach a new patch anon.

Comment 14

[Scott Collins]

Someone (me?, hewitt?) probably needs to fix bug #82208 for this to work.  See
the patch attached there:

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35647

Comment 15

[Scott Collins]

Attachment: just fix the problematic definition (and bad caller in "nsGlobalHistory.cpp")

Comment 16

[Scott Collins]

hmmm, I think previous patches (and this latest one) suffer from object
`slicing', two possible fixes are (1) cast to a reference, or else (2) always
produce an |nsDependentString|, in the hard case, over the buffer of the
|NS_ConvertASCIItoUCS2|

Comment 17

[Scott Collins]

in any case, we're only going to fix this for the common case; uses of the
`named' version are much less frequent

Comment 18

[Scott Collins]

Attachment: ...and try not to slice

Comment 19

[Scott Collins]

I don't think suggestion (2) above is possible without dangling; testing my new
patch now, then I think I'm ready for review and super-review.

Comment 20

[Simon Fraser [no longer active]]

The nsGlobalHistory changes scare me. In that file, I see (with the patch)

  nsString temp = new nsString;
  *temp = NS_LITERAL_STRING("http://www.");
  mIgnorePrefixes->ReplaceElementAt(NS_STATIC_CAST(void*, temp), 0);

and

  for(PRInt32 i = 0; i < mIgnorePrefixes->Count(); ++i) {
    nsDependentString* entry = NS_STATIC_CAST(nsDependentString*, 
mIgnorePrefixes->ElementAt(i));
    delete entry;
  }

and
  for (PRInt32 i = 0; i < mIgnorePrefixes->Count(); ++i) {
    nsString* string = (nsString*) mIgnorePrefixes->ElementAt(i);    

Can we be consistent about what's in mIgnorePrefixes? Why use nsDependentString 
in the second case?

Also, | mIgnorePrefixes| itself is leaked.

There must be a better way to store a list of ignored prefixes, like a static 
const array of char[].

Comment 21

[Scott Collins]

See bug #82208.

Comment 22

[Scott Collins]

the non-slicing version of the patch builds and runs, now I just need to address
sfraser's concerns re "nsGlobalHistory.cpp"

Comment 23

[Simon Fraser [no longer active]]

sr=sfraser on the nsLiteralString.h change.

Comment 24

[Scott Collins]

Attachment: only the change to "nsLiteralString.h", as the "nsGlobalHistory.cpp" patch was checked in separately as part of bug #82208

Comment 25

[Chris Waterson]

r=thrill-kitty

Comment 26

[Scott Collins]

fix checked in