234867 - Rationalize nsIMsgHeaderParser.idl

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

Bug 145798 got me thinking... a) why are many of the methods void with a single
out parameter b) why are the ones that are not scriptable, not scriptable c) why
are there scriptable wstring versions of some of the not scriptable methods.
The only method that I'm not interested in is parseHeaderAddresses because the
returned strings may contain nulls.

Comment 1

[[e-mail]]

Attachment: Work in progress

Comment 2

[[e-mail]]

Attachment: Proposed patch

Comment 3

[David :Bienvenu]

looks good, but need similar patch for tbird file mailCommands.js, abcommon.js
(there seem to be two instances of this file in the tbird tree - not sure if
either or both of them need to be changed as well)

Comment 4

[Jungshik Shin]

(In reply to comment #0)
> Bug 145798 got me thinking... b) why are the ones that are not scriptable, not
> scriptable c) why are there scriptable wstring versions of some of the not 
> scriptable methods.

  It seems like  non-scriptable versions were originally written to deal with
'char *' of an any chracter encoding ('charset' parameter is to indicate the
encoding). However, if you look at their implementations, it's assumed that by
the time we reach there, everything has to be in UTF-8 [1]. I get a lot of
assertions in xpcom/string (UTF-8 conversion routine) when I open a mail folder
with 'raw 8bit characters'. Mozilla moves along fine because static functions
actually dealing with header fields in nsMsgHeaderParser.cpp can deal with
non-UTF-8 8bit strings. Given this, I guess NS_ASSERTION(IsUTF8())'s have to be
replaced by NS_WARNING.


[1] 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsMsgHeaderParser.cpp&branch=&root=/cvsroot&subdir=mozilla/mailnews/mime/src&command=DIFF_FRAMESET&rev1=1.53&rev2=1.54

Comment 5

[[e-mail]]

I didn't mean to look at the implementation... but that strikes me as odd, why
check for utf8ness if we pass an explicit character set?

Comment 6

[Jungshik Shin]

Yeah, it's odd. I havent' done any code archaelogy. Perhaps, in the past,
'charset' parameter was used, but it's not used in any of implementations.
Anyway, unless we're sure that all 'string's are ASCII-only, we can't make
methods with 'string's cross the xpconnect boundary, can we? If they're always
UTF-8, we have to use AUTF8String, instead as you know well.

Comment 7

[[e-mail]]

Attachment: Simplified patch

Ok, the rest of the file will have to wait for jshin's general string cleanup,
so for now I just want to change an out parameter to be a return value.

Comment 8

[[e-mail]]

Comment on attachment 143227 [details] [diff] [review]
Simplified patch

David, do you want me to find separate review for this?

Comment 9

[David :Bienvenu]

Comment on attachment 143227 [details] [diff] [review]
Simplified patch

r/sr=bienvenu

Comment 10

[Mike Cowperthwaite]

Neil, is it OK to mark this Fixed?  The current patch for bug 145798 relies 
on this change.

Comment 11

[Wayne Mery (:wsmwk)]

Neil?  ^^

Comment 12

[[e-mail]]

Attachment: Odds and ends

Not making them scriptable this time, but just converting outparams to retvals.

Comment 13

[[e-mail]]

Pushed changeset e8ba2f193d98 to comm-central.

I'll resolve this bug now; anyone doing other cleanup can open another one.