334468 - no date shown in email meeting requests

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Jason Haar]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: TB 1.5 

When I receive a meeting request, it appears like

Start: 9:00am
End:   9:30am

...along with the "Accept/decline meeting" buttons.

...but there's no DATE! i.e. "Tues 3rd April", etc.

It really needs to show the date. 



Reproducible: Always

Comment 1

[Tony Jacobs]

Attachment: More descriptive implementation of the HTML converter

Replacement version of the Lightning 0.1 version of lightningTextCalendarConverter.js to put a more descriptive blurb in the text panel.

Comment 2

[Tony Jacobs]

Attachment: Email .ics viewer with when/where/who/summary display

After looking at a meeting request and wondering where I was supposed to go, I realized that the last patch didn't include where the event takes place.  I spent a few more minutes on it, and added:
* Where the meeting takes place
* For events that start/stop on the same day, a simplified <date> <from> <to> readout
* Commented out 'Tentative' and 'Reschedule' buttons - they will be needed later
* Formatting for the actual meeting notice.  It's still a big table (now nested).

Comment 3

[Marcel Berteler]

I have exactly the same issues. When can we expect this patch to be included into the CVS version?

Comment 4

[Matthew (lilmatt) Willis]

Comment on attachment 224869 [details] [diff] [review]
Email .ics viewer with when/where/who/summary display

Over to ctalbert, since he's likely done some more work in this area.

Comment 5

[cmtalbert]

*** Bug 337293 has been marked as a duplicate of this bug. ***

Comment 6

[cmtalbert]

This is an unfortunate occurrence. Tony, you and I have duplicated effort, but we have done so in pursuit of the same goal. I have been working on the larger picture of the overall iTIP/iMIP (meeting request) logic in Lightning. In order to properly handle iMIP mime requests in the best way possible, the entire way we handle the mime encoded calendar file underwent some extensive re-design. As a result, the meeting request as displayed by the original LightningTextCalendarConverter.js is no longer necessary -- the buttons had to go. There were also some security issues with including a javascript file in the HTML of the displayed message. Furthermore, the original file (and your patch) is not localizable. 

The new version addresses your specific concers (the lack of "when" and "location " fields) as well as all these other issues. 

I hope to have the pleasure of reviewing your code in the future, and I'm terribly sorry about the collision of effort on this particular bug.

Comment 7

[cmtalbert]

Attachment: New Lightning Mime converter for the improved iMIP support

This is a subset of the larger iTIP/iMIP patch. The full patch resides in Bug 334685.

Comment 8

[cmtalbert]

Attachment: String changes (containing the new HTML) for this patch

For your convenience, these are the string changes that are specific to the LightningTextCalendarConverter.js patch. Note that the HTML is localizable and is contained inside these strings. This is a subset of the full iTIP/iMIP patch that resides in Bug 334685.

Comment 9

[cmtalbert]

Attachment: New patch for MIME HTML generation now using e4x

This is the HTML emitter for the text/calendar mime type that is called when encountering an iMIP/iTIP message. It is based on sipaq's suggested HTML.

Comment 10

[cmtalbert]

Attachment: Sipaq's sample HTML

Comment 11

[cmtalbert]

Attachment: The MIME HTML of a publish iTIP message

Note that no description was specified, and that a common name was provided for the organizer

Comment 12

[cmtalbert]

Attachment: MIME HTML of an iTIP request from Outlook

Note in this picture that all fields were specified and no common name was given for the organizer. The text in the Description field is auto-generated by Outlook. If the sender of this message had actually provided description, then that text would occur after the *~*~*~*~*~*~*~*~ delimiter.

Comment 13

[Matthew (lilmatt) Willis]

Comment on attachment 235188 [details] [diff] [review]
New patch for MIME HTML generation now using e4x

Review commentary via IRC:

