236954 - Merge uses of mail.show_headers pref

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Karsten Düsterloh]

Currently, we have three places evaluating the mail.show_headers pref 
(that determines which mail/news headers are shown):
- mailnews\mime\src\mimedrft.cpp:
  which headers get included in forward inline
- mozilla\mailnews\mime\emitters\src\nsEmitterUtils.cpp:
  which headers get used in printing (but see below)
  or if no messagesink is waiting for headers
- mozilla\mailnews\mime\emitters\src\nsMimeHtmlEmitter.cpp:
  which headers are shown in the headerpane of the mailnews window

Not only do these locations use *different* sets of headers for the *same* pref
value, they even have their behaviour hardcoded. If adding/changing/removing
headers, all three locations have to be touched - and that already failed in the
past:

Example:
Given |user_pref("mail.show_headers", 1);| aka "Normal View",
|user_pref("mailnews.headers.showUserAgent", true);| and these headers:

Subject: test
From: <[e-mail]>
Date: Sat, 09 Mar 2004 11:11:11 +1111
To: <[e-mail]>
Resent-Comments: just a COMMENT!
User-Agent: some Mozilla/5.0
Message-ID: <[e-mail]>

The headerpane will show this:

Subject: test
From: <[e-mail]>
Date: Sat, 09 Mar 2004 11:11:11 +1111
To: <[e-mail]>
User-Agent: some Mozilla/5.0

The print preview will show this:

Subject: test
From: <[e-mail]>
Date: Sat, 09 Mar 2004 11:11:11 +1111
To: <[e-mail]>

And forwarding this mail inline gets us:

-------- Original Message --------
Subject: test
Resent-Comments: just a COMMENT!
Date: Sat, 09 Mar 2004 11:11:11 +1111
From: <[e-mail]>
To: <[e-mail]>

This isn't very consistent. ;-)

Since nsEmitterUtils.cpp consists of only one function that does such a header
check (but is never 'really' used), that would a possible place for a unifying
implementation, maybe a class like:

class nsMimeHeaderPrefs
{
protected:
    nsCStringArray  m_rstrHeaderArray;  // allowed headers
    PRBool          m_bShowAll;         // show all headers?

public: 
    // usage of nsMimeHeaderDisplayTypes as aiDisplayType is recommended here
                    nsMimeHeaderPrefs(PRInt32 aiDisplayType);
    virtual         ~nsMimeHeaderPrefs();

    // reinit targeted header pref
    // usage of nsMimeHeaderDisplayTypes as aiDisplayType is recommended here
    PRBool          Init        (PRInt32 aiDisplayType);
    PRBool          InitForPrint(PRInt32 aiDisplayTypeOffset);
    
    // check whether this header is allowed for display with this display type
    PRBool          EmitThisHeader(const char *header);
};

I'm willing to implement this merge.

Comments?

Comment 1

[Jens Bannmann]

Sounds like a good idea!

Another (related?) issue: nsMimeHtmlEmitter's hard-coded set of headers prevents
chrome-level extensions from displaying or processing extra headers. Currently,
I have to work around this by setting the pref to ALL before and back to NORMAL
after loading messages, which is quite ugly. Could you introduce a scriptable
method to register additional headers for certain display types on-the-fly?

Comment 2

[Karsten Düsterloh]

My current implementation reads the entire header configuration from the
preferences. The interface shown in comment #0 is very outdated. I hope I will
have a 'beautified' patch available this year. ;-)

Comment 3

[Jens Bannmann]

