Last Comment Bug 239123 - exported functions in nsStringAPI.h should be frozen for 1.7 final
: exported functions in nsStringAPI.h should be frozen for 1.7 final
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.7final
Assigned To: Darin Fisher
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-29 15:37 PST by Darin Fisher
Modified: 2004-04-11 13:52 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (50.32 KB, patch)
2004-04-08 17:56 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1.1 patch (52.45 KB, patch)
2004-04-09 09:31 PDT, Darin Fisher
benjamin: review+
dbaron: superreview+
asa: approval1.7+
Details | Diff | Splinter Review

Description Darin Fisher 2004-03-29 15:37:45 PST
the exported functions in nsStringAPI.h should be frozen for 1.7 final.
Comment 1 Darin Fisher 2004-04-06 21:00:07 PDT
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?
Comment 2 Brendan Eich [:brendan] 2004-04-07 13:14:40 PDT
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
Comment 3 Darin Fisher 2004-04-08 15:48:16 PDT
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 :-(
Comment 4 Darin Fisher 2004-04-08 17:56:39 PDT
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.
Comment 5 Darin Fisher 2004-04-08 18:53:40 PDT
Comment on attachment 145720 [details] [diff] [review]
v1 patch

actually, this patch is incomplete.
Comment 6 Darin Fisher 2004-04-09 09:31:00 PDT
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.
Comment 7 Darin Fisher 2004-04-09 09:32:12 PDT
Comment on attachment 145747 [details] [diff] [review]
v1.1 patch

for real this time :)
Comment 8 Asa Dotzler [:asa] 2004-04-10 22:16:51 PDT
Comment on attachment 145747 [details] [diff] [review]
v1.1 patch

a=asa (on behalf of drivers) for checkin to 1.7
Comment 9 Darin Fisher 2004-04-11 13:52:49 PDT
fixed-on-trunk for 1.7 final

Note You need to log in before you can comment on or make changes to this bug.