559489 - Remove dead code in intl

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Ehren Metcalfe]

Attachment: patch v1

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&)

Comment 1

[Simon Montagu :smontagu]

(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?

Comment 2

[(dormant account)]

(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?

Comment 3

[Simon Montagu :smontagu]

(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!

Comment 4

[Mark Banner (:standard8)]

(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?

Comment 5

[Ehren Metcalfe]

Attachment: patch v2

> 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()

Comment 6

[Ehren Metcalfe]

Attachment: patch v3

sorry, missed a comma in nsNativeUConvService.cpp. This must not be build on linux.

Comment 7

[Simon Montagu :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?

Comment 8

[Masatoshi Kimura [:emk]]

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.

Comment 9

[Ehren Metcalfe]

> 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).

Comment 10

[Masatoshi Kimura [:emk]]

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".

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #10)
> I propose incorporating Hankaku-to-Zenkaku conversion into ISO-2022-JP encoder
> (nsUnicodeToISO2022JP).

Filed bug 563283 on this

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 439252 [details] [diff] [review]
patch v3

r=me without the HankakuToZenkaku changes, which are obsoleted by bug 563283.

Comment 13

[Ehren Metcalfe]

Attachment: patch v4

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.

Comment 14

[Simon Montagu :smontagu]

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

Comment 15

[Simon Montagu :smontagu]

... which would be because of the changes from bug 568691 which got merged to mozilla-central yesterday

Comment 16

[Ehren Metcalfe]

Attachment: patch v5

This builds with ffx but comm-central's borked at the moment.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

bug 575740 is tracking the un-borking of comm-central.

Comment 18

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/4a6b283ec78c

Comment 19

[Simon Montagu :smontagu]

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