343677 - Need access to encapsulated MIME content-type declarations for Lightning iMIP

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[cmtalbert]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla Thunderbird version 3 alpha 1 (20060705)

Implementing iMIP (RFC 2447) in Lightning requires that the iMIP code have access to the "METHOD" parameter of the VCALENDAR mime part content-type header. There are several methods that were discussed (via IRC and newsgroup) to obtain this information. The newsgroup post where this was first mentioned can be viewed at:
http://groups.google.com/group/mozilla.dev.apps.thunderbird/browse_thread/thread/cf4f1db94fdedf41/326c52a8c297c07c#326c52a8c297c07c



Reproducible: Always

Steps to Reproduce:
1. Send yourself a meeting request from KOrganizer or Outlook
1a. Note in the message source page you should see a Mime header for the VCALENDAR part that looks like: 
Content-Type: text/calendar;
  charset="utf-8";
  name="cal.ics";
  method="request"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment

BEGIN:VCALENDAR
....
2. This will be inside an interior mime part of the email message.
3. Using all available XPCOM interfaces (nsIMsgDbHdr, nsIMimeHeaders, nsIMsgHeaderParser, etc), attempt to grap the "method" parameter from this content-type declaration.


Actual Results:  
There is no way to grab this information unless you are attempting to do so from within the MIME object system during the time that the MIME message is being parsed. After the message is parsed, you cannot retrieve this information.

Expected Results:  
That there would be some mechanism to obtain this information.

Since then, several approaches have been suggested, and several were tried. 
Here is an overview:

-- Lightning Design Restrictions --
1. Is uses a nsISimpleMimeConverter object in http://lxr.mozilla.org/seamonkey/source/calendar/lightning/components/lightningTextCalendarConverter.js
in order to handle the text/calendar mime type. 
2. Whatever handler that is used to address text/calendar mime MUST re-use the libical library from http://lxr.mozilla.org/seamonkey/source/calendar/libical/ and the VCALENDAR object classes http://lxr.mozilla.org/seamonkey/source/calendar/base/public/ so that calendar code (parsing VCALENDAR objects for instance) is not duplicated.

-- Approaches Attempted --
1. Extend the nsISimpleMimeConverter interface to pass this information to its javascript implementation. 
   A. Pro: Easy
   B. Con: Destroys the genericness of the nsISimpleMimeConverter
   C. (Alternative) Attempted an "initialize" interface on nsISimpleMimeConverter, but could not find a way to pass in the mime part's header block to java script since that it is a C object and not an XPCOM object.

2. Returned to using the C++ text/calendar handler at: http://lxr.mozilla.org/seamonkey/source/mailnews/mime/cthandlers/calendar/mimecal.cpp
    A. Pro: Easy to get the header information
    B. Con: Cannot use the VCALENDAR interfaces and objects (calendar/libical and calendar/base/public) without creating a build-time dependency between calendar and the mime/cthandler component.

3. Using approach number 2, I attempted to use a mime based interface implemented in JS that functioned as a wrapper to the lightningTextCalendarConverter.js code.
    A. Pro: it did actually work (yikes)
    B. Con: way too difficult to maintain/understand/explain
            Why would we have a mime interface that does nothing but special wrapper handling for a specific calendar function? Might as well have the build 
dependency.