(In reply to comment #2)
> My current implementation reads the entire header configuration from the
> preferences.

While prefs are certainly better than hard-coded stuff, they still have a
drawback: As extensions cannot alter existing preferences at install-time (only
provide new defaults), they would have to alter the header config prefs on first
run. This would leave the modifications lying around after disabling or
uninstalling the extension, which is not desirable.

Therefore, I'd suggest that in addition to the prefs thing, we should also
provide a scriptable XPCOM interface for on-the-fly changes. An extension could
then just register the headers it needs on startup, in a non-permanent way.

(In reply to comment #2)
> I hope I will have a 'beautified' patch available this year. ;-)

You'll be my personal hero if this is included in the 1.0 release :-)

Comment 4

[Karsten Düsterloh]

> You'll be my personal hero if this is included in the 1.0 release :-)

TB 1.0? This is extremely unlikely.

Comment 5

[Seth Ladd]

As a fledgling extensions author, I would love to have a way to get all headers
for the currently selected (or viewed) message.  That is, without having to
Display all headers and then set the preference back (see
https://bugzilla.mozilla.org/show_bug.cgi?id=236954#c1).  Having a way to
register my extension to be notified of certain headers would be extremely
helpful and really open up the extension implementation possibilities.

Thanks,
Seth

Comment 6

[Jens Bannmann]

(In reply to comment #0)
> - mozilla\mailnews\mime\emitters\src\nsMimeHtmlEmitter.cpp:
>   which headers are shown in the headerpane of the mailnews window

Karsten: to be exact, this does not control which headers are shown in the UI -
it only controls which headers are made accessible to the UI. The mailnews
chrome defines its own list of which headers to show (and how to show them) at
http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#98
and adds user-agent/organization on-the-fly at
http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgHdrViewOverlay.js#178

So in fact, we have our fourth(!) place here. I don't know whether that one
should be changed, too - perhaps it should read which headers to show from your
nsMimeHeaderPrefs, and only define the special output functions. However, that
is chrome/JS logic and changing it probably should be the scope of another
follow-up bug depending on this.

In any case, we should keep the distinction between sent-to-UI and shown-in-UI -
for example, it is very important for the MessageFaces extension, which needs to
access the Face: header field - although it should not be shown as a header line.

Hope this is somehow doable with your implementation plans... Did you have any
time for working on this in the meanwhile?

Comment 7

[Karsten Düsterloh]

>> - mozilla\mailnews\mime\emitters\src\nsMimeHtmlEmitter.cpp:
>>   which headers are shown in the headerpane of the mailnews window
> 
> Karsten: to be exact, this does not control which headers are shown in the UI -
> it only controls which headers are made accessible to the UI.

Yeah, I know. 

> So in fact, we have our fourth(!) place here. I don't know whether that one
> should be changed, too - perhaps it should read which headers to show from your
> nsMimeHeaderPrefs, and only define the special output functions. However, that
> is chrome/JS logic and changing it probably should be the scope of another
> follow-up bug depending on this.

My current implementation just tinkers with the backend selection of headers.

> In any case, we should keep the distinction between sent-to-UI and shown-in-UI -
> for example, it is very important for the MessageFaces extension, which needs to
> access the Face: header field - although it should not be shown as a header line.

Yes. So do Mnenhy, Enigmail, etc.

> Hope this is somehow doable with your implementation plans... 
> Did you have any time for working on this in the meanwhile?

Not enough. :(
But I'll try to attach a diff (no actual "patch" since there's still some work
to be done) against the current trunk next week, so that interested parties ;-)
may give first comments.

Comment 8

[Jens Bannmann]

(In reply to comment #7)
> > Did you have any time for working on this in the meanwhile?
> Not enough. :(
> But I'll try to attach a diff (no actual "patch" since there's still some work
> to be done) against the current trunk next week, so that interested parties ;-)
> may give first comments.

Karsten, do you have any updates/guesstimates on this? (Just out of
curiousity...) If you provide a first draft and some pointers what's left to be
done, I *might* be able to help out a bit (no promises, though).

Comment 9

[Karsten Düsterloh]

Attachment: Proposed header display handling service interface

So, I finally started overhauling this whole shebang. My current plan is to use
the attached interface in all the relevant places, e.g. mimedrft.cpp or
nsMime*Emitter.cpp. Comments are very appreciated!

Comment 10

[Jens Bannmann]

1)
Could you explain the reasoning behind having "parts"? For defining the order of
the headers, they wouldn't be neccessary (one pref per mode would be enough)...

2)
The mail.show_headers is kept as an int, where 3 means the "first custom header
list", which is mapped to mail.show_headers.list.custom.* prefs. Shouldn't this
be custom1, custom2 and so on? Or even just the number matching the int pref.

3)
The empty string pref for indicating "all others" looks a bit odd, but I guess a
real wildcard like * can't be used as it is a legal header name...

4)
The name "getDisplayPartIndexCount" is misleading - it counts headers, not
indexes. How about "getDisplayPartHeaderCount"?

5)
Is it intentional that there's no way to actually retrieve all headers of a
certain part, interface-wise (not looking directly at the prefs)? I don't know
how the calling code parts look like - I can imagine they examine existing
header after header and decide whether to display it, but a 
 string getDisplayPartHeader(in long aPart, in long aHeaderIndexInsidePart)
function might be a nice roundup.

6)
I thought a while about where extensions like Enigmail, Mnenhy or MessageFaces
fit in the picture, but couldn't find a way for distinguishing between the
"visible" header uses (inline, printing, front-end headerpane plus indirectly
nsMimeHtmlEmitter) and the "invisible" one (extra headers sent by
nsMimeHtmlEmitter, but not displayed) in the prefs. However, we certainly need a
clean extension hook for the emitter - I see two approaches:

A: This interface will be only about the visible stuff, and we put an extension
hook directly into nsMimeHtmlEmitter

B: We keep nsMimeHtmlEmitter clean, but put the extension hook into the
interface, e.g. isEmitted(in string aHeader). nsMimeHtmlEmitter then uses this
function in addition to the regular lists.

