Closed Bug 239123 Opened 17 years ago Closed 17 years ago

exported functions in nsStringAPI.h should be frozen for 1.7 final


(Core :: String, defect)

Not set





(Reporter: darin.moz, Assigned: darin.moz)



(1 file, 1 obsolete file)

the exported functions in nsStringAPI.h should be frozen for 1.7 final.
Target Milestone: --- → mozilla1.7final
It was suggested in bug 239303 that the string conversion routines define an
enum type name for the NS_ENCODING_* values.  Originally, I left this out
because I couldn't think of a good name for the type.  Lame, I admit.

Perhaps nsCStringEncoding is good choice?
Sounds like a fine name to me.  Some purists make the enum names use a prefix
derived from the type name, maybe with some contraction or acronym'ing. 
NS_CSTRING_ENCODING_* seems a bit overlong, though, and NS_CSTRENC_ is
cybercrud. So NS_ENCODING_* is fine by me, except there are lots of "encoders"
and "decoders" in our world, and we don't want to collide.

Your call, but NS_CSTRING_ENCODING_ is the sure way to avoid problems, even if a
bit overlong.

I now think that we should also fixup the signatures of all the NS_C?String
methods to return nsresult (or maybe just PRBool) so that we have a way to
indicate OOM errors.  Sure, our current string implementation doesn't afford us
an easy way to detect these errors, but at least our API should have the ability
to convey it in the future.  For now, we can just lie and always return a
success code :-(
Attached patch v1 patch (obsolete) — Splinter Review
OK, this patch does the following:

1) sets the visibility hidden attribute on all inline functions in
nsStringAPI.h.	this is necessary to allow embedders to use the nsAC?String
methods in a debug build without crashing on Linux.  the code as is requires
all methods on nsAC?String to be inlined, but that doesn't happen in a debug
build, so we need to make sure they are not visible to other modules, like

2) defines the nsCStringEncoding enum type w/ NS_ENCODING changed to

3) adds BeginReading and EndReading methods to nsAC?String so embedders don't
have to resort to calling NS_C?StringGetData to access the raw buffer from a
nsAC?String instance.

4) in nsXPCOMGlue.cpp, replaced boolean xpcomGlueInit check with null checks of
the function pointers in xpcomFunctions.  this is basically reverting an
earlier change that i made during the 1.7 cycle.  i realized recently that
checking the individual functions is necessary in order to avoid crashes when a
component compiled against 1.7 is dropped into the components directory of for
example moz 1.6.
Attachment #145720 - Flags: superreview?(dbaron)
Attachment #145720 - Flags: review?(bsmedberg)
Comment on attachment 145720 [details] [diff] [review]
v1 patch

actually, this patch is incomplete.
Attachment #145720 - Attachment is obsolete: true
Attachment #145720 - Flags: superreview?(dbaron)
Attachment #145720 - Flags: review?(bsmedberg)
Attached patch v1.1 patchSplinter Review
I had forgotten to update the comment in nsStringAPI.h that gives sample usage
of the NS_C?String functions.
Comment on attachment 145747 [details] [diff] [review]
v1.1 patch

for real this time :)
Attachment #145747 - Flags: superreview?(dbaron)
Attachment #145747 - Flags: review?(bsmedberg)
Attachment #145747 - Flags: review?(bsmedberg) → review+
Attachment #145747 - Flags: superreview?(dbaron) → superreview+
Attachment #145747 - Flags: approval1.7?
Comment on attachment 145747 [details] [diff] [review]
v1.1 patch

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145747 - Flags: approval1.7? → approval1.7+
fixed-on-trunk for 1.7 final
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.