86000 - convert LDAP XPCOM SDK to use AUTF8String

Status: RESOLVED INCOMPLETE

(Directory Graveyard :: LDAP XPCOM SDK, enhancement, P3)

Description

[Leif Hedstrom]

We need to "clean" up the strings a little in the XPCOM. A few things should
probably be done:

1. Change |wstring| back to |string|, and assume (and enforce) UTF8 encoding,
where appropriate. Right now, we are converting a lot of strings back and forth
between UTF8 and UCS2, on the API boundaries.

2. Make sure we use the right XPIDL strings throughout the entire API (e.g.
|string| vs |AString| vs |wstring|.

3. There might be places where we can use a "cleaner" API, like

    |*_retval = ToNewUnicode(rv);|

instead of

    |*_retval = NS_ConvertUTF8toUCS2(rv).ToNewUnicode();|

(per scc suggestion).

I'm sure there are other string related cleanup that can be done as well,
particularly now that we have now cool abstract strings.


-- Leif

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

One reason why this would be nice is that sometimes stuff cross the LDAP XPCOM
SDK boundary several times, and each crossing implies a conversion.  This
definitely depends on 84186.

Comment 2

[scottputterman]

moving out. When we get more milestones we may want to bring this one back.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

We now have an XPCOM way to pass UTF8 strings!  Woot!  Changing summary to
reflect this...

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Man, I wish I hadn't tried to fix this.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 96945 [details] [diff] [review]
v1

OK, this looks great.  I was wrong about the earlier thing I mentioned to you
in email; the patch looks fine in that regard.	The one thing I do see that's
important is this:

nsAbLDAPDirectory.cpp
nsLDAPPrefsService.js
MsgComposeCommands.js
---------------------
We still need to use GetComplexPref (sigh).  GetCharPref is defined
to be ASCII-only data.	There really ought to be a UTF8 pref-getter
(especially since the prefs are written to disk as UTF8!), but, alas,
there is not.

Also in nsAbLDAPChangeLogQuery::QueryAuthDN,
I think this:

+    filter += NS_LITERAL_CSTRING("=") + aValueUsedToFindDn;

can probably be changed to the slightly more efficient:

+    filter += '=' + aValueUsedToFindDn;

but I might be wrong.

r=dmose, once you address the first comment.  Whether you address the second is
up to you.

Thanks for doing this!

Comment 6

[Peter Van der Beken [:peterv]]

After talking to dmose we decided to use GetCharPref anyway, since
GetComplexValue does GetCharPref and then converts from UTF8 to UCS2 (see
http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefBranch.cpp#377).
We're hoping that alecf (who I'll ask to sr) will comment on that.

The second suggestion didn't work but I've changed it to this:

    nsCAutoString filter(DIR_GetFirstAttributeString(mDirServer, auth));
    filter += '=';
    filter += aValueUsedToFindDn;

instead of

    nsCAutoString filter(DIR_GetFirstAttributeString(mDirServer, auth));
    filter += NS_LITERAL_CSTRING("=") + aValueUsedToFindDn;

Comment 7

[Alec Flett]

Comment on attachment 96945 [details] [diff] [review]
v1

yeah, GetCharPref is a fine optimization. the sort of unwritten rule is that
string prefs are either ASCII or UTF8.
this looks great though. sr=alecf

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 96945 [details] [diff] [review]
v1

This patch was checked in. I'll leave the bug open so dmose can decide what
else needs to be done (he mumbled something about getValues).

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

So I thought about this more, and the GetCharPref switch in JS will have
regressed stuff: XPConnect inflates |string|s with a 0 high byte; it doesn't
convert them from UTF8 to UCS2.  nsIPrefBranch really does need an AUTF8String
getter.  So the JS uses of that shortcut probably need to be backed out.

Comment 10

[Peter Van der Beken [:peterv]]

Ok, patch coming up that reverts the getCharPref change in the two js files.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Back to getComplexValue

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 98757 [details] [diff] [review]
Back to getComplexValue

r=dmose

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 98757 [details] [diff] [review]
Back to getComplexValue

Backed out the two changes to getCharPref.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Assigning bugs that I'm not actively working on back to nobody; use SearchForThis as a search term if you want to delete all related bugmail at once.

Comment 15

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"