That extension hook would would be backed by either on-the-fly registration
and/or prefs [one per header, so extensions can register using default prefs
without modifying existing ones, see comment 3]

What do you all think? I'd be fine with either approach.

Comment 11

[Jens Bannmann]

(In reply to comment #10)
> Is it intentional that there's no way to actually retrieve all headers of a
> certain part, interface-wise (not looking directly at the prefs)?

Forget that, I overlooked the enumerateHeaders() function...

Comment 12

[Karsten Düsterloh]

I'm well aware that discussing an interface without the actual implementation is
somewhat academic, but I'm revamping that currently and post it asap.
Furthermore, the proposed interface contains some stuff that is (partly) "just"
moved from somewhere else (eg. localized headers and the extended
nsMimeHeaderDisplayTypes).

(In reply to comment #10)
> 1) Could you explain the reasoning behind having "parts"? For
> defining the order of the headers, they wouldn't be neccessary (one
> pref per mode would be enough)...

When displaying messages in a browser window (or if no message sink is
available), headers are "presorted" into currently three differently styled
parts. See nsMimeBaseEmitter::WriteHTMLHeaders().

> 2) The mail.show_headers is kept as an int, where 3 means the "first
> custom header list", which is mapped to
> mail.show_headers.list.custom.* prefs. Shouldn't this be custom1,
> custom2 and so on? Or even just the number matching the int pref.

mail.show_headers is int to maintain a certain degree of backwards compatibility. 

mail.show_headers is the index in mail.show_headers.lists, the actual name
"custom" is irrelevant. You could use "custom1" or "mypreferredones" - it
doesn't matter as long as Mozilla's preference system is able to parse it as a
pref tree branch name.
Mind, you could have multiple custom header sets!

> 3) The empty string pref for indicating "all others" looks a bit odd,
> but I guess a real wildcard like * can't be used as it is a legal
> header name...

Yes.

> 4) The name "getDisplayPartIndexCount" is misleading - it counts
> headers, not indexes. How about "getDisplayPartHeaderCount"?

Okay, seems better.

> 6) I thought a while about where extensions like Enigmail, Mnenhy or
> MessageFaces fit in the picture, but couldn't find a way for
> distinguishing between the "visible" header uses (inline, printing,
> front-end headerpane plus indirectly nsMimeHtmlEmitter) and the
> "invisible" one (extra headers sent by nsMimeHtmlEmitter, but not
> displayed) in the prefs.

Actually, I believe this is something different.
This interface here takes care of header display only and extensions should be
able to request any header they need indepently from that.
I already discussed that with Patrick (Enigmail) a while ago and Christian
(mailinglistheader) last week and there seems to be a certain agreement that the
best way to achieve this would be a function like "getHeaderValue(headerName)"
in nsIMsgHdr.

> However, we certainly need a clean extension
> hook for the emitter - I see two approaches:
> 
> A: This interface will be only about the visible stuff, and we put an
> extension hook directly into nsMimeHtmlEmitter
> 
> B: We keep nsMimeHtmlEmitter clean, but put the extension hook into
> the interface, e.g. isEmitted(in string aHeader). nsMimeHtmlEmitter
> then uses this function in addition to the regular lists.
> 
> That extension hook would would be backed by either on-the-fly
> registration and/or prefs [one per header, so extensions can register
> using default prefs without modifying existing ones, see comment 3]

My current implementation here simplifies 
nsMimeHtmlEmitter insofar as it just makes checks against the interface and
skips all that hard coded stuff, because I want to keep apart the display of
headers from the processing of headers by extensions.

Comment 13

[[e-mail]]

Comment on attachment 187336 [details]
Proposed header display handling service interface

>  long    getDisplayPartCount     ();
readonly attribute displayPartCount; is the preferred style for this sort of
thing. But at least you didn't make the mistake that other mail interfaces do
of using void getDisplayPartCount(out long aDisplayPartCount); ;-)

Comment 14

[Gary Oberbrunner]

Just wondering if this bug is getting any action; hasn't been updated since 2005.  It makes mnenhy (tbird extension) header view have problems when printing or forwarding messages (because it has to turn on all headers internally and then hide most); see here: https://www.mozdev.org/bugs/show_bug.cgi?id=8282.

Comment 15

[Ludovic Hirlimann [:Usul]]

Karsten are you still working on this ?

Comment 16

[Karsten Düsterloh]

Hm, well, in theory, yes, of course. ;-)

OTOH, given 5 years of technology progress both in hardware and software (e.g. JITted chrome JS), the question arises whether doing all these nifty UI decisions in the backend is still needed for perf reasons - we might as well just only preparse headers into JSON or use jsctypes or something and let the frontend do the filtering.

Thoughts?

Comment 17

[:Hb]

Easiest way with filtering in the frontend/UI is best way.

Comment 18

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.