575043 - deCOMtaminate nsICaseConversion

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

nsICaseConversion should not be an interface.  It's not available from script, and acts essentially as a namespace class (but one with virtual functions and outparams).  This patch converts it to actually be a namespace class and fixes up all the consumers and the tests.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Managed to sneak in a few bits from another patch.  This version is clean.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 454335 [details] [diff] [review]
Patch

This is fine as-is, but maybe we could improve it? In particular, can we just remove nsUnicharUtils.cpp completely and have the callers call nsCaseConversion directly?

Also, why not go the whole way and make these static functions in a C++ namespace? Just the mozilla namespace would be fine IMHO.

Does this work in both libxul and non-libxul builds?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #2)
> (From update of attachment 454335 [details] [diff] [review])
> This is fine as-is, but maybe we could improve it? In particular, can we just
> remove nsUnicharUtils.cpp completely and have the callers call nsCaseConversion
> directly?

We could do that.

> Also, why not go the whole way and make these static functions in a C++
> namespace? Just the mozilla namespace would be fine IMHO.

OK.

> Does this work in both libxul and non-libxul builds?

No, it breaks non-libxul builds, but fixing it is trivial enough (we just need to link the library that nsCaseConversion.cpp ends up in into the library nsUnicharUtils.cpp lives in)

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The other issue is that we seem to promise what's in nsUnicharUtils.cpp to frozen linkage consumers.  I assume that combining nsUnicharUtils and nsCaseConversion into stuff in the mozilla namespace would break those linked against it.  Do we care?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Doesn't that patch break non-libxul builds?  Do we link spellcheck into libxul in libxul builds, even?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #5)
> Doesn't that patch break non-libxul builds?
Yes, but the fix is simple.

> Do we link spellcheck into libxul in libxul builds, even?
Yes, http://mxr.mozilla.org/mozilla-central/source/toolkit/library/libxul-config.mk#333

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This folds nsCaseConversion into the unicharutils library, which gets us around all of our non-libxul troubles.  I didn't move this into the mozilla namespace because that gets tricky with nsUnicharUtils.h and the like, and I didn't want to run around changing half the codebase.  I think we should punt fixing that to a day when we make our string APIs sane :-)

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Talking to timeless about this on irc: he says that there are good reasons for continuing to expose the interface externally (since Gecko's character handling may not match the system) even after we remove it internally.  Not sure how I feel about that.  Thoughts?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about doing an "hg rename" from intl/unicharutil/public/nsICaseConversion.h or nsCaseConversionImp2.h to intl/unicharutil/public/nsCaseConversion.h, and from intl/unicharutil/src/nsCaseConversionImp2.cpp to intl/unicharutil/util/nsCaseConversion.cpp, so we get a better diff?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This folds away nsCaseConversion and puts all of its functionality into nsUnicharUtils.cpp, and then reimplements the interface on top of that for any external consumers that want it.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Fixing a minor bug.  Carrying forward sr.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/e0fa43b96c03