4. Attempt to use the existing interfaces for header parsing: nsIMimeHeaders, nsIMsgDbHdr, nsIMsgHeaderParser, direct MIME object access via nsIMimeObjectClassAccess.h (these are in: http://lxr.mozilla.org/seamonkey/source/mailnews/mime/public/)
     Pro: Support already appeared to exist
     Con: These methods only exposed the outer MIME part (i.e. TO: CC: etc), and the direct MIME object accessors were all null when accessed outside of an active MIME parsing session.

5.  Allow Lightning to use its lightningTextCalendarConverter.js for text/calendar support, and put something in the mime code to set nsIMsgDbHdr header properties for the METHOD parameter.
    A. Pro: Very simple solution
            These properties would NOT be set if a text/calendar mime handler is NOT present (i.e. Thunderbird without Lightning).
    B. Con: It is hacky to have a special case for text/calendar support.
            Involves changing core mime code.

After much discussion, we (myself, dmose, and bienvenu) decided to propose approach 5.  Obviously, this is not an optimal solution, and I am open to better ideas. I will propose a patch using approach 5 once my last test build completes.

While this is an enahancement to Thunderbird, it is a critical issue that must be resolved for Lightning.

Comment 1

[cmtalbert]

Attachment: Patch representing approach number 5

This is the patch that was implemented for approach number 5. Note that neither dmose or dbienvenu signed off on this code per se (I didn't want to misrepresent that they had). They agreed in principle to the approach. I chose the mime_find_class function to locate this code in for a few reasons:
1. It already does something simliar for junk mail support
2. It does specific things for various mime types
3. It allowed me a simple location where I could ensure that this code would only be called if an external text/calendar MIME handler was found.

All that said, I'm very open to better ideas. This still feels rather hackish to me.

Comment 2

[Scott MacGregor]

This approach seems fine to me given the complexity of some of the other examples.

On a related note, currently we show the .ics contents as an inline attachment.  I wonder if we shouldn't just be showing it as an external attachment. Seeing "BEGIN:VCALENDAR
VERSION:2.0
X-WR-CALNAME:Test
PRODID:-//Apple Computer\, Inc//iCal 2.0//EN
CALSCALE:GREGORIAN
METHOD:PUBLISH
END:VCALENDAR"

as part of the message body doesn't seem like the right thing to do to me. Thoughts?

Comment 3

[cmtalbert]

> On a related note, currently we show the .ics contents as an inline attachment.
>  I wonder if we shouldn't just be showing it as an external attachment. Seeing

I think you're right, you wouldn't want to show the ics contents in the message. That could be achieved by turning off the "display inline" feature when no external content handler for text/calendar is found.  Should that be added to this patch or should it be handled as a separate bug/feature?

Comment 4

[David :Bienvenu]

Comment on attachment 228201 [details] [diff] [review]
Patch representing approach number 5

+      if (!nsCRT::strncasecmp(content_type, "text/calendar", 13))
+      {
+          char *full_content_type = nsnull;
+          char *imip_method = nsnull;
+          if (hdrs) 
+          {

the if (hdrs) case can be added to the !nsCRT::strncasecmp to save a couple lines of code.

the decl of full_content_type can be delayed until where's it's initialized:

char *full_content_type = MimeHeaders_get

same think for imap_method:

char *imip_method = MimeHeaders_get...

Comment 5

[cmtalbert]

Attachment: An updated patch to address bienvenu's comments

Here is a second patch that addresses David's concerns with the first one.

Comment 6

[David :Bienvenu]

Comment on attachment 228677 [details] [diff] [review]
An updated patch to address bienvenu's comments

great, I'll land this when I get a chance and get it on the 2.0 branch as well.

Comment 7

[David :Bienvenu]

Comment on attachment 228677 [details] [diff] [review]
An updated patch to address bienvenu's comments

actually, I think I'm going to check in something slightly different

if (msgHdr)
  msgHdr->SetStringProperty("imip_method", (imip_method) ? imip_method : "nomethod");
// PR_Free checks for null
PR_Free(imip_method);

Do you really need to set the "nomethod" property or can you deduce it from the empty property? I.e., do you need to distinguish the case where we we've parsed the message with mime from the case where we haven't? That's the only case where we'd have a content type of text/calendar and no imip_method...if not, the code can be even simpler.

Comment 8

[David :Bienvenu]

I was led down the simplification path by thinking that we'll leak imip_method if msgHdr is null, but imip_method is not. Not sure if that can really happen, but it might (e.g., we're parsing a message which contains a forwarded message that has a text/calendar part)

Comment 9

[cmtalbert]

> Do you really need to set the "nomethod" property or can you deduce it from the
> empty property? I.e., do you need to distinguish the case where we we've parsed
> the message with mime from the case where we haven't? That's the only case
> where we'd have a content type of text/calendar and no imip_method...if not,
> the code can be even simpler.

Yes, we need the nomethod attribute because the javascript that uses the property of the message has to be called on the "onEndMessageHeaders" event, which happens for every message. If we just watch for an empty property, then the iMIP stuff will be called into action for every non-iMIP message. I encountered this problem in an early version of the solution, and that is where the "nomethod" attribute came from.  

I guess what this gives us is the ability to handle malformed iMIP headers because not everyone adheres to the standard. 

As far as the changes in the other comment, I'm fine with that. I didn't realize that PR_FREE does a check for null.

Comment 10

[Scott MacGregor]

Attachment: patch as checked in

It looks like this never got checked in. I've updated the patch based on some comments from David at the end of the bug and will land this on the branch and trunk right now.