351610 - Hooks needed from mime parser to message window display and nsMsgHeaderSink

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[cmtalbert]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Thunderbird version 3 alpha 1 (20060811)

The problem here is that Lightning's iMIP support must move information from the mime parser (an nsISimpleMimeConverter) to the messageWindow UI display. We currently use the ObserverService to implement that. However, if the user opens multiple iMIP messages, then our calendar information can get associated with the incorrect message window, or fail to be associated at all. We could better solve this problem if we had some way to coordinate what messageWindow (or what message) will be displaying the contents of the mime code we are currently parsing.


Reproducible: Always

Steps to Reproduce:
1.Install Lightning (0.3 or higher) onto Thunderbird
2. Email yourself two iMIP messages (ones with Calendar events attached in text/caelndar mime format)
3. Open a second three pane thunderbird window
4. Select both messages and note how Lightning's iMIP bar will show for one window, not the other, vice versa. 
5. Add the events to your calendars and note that sometimes the event added is not the event that was displayed.

Actual Results:  
iMIP Bar not always displayed
Event to be added to calendar is not the event displayed by the HTML Mime rendering in the message.

Expected Results:  
iMIP bar is always displayed
Correct event is always added to the calendar

There have been several ideas on how to fix this.
1. dmose and mscott talked about using the msgHeaderSink::securityInfo attribute. This attribute would be renamed and changed into a HashArray so that any mime handler could use that as a hook to store valid information that other parts of the code could query and use (in much the same way as SMIME uses it).
2. (Perhaps in addition to step 1) We could also extend nsISimpleMimeConverter so that it has access to the Message URL. From the message URL, the nsISimpleMimeConverter could work through the interfaces to get the messageWindow and therefore, the msgHeaderSink in order to access this new object described in idea 1. This is also very akin to the way SMIME does this, please reference: http://lxr.mozilla.org/mozilla/source/mailnews/mime/src/mimecms.cpp#453 to line 492.

Comment 1

[Scott MacGregor]

Let's see if we can solve this for 2

Comment 2

[David :Bienvenu]

or perhaps we could use nsIPropertyBag2 instead of a hash table so that we really can put arbitrary data in there...I'm probably going to have to play with nsISimpleMimeConverter for an other bug, so I'll try to look at this as well. 

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Yeah, nsIPropertyBag2 sounds like a good plan.

Comment 4

[David :Bienvenu]

Clint (and anyone else who's listening), if I try to whip something up in the next couple days, will you be able to tell me if it's usable for you? We'd like this for our upcoming beta release, if it's still important for you.

Comment 5

[David :Bienvenu]

is there any reason to hijack securityInfo? I'd rather add a new attribute, so I don't have to mess with/break all the users of securityInfo.

Comment 6

[Matthew (lilmatt) Willis]

(In reply to comment #4)
> Clint (and anyone else who's listening), if I try to whip something up in the
> next couple days, will you be able to tell me if it's usable for you? We'd like
> this for our upcoming beta release, if it's still important for you.

It's definitely still important to us. 

Comment 7

[David :Bienvenu]

Attachment: add properties to header sink interface

untested...

Comment 8

[Scott MacGregor]

(In reply to comment #5)
> is there any reason to hijack securityInfo? I'd rather add a new attribute, so
> I don't have to mess with/break all the users of securityInfo.

I think on the trunk, we *might* want to get rid of securityInfo and have the SMIME code go through the property bag. Enigmail might want to do the same thing. The branch we'd want to leave alone of course. 

Comment 9

[David :Bienvenu]

It looks to me like the *only* hook currently available to lightning is the ConvertToHTML method call - I don't believe Lightning's nsISimpleMimeConverter has any other way to get context information. So I could either add a URI parameter to ConvertToHTML, or add a method to nsISimpleMimeConverter to notify it about the URI being processed. It's pretty much the same to me - any preferences, or am I missing something? Either way I'd need to change nsISimpleMimeConverter, and the correspoonding Lightning code. I presume there are no other clients of nsISimpleMimeConverter at the moment...

Comment 10

[David :Bienvenu]

Attachment: proposed fix

this is my proposed fix - lightning will need to implement the uri attribute in its nsISimpleMimeConverter in order to get the uri, but Lightning still works w/o it...

I'd rather worry about converting s/mime to use the writableproperty bag later, after 2.0

Comment 11

[David :Bienvenu]

Comment on attachment 239443 [details] [diff] [review]
add properties to header sink interface

the nsIMimeMiscStatus.idl changes are in both patches, but other than that, they're independent.

Comment 12

[Scott MacGregor]

Comment on attachment 239443 [details] [diff] [review]
add properties to header sink interface

Thanks for doing this. Can you add a comment to nsIMsgHeaderSink documenting the properties attribute (hook for extensions like lightening, enigmail, etc. to pass data from mime to the front end during message display.

Comment 13

[Scott MacGregor]

Comment on attachment 239545 [details] [diff] [review]
proposed fix

Can we add a comment about what the uri in nsISimpleMimeConverter is? (the rdf URI of the message being rendered right?)

Comment 14

[David :Bienvenu]

I'll add some comments when I get to the machine that has these patches.

Comment 15

[David :Bienvenu]

fixed on trunk and branch.

Comment 16

[Scott MacGregor]

clearing blocking flag on a fix which has already landed on the branch.