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

RESOLVED FIXED in mozilla1.7final

Status

()

Core
String
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.7final
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

52.45 KB, patch
Benjamin Smedberg
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
the exported functions in nsStringAPI.h should be frozen for 1.7 final.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7final
(Assignee)

Comment 1

14 years ago
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.

/be
(Assignee)

Comment 3

14 years ago
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 :-(
(Assignee)

Comment 4

14 years ago
Created attachment 145720 [details] [diff] [review]
v1 patch

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
Gecko!

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

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.
(Assignee)

Updated

14 years ago
Attachment #145720 - Flags: superreview?(dbaron)
Attachment #145720 - Flags: review?(bsmedberg)
(Assignee)

Comment 5

14 years ago
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)
(Assignee)

Comment 6

14 years ago
Created attachment 145747 [details] [diff] [review]
v1.1 patch

I had forgotten to update the comment in nsStringAPI.h that gives sample usage
of the NS_C?String functions.
(Assignee)

Comment 7

14 years ago
Comment on attachment 145747 [details] [diff] [review]
v1.1 patch

for real this time :)
Attachment #145747 - Flags: superreview?(dbaron)
Attachment #145747 - Flags: review?(bsmedberg)

Updated

14 years ago
Attachment #145747 - Flags: review?(bsmedberg) → review+
Attachment #145747 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Updated

14 years ago
Attachment #145747 - Flags: approval1.7?

Comment 8

14 years ago
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+
(Assignee)

Comment 9

14 years ago
fixed-on-trunk for 1.7 final
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.