Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #454334 - Flags: superreview?(roc)
Attachment #454334 - Flags: review?(smontagu)
Posted patch Patch (obsolete) — Splinter Review
Managed to sneak in a few bits from another patch.  This version is clean.
Attachment #454334 - Attachment is obsolete: true
Attachment #454335 - Flags: superreview?(roc)
Attachment #454335 - Flags: review?(smontagu)
Attachment #454334 - Flags: superreview?(roc)
Attachment #454334 - Flags: review?(smontagu)
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?
Attachment #454335 - Flags: superreview?(roc) → superreview+
(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)
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?
Doesn't that patch break non-libxul builds?  Do we link spellcheck into libxul in libxul builds, even?
(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
Attachment #454335 - Flags: review?(smontagu)
Posted patch Patch (obsolete) — Splinter Review
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 :-)
Attachment #458164 - Flags: superreview?(roc)
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?
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?
Attachment #458164 - Attachment is obsolete: true
Attachment #458164 - Flags: superreview?(roc)
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #459324 - Flags: superreview?(roc)
Attachment #459324 - Flags: review?
Attachment #459324 - Flags: superreview?(roc) → superreview+
Attachment #459324 - Flags: review? → review?(smontagu)
Posted patch PatchSplinter Review
Fixing a minor bug.  Carrying forward sr.
Attachment #459324 - Attachment is obsolete: true
Attachment #460067 - Flags: superreview+
Attachment #460067 - Flags: review?
Attachment #459324 - Flags: review?(smontagu)
Attachment #460067 - Flags: review? → review?(smontagu)
Attachment #460067 - Flags: review?(smontagu) → review+

Updated

9 years ago
Attachment #460067 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/e0fa43b96c03
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 622637
Blocks: 660755
You need to log in before you can comment on or make changes to this bug.