[21:04] 	lilmatt	Index: lightning/jar.mn
[21:04] 	lilmatt	 classic.jar:
[21:04] 	lilmatt	 #expand  skin/classic/lightning/lightning.css (themes/__THEME__/lightning.css)
[21:04] 	lilmatt	+#expand  skin/classic/lightning/imip.css        (themes/__THEME__/imip.css)
[21:04] 	lilmatt	Add a space before the parentheses -- I've been trying to align these at 50chars when possible.
[21:04] 	lilmatt	Index: lightning/components/lightningTextCalendarConverter.js
[21:05] 	lilmatt	-/* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
[21:05] 	lilmatt	+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
[21:05] 	lilmatt	Leave this alone. Setting tab to 20chars is so any tabs in the file jump out at you.
[21:05] 	ctalbert	Oh I thought my editor had munged it or something
[21:05] 	lilmatt	No that's why you'll see that in a lot of places. It's a vladism
[21:06] 	ctalbert	ok
[21:06] 	ctalbert	I may have changed them all. I'll have to go back and check
[21:06] 	lilmatt	  * ***** END LICENSE BLOCK ***** */
[21:06] 	lilmatt	-
[21:06] 	lilmatt	 const CI = Components.interfaces;
[21:06] 	lilmatt	Don't remove the blank line, and use Ci (lowercase i) for that constant. Also add Cc = Components.classes and use that to make lines wrap properly
[21:06] 	lilmatt	Cc and Ci are used elsewhere in calendar and in Fx code
[21:07] 	lilmatt	+function createHTMLTableSection(label, text)
[21:07] 	lilmatt	+{
[21:07] 	lilmatt	+    var tblRow = <tr>
[21:07] 	lilmatt	+                    <td class="description">
[21:07] 	lilmatt	+                        <p>{label}</p>
[21:07] 	lilmatt	+                    </td>
[21:07] 	lilmatt	+                    <td class="content">
[21:07] 	lilmatt	+                        <p> {text} </p>
[21:07] 	lilmatt	+                    </td>                    
[21:07] 	lilmatt	You've got lotsa extra spaces at the end of that last line
[21:07] 	lilmatt	Also is there a reason you have spaces around {text} but not around label?
[21:08] 	ctalbert	No reason. I'll remove them. prob. typo
[21:09] 	lilmatt	Shortening var names here would likely make it fit better inside 80chars
[21:09] 	lilmatt	+function getLightningStringBundle()
[21:09] 	lilmatt	+{
[21:09] 	lilmatt	+    var bundleSvc = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(CI.nsIStringBundleService);
[21:09] 	lilmatt	+    var stringBundle;
[21:09] 	lilmatt	+    if (bundleSvc) {
[21:09] 	lilmatt	+        stringBundle = bundleSvc.createBundle("chrome://lightning/locale/lightning.properties");
[21:09] 	lilmatt	+    }
[21:09] 	lilmatt	+    return stringBundle;
[21:09] 	lilmatt	 }
[21:09] 	lilmatt	 
[21:10] 	ctalbert	ok
[21:10] 	lilmatt	+function getLightningStringBundle()
[21:10] 	lilmatt	+{
[21:10] 	lilmatt	+    var svc = Cc["@mozilla.org/intl/stringbundle;1"]
[21:10] 	lilmatt	+              .getService(Ci.nsIStringBundleService);
[21:10] 	lilmatt	+    var sb;
[21:10] 	lilmatt	+    if (bundleSvc) {
[21:10] 	lilmatt	+        sb = svc.createBundle("chrome://lightning/locale/lightning.properties");
[21:10] 	lilmatt	+    }
[21:10] 	lilmatt	+    return sb;
[21:10] 	lilmatt	 }
[21:10] 	lilmatt	 
[21:10] 	lilmatt	Like that ^^^^^^^^^
[21:10] 	ctalbert	cool
[21:11] 	lilmatt	Flush left all the "dump("\n\n **LIGHTNING::Creating HTML \n\n");" stuff so it's easy to pick out and nuke later.
[21:11] 	ctalbert	or just nuke them now :-)
[21:11] 	lilmatt	That too
[21:11] 	lilmatt	+        //html = <!DOCTYPE HTML PUBLIC '-//W3C//DTD HTML 4.01//EN' 'http://www.w3.org/TR/html4/strict.dtd'/>
[21:11] 	lilmatt	Nuke that line. The comment is enough
[21:12] 	lilmatt	+                    <meta http-equiv='Content-Type' content='text/html; charset=iso-8859-1'/>
[21:12] 	lilmatt	Are you using 8859 for a reason here? (instead on UTF8)
[21:12] 	lilmatt	s/on/of
[21:13] 	ctalbert	No reason, I think it was probably in the Html that I copied this from. It should be UTF8 shouldn't it?
[21:13] 	lilmatt	UTF-8
[21:13] 	lilmatt	yes
[21:13] 	lilmatt	+        // Create header row. 
[21:13] 	lilmatt	Trailing space on that line
[21:13] 	lilmatt	Does this ever appear?
[21:13] 	lilmatt	+                    <title>Untitled Document</title>
[21:13] 	lilmatt	If so, it's unlocalizable
[21:14] 	ctalbert	nuked. 
[21:14] 	ctalbert	(it doesn't appear)
[21:14] 	lilmatt	+                    <p class="header"> {labelTxt} </p>
[21:14] 	lilmatt	spaces around braces
[21:14] 	lilmatt	(remove em)
[21:14] 	lilmatt	Two lines later:
[21:14] 	lilmatt	+        );        
[21:15] 	lilmatt	Bunch of trailing spaces on that line.
[21:15] 	lilmatt	+           labelTxt = stringBundle.GetStringFromName("imipHTML.summary");
[21:16] 	lilmatt	These strings should be imipHtml to follow our camelhumps style elsewhere. 
[21:17] 	lilmatt	I'd also find/replace your labelTxt variable name to be "labelText". One character makes it a real word
[21:18] 	lilmatt	+        if (event.title) {
[21:18] 	lilmatt	+           labelTxt = stringBundle.GetStringFromName("imipHTML.summary");
[21:18] 	lilmatt	+           html.body.table.appendChild(createHTMLTableSection(labelTxt, event.title));
[21:18] 	lilmatt	+        }
[21:18] 	lilmatt	The 2 indented lines are only indented by 3 spaces. should be 4
[21:19] 	lilmatt	The next blank line has a bunch of spaces
[21:19] 	lilmatt	(there's a number of those)
[21:19] 	lilmatt	+        var eventLocation =  event.getProperty("LOCATION");
[21:19] 	lilmatt	Two spaces after the equals?
[21:20] 	lilmatt	Remove the "blank" line above if (event.organizer && (event.organ.....
[21:20] 	ctalbert	I'll go through and kill the lines with the blank spaces
[21:21] 	lilmatt	+        dump("\n\n Thisis the HTML: \n" + html + "\n\n");        
[21:21] 	lilmatt	This line has trailing spaces 
[21:21] 	lilmatt	+    }
[21:21] 	lilmatt	+    else {
[21:21] 	lilmatt	no new line before "else"
[21:21] 	ctalbert	you want }else{
[21:21] 	lilmatt	+        var rows = [["Invitation from", "<a href='" + event.organizer.id + "'>" + 
[21:21] 	lilmatt	+                                        event.organizer.commonName + "</a>"],
[21:21] 	lilmatt	+                    ["Topic:", event.title],
[21:21] 	lilmatt	+                    ["Start:", event.startDate.jsDate.toLocaleTimeString()],
[21:21] 	lilmatt	+                    ["End:", event.endDate.jsDate.toLocaleTimeString()]];
[21:21] 	lilmatt	} else {
[21:21] 	lilmatt	um
[21:21] 	lilmatt	I want 
[21:21] 	lilmatt	} else {
[21:22] 	lilmatt	+        var rows = [["Invitation from", "<a href='" + event.organizer.id + "'>" + 
[21:22] 	lilmatt	+                                        event.organizer.commonName + "</a>"],
[21:22] 	lilmatt	+                    ["Topic:", event.title],
[21:22] 	lilmatt	+                    ["Start:", event.startDate.jsDate.toLocaleTimeString()],
[21:22] 	lilmatt	+                    ["End:", event.endDate.jsDate.toLocaleTimeString()]];
[21:22] 	lilmatt	+
[21:22] 	lilmatt	Why do we even have this still? Do we expect to not have a string bundle?
[21:23] 	ctalbert	Dunno. I always feel like components javascript is the red headed step child from hell. I don't expect anything to work. Shall I nuke it?
[21:23] 	lilmatt	I think so.
[21:23] 	ctalbert	I'd love to nuke it. I hate that code
[21:23] 	lilmatt	If it breaks, it's a problem that needs to be fixed.
[21:24] 	lilmatt	Simple is better.
[21:24] 	ctalbert	I like the way you think
[21:25] 	lilmatt	Ok. Down in the mime converter prototype
[21:25] 	ctalbert	yes
[21:25] 	lilmatt	+        var event = Components.classes["@mozilla.org/calendar/event;1"].createInstance(CI.calIEvent);
[21:25] 	lilmatt	Use our new Cc and Ci and wrap that line within 80 chars like the others.
[21:25] 	lilmatt	Actually
[21:26] 	lilmatt	+function createHTMLTableSection(label, text)   should be named Html, not HTML    
[21:26] 	lilmatt	Same in createHTML ()
[21:27] 	ctalbert	changed
[21:27] 	lilmatt	Can you find/replace iTipItem and iTipURI to be itipItem and itipUri ?
[21:27] 	lilmatt	+            var iTipItem = Components.classes["@mozilla.org/calendar/itip-item;1"].createInstance(CI.calIITipItem);
[21:27] 	lilmatt	Use Cc and Ci and line wrap
[21:27] 	ctalbert	actually, I need to just remove itipURI
[21:28] 	lilmatt	I was going to ask about that next
[21:28] 	lilmatt	and this +            //var escapedData = escape(data);
[21:28] 	lilmatt	+            var observerSvc = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
[21:28] 	lilmatt	Use Cc Ci and line wrap, and maybe chose a shorter var name
[21:29] 	lilmatt	+        catch(err) {
[21:29] 	lilmatt	+            dump("\nERROR WITH ITIP ITEM CREATION: " + err + "\n\n");
[21:29] 	lilmatt	Space between catch and (, and I've usually seen exceptions as "e" or "ex" in our code, not "err"
[21:29] 	lilmatt	In both css files:
[21:29] 	lilmatt	+       margin-left: auto; 
[21:29] 	lilmatt	Trailing space
[21:30] 	lilmatt	Close brace shouldn't be indented
[21:30] 	lilmatt	Single space between selector name and open brace
[21:30] 	lilmatt	Add license header
[21:31] 	lilmatt	Use a /* */ style comment for the license header 
[21:31] 	lilmatt	(not #)
[21:31] 	ctalbert	right
[21:31] 	lilmatt	Both css files appear to have no newline at the end. Add one
[21:31] 	ctalbert	should closing braces be left aligned? left edge of file
[21:31] 	lilmatt	Yes
[21:31] 	lilmatt	in css
[21:32] 	lilmatt	+       color: white; 
[21:32] 	lilmatt	Trailing spaces
[21:32] 	lilmatt	+
[21:32] 	lilmatt	+# HTML event display in message
[21:32] 	lilmatt	+imipHTML.header=Event Invitation
[21:32] 	lilmatt	+imipHTML.summary=Title:
[21:32] 	lilmatt	+imipHTML.location=Location:
[21:32] 	lilmatt	+imipHTML.when=When:
[21:32] 	lilmatt	+imipHTML.organizer=Organizer:
[21:32] 	lilmatt	+imipHTML.description=Description:
[21:32] 	lilmatt	+
[21:32] 	lilmatt	Use "imipHtml" like I said before
[21:32] 	ctalbert	gotta change those

Comment 14

[cmtalbert]

Attachment: Patch incorporating lilmatt's comments

This patch addresses Matt's comments on the earlier patch (attachment 235188 [details] [diff] [review]).

Comment 15

[Stefan Sitter [:ssitter]]

Comment on attachment 236349 [details] [diff] [review]
Patch incorporating lilmatt's comments


>+function getLightningStringBundle()
>+{
>+    var svc = Cc["@mozilla.org/intl/stringbundle;1"]
>+              .getService(Ci.nsIStringBundleService);
>+    var sb;
>+    if (svc) {
>+        sb = svc.createBundle("chrome://lightning/locale/lightning.properties");
>+    }
>+    return sb;
>+}

Drive by comment: Instead of adding this helper here how about adding a new helper e.g. ltnGetString() next to function calGetString(aBundleName, aStringName) in calendarUtils.js that loads strings from chrome://lightning/locale/ instead? That helper might be useful in other places too.

Comment 16

[Matthew (lilmatt) Willis]

Comment on attachment 236349 [details] [diff] [review]
Patch incorporating lilmatt's comments

>Index: lightning/components/lightningTextCalendarConverter.js
>===================================================================
>-
>-const CI = Components.interfaces;
>+ 
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
The blank line above const Cc has a trailing space.

>+function createHtmlTableSection(label, text)
>+{
>+    var tblRow = <tr>
>+                    <td class="description">
>+                        <p>{label}</p>
>+                    </td>
>+                    <td class="content">
>+                        <p>{text}</p>
>+                    </td>                    
This line ^^^ has a bunch of trailing spaces

> ltnMimeConverter.prototype = {
>     QueryInterface: function (aIID) {
>-        if (!aIID.equals(CI.nsISupports) &&
>-            !aIID.equals(CI.nsISimpleMimeConverter))
>+        if (!aIID.equals(Ci.nsISupports) &&
>+            !aIID.equals(Ci.nsISimpleMimeConverter))
>             throw Components.interfaces.NS_ERROR_NO_INTERFACE;
You could use Ci here ----^^^^^ but that's just bonus.

>+        try {
>+            dump("\n\n**LIGHTNING::Attempting itipItem\n\n");
Do you need this dump still here? If so, flush left it so we can pull it later.


>Index: lightning/themes/pinstripe/imip.css
>===================================================================
>+ * The Initial Developer of the Original Code is Oracle Corporation
Is this css copied from somewhere else? If not, it's not originally Oracle's.

>+ * Portions created by the Initial Developer are Copyright (C) 2005
Same deal for the date. If you wrote it, it's 2006.


>Index: lightning/themes/winstripe/imip.css
>===================================================================
>+ * The Initial Developer of the Original Code is Oracle Corporation
See above.

>+ * Portions created by the Initial Developer are Copyright (C) 2005
See above.


r1=lilmatt with those nits fixed

Comment 17

[Matthew (lilmatt) Willis]

blocking0.3+ per dmose

We want to improve the iMIP/iTIP story in 0.3

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236349 [details] [diff] [review]
Patch incorporating lilmatt's comments

cal-ui-review+.  We'll want some more design finesse on this before 1.0, but this looks a reasonable direction to be moving in.

In general, the patch looks good.  A few tweaks and questions follow:

>Index: lightning/components/lightningTextCalendarConverter.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/lightning/components/lightningTextCalendarConverter.js,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 lightningTextCalendarConverter.js
>--- lightning/components/lightningTextCalendarConverter.js	21 Aug 2006 17:46:24 -0000	1.3
>+++ lightning/components/lightningTextCalendarConverter.js	1 Sep 2006 02:54:59 -0000
>@@ -1,9 +1,9 @@
>-/* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: 20; c-basic-offset: 4 -*- */

This change looks wrong to me.  What was the problem with the original modeline?


>+function getLightningStringBundle()
>+{
>+    var svc = Cc["@mozilla.org/intl/stringbundle;1"]
>+              .getService(Ci.nsIStringBundleService);
>+    var sb;
>+    if (svc) {

No need for the if, since if there was a problem in the getService, an exception will have been thrown and this code will never be reached.

>+        sb = svc.createBundle("chrome://lightning/locale/lightning.properties");
>+    }
>+    return sb;

Similarly, if we can't create the bundle, something is really wrong, and an exception should be thrown.  So just do

return svc.createBundle(...);

instead of using the temporary.

>+
>+function createHtmlTableSection(label, text)
>+{
>+    var tblRow = <tr>
>+                    <td class="description">
>+                        <p>{label}</p>
>+                    </td>
>+                    <td class="content">
>+                        <p>{text}</p>
>+                    </td>                    
>+                 </tr>

Adding a ; at the end of this (and other statements assigning e4x literals) would be good style.

>+        // Using e4x javascript support here
>+        // which doesn't seem to work with the doctype header

Just prepend the header and/or doctype once you've finished constructing the document using e4x.

>+        if (event.organizer && 
>+           (event.organizer.commonName || event.organizer.id)) {
>+            labelText = stringBundle.GetStringFromName("imipHtml.organizer");
>+            var orgname = (event.organizer.commonName != null) ?
>+                           event.organizer.commonName : event.organizer.id;

var orgname = event.organizer.commonName || event.organizer.id; 

would be a little easier to read, I think.

>+            if (observer) {
>+                observer.notifyObservers(itipItem, "onITipItemCreation", 0);
>+            }

Is it necessary to use an observer-based architecture here to workaround security-context restrictions?  Or could the iMIP bar creation code just be called directly instead?

Comment 19

[cmtalbert]

(In reply to comment #18)
> (From update of attachment 236349 [details] [diff] [review] [edit])
> cal-ui-review+.  We'll want some more design finesse on this before 1.0, but
> this looks a reasonable direction to be moving in.
> 
> In general, the patch looks good.  A few tweaks and questions follow:
> 
> >Index: lightning/components/lightningTextCalendarConverter.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/calendar/lightning/components/lightningTextCalendarConverter.js,v
> >retrieving revision 1.3
> >diff -u -8 -p -r1.3 lightningTextCalendarConverter.js
> >--- lightning/components/lightningTextCalendarConverter.js	21 Aug 2006 17:46:24 -0000	1.3
> >+++ lightning/components/lightningTextCalendarConverter.js	1 Sep 2006 02:54:59 -0000
> >@@ -1,9 +1,9 @@
> >-/* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> >+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: 20; c-basic-offset: 4 -*- */
> 
> This change looks wrong to me.  What was the problem with the original
> modeline?
Matt wanted it changed.
..snip..
> Is it necessary to use an observer-based architecture here to workaround
> security-context restrictions?  Or could the iMIP bar creation code just be
> called directly instead?
> 
Yes, it is necessary for two reasons. The iMIP bar code will only be called after the mime parser has finished parsing. This patch's code is launched by the mime parser and more parsing will be performed after this code returns its HTML to the nsISimpleMimeConverter interface. So, the observer is a way for the iMIP bar code to recieve the VCALENDAR information from the mime layer. The second reason we have to use this roundabout mechanism is because I have not seen a method where an interface from the Components directory can call into javascript inside chrome. Perhaps if there is some lightweight way to do that, we could try that instead. (Using an interface for that was shot down fairly early in the design, but it could be revisited).

I will make the other changes you suggested.

Comment 20

[cmtalbert]

(In reply to comment #15)
> 
> Drive by comment: Instead of adding this helper here how about adding a new
> helper e.g. ltnGetString() next to function calGetString(aBundleName,
> aStringName) in calendarUtils.js that loads strings from
> chrome://lightning/locale/ instead? That helper might be useful in other places
> too.
> 
Stefan,
I'd love to, but it won't help in this patch. This code lives in the components directory and it cannot call anything in Chrome. It can only access internal layers via interfaces.

Comment 21

[cmtalbert]

Attachment: Patch incorporating lilmatt and dmose comments

This patch addresses the most recent comments from Matt and Dan. The only thing that could not be addressed was Dan's idea to prepend the DOCTYPE header after the HTML is constructed. e4x doesn't seem to like the DOCTYPE header literal, it thinks it's improper XML and barfs. The result is that the entire JS file is thrown out of the loader and we get no html Mime whatsoever.  Therefore, I left the doctype header out. Please let me know what you think.

Comment 22

[Matthew (lilmatt) Willis]

Comment on attachment 236993 [details] [diff] [review]
Patch incorporating lilmatt and dmose comments

>Index: lightning/components/lightningTextCalendarConverter.js
>===================================================================
>+        try {
>+dump("\n\n**LIGHTNING::Attempting itipItem\n\n");
>+            var itipItem = Cc["@mozilla.org/calendar/itip-item;1"]
>+                           .createInstance(Ci.calIItipItem);
>+            itipItem.initialize(data);
>+            var observer = Cc["@mozilla.org/observer-service;1"]
>+                           .getService(Ci.nsIObserverService);
>+            if (observer) {
>+                observer.notifyObservers(itipItem, "onITipItemCreation", 0);
>+            }
>+        }
>+        catch (e) {
>+dump("\nERROR WITH ITIP ITEM CREATION: " + e + "\n\n");
>+        }
> 
>-        // dump("Generated HTML:\n\n" + html + "\n\n");
>         return html;
>     },
>-    
> };
Two nits that I missed these before. My apologies.
1. Move the catch up on the same line as the }
2. Lose the trailing comma for javascript strict error checking

r1=lilmatt with those nits

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 236993 [details] [diff] [review]
Patch incorporating lilmatt and dmose comments

Looks good; r=dmose with a few nits fixed:

>+        try {
>+dump("\n\n**LIGHTNING::Attempting itipItem\n\n");

Can we just get rid of this dump?

>+        catch (e) {
>+dump("\nERROR WITH ITIP ITEM CREATION: " + e + "\n\n");

Use Components.utils.reportError() instead of dump() here, if you would.

Can you file a new bug about the issues with multiple windows and then reference it in a comment in the code here?

Additionally, please file a bug against Thunderbird about providing sufficient hooks for fixing this (one or possibly both of tweaking nsISimpleMimeConverter and making the securityInfo stuff more generic).  Please CC [e-mail] and mark it as blocking2.0?

Thanks for the great work!

Comment 24

[cmtalbert]

Attachment: patch with dmose's last comments

This is the same patch as before, just addressing dmose's last comments.
The bug was submitted. It is bug 351610.

Comment 25

[cmtalbert]

Attachment: Didn't see Matt's last comments. Added those too

Whoops! I didn't see Matt's last comments. This now addresses both Matt and Dan's last comments on the patch.

Comment 26

[Matthew (lilmatt) Willis]

Comment on attachment 237029 [details] [diff] [review]
Didn't see Matt's last comments. Added those too

r1=lilmatt 
(You could prob carry the r+s forward and check this in)

Comment 27

[cmtalbert]

Comment on attachment 237029 [details] [diff] [review]
Didn't see Matt's last comments. Added those too

Carry dmose's r+ forward

Comment 28

[Matthew (lilmatt) Willis]

Patch landed on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Congrats!

Comment 29

[Matthew (lilmatt) Willis]

*** Bug 338584 has been marked as a duplicate of this bug. ***