Closed Bug 234867 Opened 18 years ago Closed 13 years ago

Rationalize nsIMsgHeaderParser.idl

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Work in progress (obsolete) — Splinter Review
Assignee: sspitzer → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #141733 - Attachment is obsolete: true
Attachment #141735 - Flags: superreview?(bienvenu)
Attachment #141735 - Flags: review?(jshin)
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)
(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

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?
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.
Attached patch Simplified patchSplinter Review
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.
Attachment #141735 - Attachment is obsolete: true
Comment on attachment 143227 [details] [diff] [review]
Simplified patch

David, do you want me to find separate review for this?
Attachment #143227 - Flags: superreview?(bienvenu)
Blocks: 145798
Comment on attachment 143227 [details] [diff] [review]
Simplified patch

r/sr=bienvenu
Attachment #143227 - Flags: superreview?(bienvenu) → superreview+
Neil, is it OK to mark this Fixed?  The current patch for bug 145798 relies 
on this change.
No longer blocks: 145798
Attachment #141735 - Flags: superreview?(bienvenu)
Attachment #141735 - Flags: review?(jshin)
Product: MailNews → Core
Neil?  ^^
QA Contact: esther → backend
Product: Core → MailNews Core
Attached patch Odds and endsSplinter Review
Not making them scriptable this time, but just converting outparams to retvals.
Attachment #338848 - Flags: review?(bienvenu)
Attachment #338848 - Flags: review?(bienvenu) → review+
Pushed changeset e8ba2f193d98 to comm-central.

I'll resolve this bug now; anyone doing other cleanup can open another one.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → Thunderbird 3.0a3
You need to log in before you can comment on or make changes to this bug.