Closed
Bug 575043
Opened 14 years ago
Closed 14 years ago
deCOMtaminate nsICaseConversion
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
People
(Reporter: khuey, Assigned: khuey)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
52.34 KB,
patch
|
smontagu
:
review+
khuey
:
superreview+
benjamin
:
approval2.0+
|
Details | Diff | 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)
Assignee | ||
Comment 1•14 years ago
|
||
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+
Assignee | ||
Comment 3•14 years ago
|
||
(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)
Assignee | ||
Comment 4•14 years ago
|
||
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•14 years ago
|
||
Doesn't that patch break non-libxul builds? Do we link spellcheck into libxul in libxul builds, even?
Assignee | ||
Comment 6•14 years ago
|
||
(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
Updated•14 years ago
|
Attachment #454335 -
Flags: review?(smontagu)
Assignee | ||
Updated•14 years ago
|
Attachment #454335 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #458164 -
Attachment is obsolete: true
Attachment #458164 -
Flags: superreview?(roc)
Assignee | ||
Comment 10•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #459324 -
Flags: review? → review?(smontagu)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #460067 -
Flags: review? → review?(smontagu)
Updated•14 years ago
|
Attachment #460067 -
Flags: review?(smontagu) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #460067 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #460067 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 12•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e0fa43b96c03
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•