Closed Bug 559489 Opened 14 years ago Closed 14 years ago

Remove dead code in intl

Categories

(Core :: Internationalization, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ehren.m, Assigned: ehren.m)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
This should be almost all the dead code in the intl directory. A couple of these removals cross module boundaries so I suppose more review will be needed if they're initially accepted.

Some of these are a bit problematic though:

* None of these are scriptable but I'm a bit confused once XPCOM comes into play eg with all the NS_New functions.

* The Hankaku code may be completely dead. By removing the reference in intl/build/nsI18nModule.cpp I was able to remove the entire class + the nsITextTransform interface. I'm guessing this is too aggressive though.

* Removing FillInfo requires removing a test from nsTestUConv.cpp (it's already commented out though)

* ToTitle also requires a test removal.

* Mirrored is generated code. I removed it from the perl script as well. 

* The big one: Everything is dead in nsCompressedCharMap.cpp, but the macros in nsCompressedCharMap.h are used. I removed nsCompressedCharMap.cpp completely and all the actual code in nsCompressedCharMap.h however I left the macros and all comments unchanged (guessing this is not quite satisfactory). I also had to remove some references in trace-malloc/types.dat

Function list:

in intl/unicharutil/src:

nsresult ::NS_NewEntityConverter(nsISupports**)
nsresult ::NS_NewSaveAsCharset(nsISupports**)
void ::HankakuToZenkaku(const PRUnichar*, PRInt32, PRUnichar*, PRInt32, PRInt32*)
nsresult nsHankakuToZenkaku::Change(nsString&, nsString&)
nsresult nsHankakuToZenkaku::Change(const PRUnichar*, PRInt32, nsString&)

intl/unicharutil/public:

nsresult nsITextTransform::Change(nsString&, nsString&)
nsresult nsICaseConversion::ToTitle(const PRUnichar*, PRUnichar*, PRUint32, PRBool)
nsresult nsCaseConversionImp2::ToTitle(const PRUnichar*, PRUnichar*, PRUint32, PRBool)

intl/unicharutil/util:

PRInt32 ::CaseInsensitiveCompare(const PRUnichar*, const PRUnichar*, PRUint32)
PRUint32 ::Mirrored(PRUint32)
PRUint32 ::SymmSwap(PRUint32)
eBidiCategory ::GetBidiCategory(PRUint32)
PRBool ::IsBidiCategory(PRUint32, eBidiCategory)

+ Everything but the macros in nsCompressedCharMap.cpp / nsCompressedCharMap.h 

intl/uconv/src:

nsresult ::NS_NewUTF8ToUnicode(nsISupports*, const nsIID&, void**)
nsresult nsICharRepresentable::FillInfo(PRUint32*) + all derived class methods (13 or so)
void nsGBKConvUtil::FillGB2312Info(PRUint32*)
nsresult ::FillInfoEUCKR(PRUint32*, PRUint16, PRUint16)
void ::FillInfoRange(PRUint32*, PRUint32, PRUint32)

intl/uconv/util:

void ::uFillInfo(const uTable*, PRUint32*)

intl/uconv/ucvko:
nsresult ::GetDecoder(nsIUnicodeDecoder**)

intl/lwbrk/public:
PRInt32 nsIWordBreaker::PrevWord(const PRUnichar*, PRUint32, PRUint32)

intl/lwbrk/src:
PRInt32 nsSampleWordBreaker::PrevWord(const PRUnichar*, PRUint32, PRUint32)

intl/chardet/src:
nsresult nsDetectionAdaptor::ProcessTokens()
nsresult nsDetectionAdaptor::WillAddToken(CToken&)
nsresult nsCyrXPCOMStringDetector::DoIt(const char*, PRUint32, const char**, nsDetectionConfident&)

intl/chardet/public:
nsresult nsIStringCharsetDetector::DoIt(const char*, PRUint32, const char**, nsDetectionConfident&)

parser/htmlparser/public:
nsresult nsIParserFilter::ProcessTokens()
nsresult nsIParserFilter::WillAddToken(CToken&)

extensions/universalchardet/src/xpcom:
nsresult nsXPCOMStringDetector::DoIt(const char*, PRUint32, const char**, nsDetectionConfident&)
Attachment #439155 - Flags: review?(smontagu)
Blocks: deadcode
(In reply to comment #0)

> * The Hankaku code may be completely dead. By removing the reference in
> intl/build/nsI18nModule.cpp I was able to remove the entire class + the
> nsITextTransform interface. I'm guessing this is too aggressive though.

A shooting-from-the-hip reaction before I look at the patch in detail: AFAIK Hankaku to Zenkaku conversion isn't dead, it's used by mailnews. Are you identifying dead code by looking only in mozilla-central and not comm-central?
(In reply to comment #1)
> (In reply to comment #0)
> 
> > * The Hankaku code may be completely dead. By removing the reference in
> > intl/build/nsI18nModule.cpp I was able to remove the entire class + the
> > nsITextTransform interface. I'm guessing this is too aggressive though.
> 
> A shooting-from-the-hip reaction before I look at the patch in detail: AFAIK
> Hankaku to Zenkaku conversion isn't dead, it's used by mailnews. Are you
> identifying dead code by looking only in mozilla-central and not comm-central?

Yes. And we shouldn't be shipping code in firefox that isn't used by it. Is there a way to ifdef it out in a thunderbird-friendly way?
(In reply to comment #2)
> Yes. And we shouldn't be shipping code in firefox that isn't used by it. Is
> there a way to ifdef it out in a thunderbird-friendly way?

I don't know. Mark?

If FillInfo is unused, nsICharRepresentable can go away completely.
Similarly, if Mirrored is unused, symmtable.h and gensymmtable.pl can just go away.

It looks as if the HTML5 parser uses nsXPCOMStringDetector::DoIt

I don't understand what's going on in trace-malloc/types.dat -- the reference to nsCompressedCharMap is via nsFontMetricsGTK, which is even deader!
(In reply to comment #3)
> (In reply to comment #2)
> > Yes. And we shouldn't be shipping code in firefox that isn't used by it. Is
> > there a way to ifdef it out in a thunderbird-friendly way?
> 
> I don't know. Mark?

No. Thunderbird is actually removing ifdefs from core not adding to them. Core should not have any product-dependent ifdefs.

Take for example if we did swap to using xulrunner for both Firefox & Thunderbird (which was on the books, but maybe not so much now, but I guess it could come back again) - we would require the same core code in xulrunner even if one app didn't use all parts, having ifdefs means that wouldn't work.

I'm not sure what the Hankaku to Zenkaku conversion is used for in mailnews, as I don't know enough about the intl code. However I suspect the questions that should be asked here are:

- Should this be in the platform anyway? Mailnews requires it, are other xulrunner based apps or even extensions to Firefox going to want it. Firefox not using it isn't necessarily a good answer in itself IMO.

- If the answer to the above question is "perhaps not", then the next question is can the code be factored out in a sensible manner such that mailnews can implement and maintain the conversion code?
Attached patch patch v2 (obsolete) — Splinter Review
> If FillInfo is unused, nsICharRepresentable can go away completely.
> Similarly, if Mirrored is unused, symmtable.h and gensymmtable.pl can just go
> away.

Nuked these. Note: I haven't tested the changes to nsWinCEUConvService.cpp. Shouldn't be a problem though.

> 
> It looks as if the HTML5 parser uses nsXPCOMStringDetector::DoIt
>

There are a bunch of unrelated methods called DoIt but there are no calls to any 4 argument versions.

> I don't understand what's going on in trace-malloc/types.dat -- the reference
> to nsCompressedCharMap is via nsFontMetricsGTK, which is even deader!

removed the references to nsFontMetricsGTK.

Still not sure what to do about Hankaku et all. According to a comm-central search it's used by js code? I thought it wasn't even scriptable.

I also neglected to mention the removal of void nsBufferDecoderSupport::DoubleBuffer()
Attachment #439155 - Attachment is obsolete: true
Attachment #439249 - Flags: review?(smontagu)
Attachment #439155 - Flags: review?(smontagu)
Attached patch patch v3 (obsolete) — Splinter Review
sorry, missed a comma in nsNativeUConvService.cpp. This must not be build on linux.
Attachment #439249 - Attachment is obsolete: true
Attachment #439252 - Flags: review?(smontagu)
Attachment #439249 - Flags: review?(smontagu)
(In reply to comment #4)
> I'm not sure what the Hankaku to Zenkaku conversion is used for in mailnews, as
> I don't know enough about the intl code.

It's used when sending mail in ISO-2022-JP: the Hankaku (half-width Katakana) characters aren't representable in ISO-2022-JP, so we convert them to Zenkaku (full-width Katakana).

> However I suspect the questions that
> should be asked here are:
> 
> - Should this be in the platform anyway? Mailnews requires it, are other
> xulrunner based apps or even extensions to Firefox going to want it. Firefox
> not using it isn't necessarily a good answer in itself IMO.

As Ehren points out, it's not scriptable. Chatzilla rolls its own implementation.

> - If the answer to the above question is "perhaps not", then the next question
> is can the code be factored out in a sensible manner such that mailnews can
> implement and maintain the conversion code?

I think that would be one possible option. Another possibility would be to remove it anyway -- in today's codebase I assume that in that case we would offer to send mail in UTF-8, and that might even be preferable. Masatoshi, what do you think here?
Japanese users tend to be conservative. Majority of Japanese mails are still encoded in ISO-2022-JP (especially for mobile phones).

By the way, I'm going to use nsCompressedCharMap.h again to fix bug 546013.
> By the way, I'm going to use nsCompressedCharMap.h again to fix bug 546013.

Looking at your gfx patch it looks like you're just using DEFINE_X_CCMAP and CCMAP_HAS_CHAR_EXT. Those are also used in nsContentUtils.cpp and aren't touched by this patch (only the non-macro c++ was removed).
I propose incorporating Hankaku-to-Zenkaku conversion into ISO-2022-JP encoder (nsUnicodeToISO2022JP).
Rationales:
* Other browsers (at least Internet Explorer, Chrome, Safari for Win, and Opera for Win) already support the conversion.
* Registration of CP50220 is in progress. It mandates the conversion on sending.
  http://mail.apps.ietf.org/ietf/charsets/msg01882.html
  This charset is meant to be used as a replacement encoding for HTML5 "misinterpret for compatibility".
Depends on: 563283
(In reply to comment #10)
> I propose incorporating Hankaku-to-Zenkaku conversion into ISO-2022-JP encoder
> (nsUnicodeToISO2022JP).

Filed bug 563283 on this
Comment on attachment 439252 [details] [diff] [review]
patch v3

r=me without the HankakuToZenkaku changes, which are obsoleted by bug 563283.
Attachment #439252 - Flags: review?(smontagu) → review+
Assignee: smontagu → ehren.m
Attached patch patch v4 (obsolete) — Splinter Review
Unfortunately, the above patch (even without HankakuToZenkaku) won't build with Thunderbird because of the 'DoIt' methods.

This patch _will_ build with both applications as of tonight though and makes a number of additional removals. It's been a while since I actually created the majority of this however so forgive me for not posting a list of removals.
Attachment #439252 - Attachment is obsolete: true
Attachment #455411 - Flags: review?
Attachment #455411 - Flags: review? → review?(smontagu)
Attachment #455411 - Flags: review?(smontagu) → review+
Keywords: checkin-needed
I get failures applying the patch:

patching file intl/chardet/src/nsChardetModule.cpp
Hunk #1 FAILED at 65
Hunk #2 FAILED at 123
2 out of 2 hunks FAILED -- saving rejects to file intl/chardet/src/nsChardetModule.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
... which would be because of the changes from bug 568691 which got merged to mozilla-central yesterday
Attached patch patch v5Splinter Review
This builds with ffx but comm-central's borked at the moment.
Attachment #455411 - Attachment is obsolete: true
Attachment #455653 - Flags: review?(smontagu)
Keywords: checkin-needed
bug 575740 is tracking the un-borking of comm-central.
Attachment #455653 - Flags: review?(smontagu) → review+
http://hg.mozilla.org/mozilla-central/rev/4a6b283ec78c
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/mozilla-central/rev/846890403c24 to fix bustage on Windows tinderboxes:

nsWindowsShellService.cpp
e:/builds/moz2_slave/mozilla-central-win32/build/browser/components/shell/src/nsWindowsShellService.cpp(474) : error C2664: 'PRBool nsAString::Equals(const nsAString::char_type *,nsAString::ComparatorFunc) const' : cannot convert parameter 2 from 'PRInt32 (__cdecl *)(const char *,const char *,PRUint32)' to 'nsAString::ComparatorFunc'
        None of the functions with this name in scope match the target type
Depends on: 580580
Depends on: 620106
Blocks: 458720
You need to log in before you can comment on or make changes to this bug.