User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060418 Mozilla Sunbird/0.3a1+ A new interface is needed to handle the iTIP protocol responses and requests. This interface will be responsible for determining the response methods available for a given iTIP method that is received, and will control making those responses by either involving the user or by working automatically from user preferences. The protocol handler will also be responsible for writing additions, changes, and deletions to the proper target calendar that the user has selected. The protocol handler will use the iTIP/iMIP sending interface described in bug 334681. Reproducible: Always Steps to Reproduce: Basic Tests: Ensuring proper methods are used to respond to iTIP requests -- i.e. For a REQUEST the user is allowed to perform one of: REPLY | REQUEST (delegate) | COUNTER Expected Results: Proper action is taken (calls generated to the Sender Interface, proper action taken on the target calendar with regard to the method. Detailed to some degree at: http://wiki.mozilla.org/Calendar:ITIP_and_iMIP_Support

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

This patch contains all changes for iTIP/iMIP. I will break out several of these changes into separate files so that they are easier to review, but for applying the patch, please use this one. Also, since this bug is dependent on Bug 287550, I had to apply that patch first, and was not able to weed out all dependencies in the resulting iTIP/iMIP patch file. So, there are one or two lines which are specifically for the attendee list patch, and should not be considered part of the iTIP/iMIP code.

The base interface and makefile changes extruded from the full patch

This is all the interface changes for the iTIP/iMIP support. Note that this is a subset of the full patch.

Correcting a line feed problem with the diff of calendar/base/src/Makefile.in

Correcting a line feed problem with the diff of calendar/base/src/Makefile.in

As discussed in IRC, I have broken out the iTIP processor set of interfaces from the rest of the patch. This works very closely with the iTIP sender interfaces described in bug 334681. I will post a full patch for all iTIP/iMIP changes to this bug once I incorporate the comments about the Mime HTML handler.

This is a patch containing all the iTIP iMIP pieces for 0.3

This is quite a lot of code to review, so that's really ambitious for 0.3. Nevertheless here are my first comments: First of all some general remarks: - I'm also rather lazy with the 80 chars per line rule, but I think you should follow it. - Some of your files don't have a newline at the end of the file. I'm not sure, if this leads to any problems. - I think your license headers are incomplete, especially the lines between the paragraphs "Software distributed ..." and "Contributor(s) ..." are missing. Please check, if those lines are required. - In your multi-line comments you use the following indentation: /** * text ... */ I actually prefer /** * text ... */ Index: base/public/calIITipItem.idl =================================================================== +* calIITipItem is an interface item that contains a calIITipURI, Doesn't calIITipItem contain icalData and not a calIITipURI? Otherwise I would have expected an attribute calIITipURI. + /** + * Attribute: receivedMethod - method the iTIP item had upon reciept + */ + attribute AUTF8String receivedMethod; Wouldn't receiveMethod be a better name? + /** + * Attribute: autoResponse - whether to respond automatically ("TRUE"), allow user to edit + * the response ("FALSE") or don't respond at all ("NO") + */ + attribute AUTF8String autoResponse; I think the semantics of the values TRUE, FALSE and NO is a little bit strange. What about an attribute response with values NONE, USER, AUTO? + /** + * Modifies the state of the given attendee in the item's ics + *@arg in parameter - AUTF8String containing attendee address + *@arg in parameter - AUTF8String contianing the new attendee status + */ + void setAttendeeStatus(in AUTF8String attendeeID, in AUTF8String status); If the calIITipItem contains a list of calIItemBase, does this method set the status on all calendar items? If so, wouldn't it be better to include a parameter "in calIItemBase item"? + + /** + * Initializes the item with a calIItemBase I think a description of the icalData would be a better choice than calIItemBase. Index: lightning/components/calITipItem.js =================================================================== What is the motivation to implement calIITipItem in JavaScript, but calIITipSender and calIITipResponder in C++? In addition, note that the modifyItem method is not implemented. +/*var calITipItemClassInfo = { + getInterfaces: function (count) { + var ifaces = [ + Components.interfaces.nsISupports, + Components.interfaces.calIITipItem, + Components.interfaces.nsIClassInfo + ]; + count.value = ifaces.length; + return ifaces; + }, + + getHelperForLanguage: function (language) { + return null; + }, + + contractID: "@mozilla.org/calendar/itip-item;1", + classDescription: "Calendar iTIP Item", + classID: Components.ID("{b84de879-4b85-4d68-8550-e0C527e46f98}"), + implementationLanguage: Components.interfaces.nsIProgrammingLanguage.JAVASCRIPT, + flags: 0 +};*/ Why is this commented out? Either add it or remove the comment. + +calITipItem.prototype = { + QueryInterface: function (aIID) { + if (aIID.equals(Components.interfaces.nsIClassInfo)) + return calAttendeeClassInfo; Return calITipItemClassInfo if it's implemented. + get isSend() { + return this.mIsSend; + }, + set isSend(value) { + this.mIsSend = value; + }, mIsSend property missing, add "mIsSend: false," before the getter + get receivedMethod() { + return this.mReceivedMethod; + }, + set receivedMethod(value) { + this.mReceivedMethod = value; + }, mReceivedMethod property missing + get responseMethod() { + return this.mResponseMethod; + }, + set responseMethod(value) { + this.mResponseMethod = value; + }, mResponseMethod property missing + get autoResponse() { + return this.mAutoResponse; + }, + set autoResponse(value) { + this.mAutoResponse = value; + }, mAutoResponse property missing + get targetCalendar() { + return this.mTargetCalendar; + }, + set targetCalendar(value) { + this.mTargetCalendar = value; + }, mTargetCalendar property missing + initialize: function(icalData) { + this.mItemList = new Array(); mItemList property missing + var icsService = Components.classes["@mozilla.org/calendar/ics-service;1"] + .getService(Components.interfaces.calIICSService); indentation + // Try to get the method property out of the calendar to set our methods + try { + var method = calComp.method; + // We set both since we do not know exactly what our state will be. + this.mReceivedMethod = method; + this.mResponseMethod = method; + } Is this approach right for both cases (one or multiple vcalendars)? + while (calComp) { + var subComp = calComp.getFirstSubcomponent("ANY"); + while (subComp) { + switch (subComp.componentType) { + case "VEVENT": + var event = Components.classes["@mozilla.org/calendar/event;1"] + .createInstance(Components.interfaces.calIEvent); + event.icalComponent = subComp; + this.mItemList.push(event); + break; + case "VTODO": + var todo = Components.classes["@mozilla.org/calendar/todo;1"] + .createInstance(Components.interfaces.calITodo); + todo.icalComponent = subComp; + this.mItemList.push(todo); + break; + default: + // Nothing -- Bug xxxx: Implement Freebusy, VJournal etc. + } + subComp = calComp.getNextSubcomponent("ANY"); + } + calComp = rootComp.getNextSubcomponent('VCALENDAR'); + } + this.mIsInitialized = true; mIsInitialized property missing + getFirstItem: function() { + + this.mCurrentItemIndex = 0; mCurrentItemIndex property missing + return this.mItemList[0]; What about the case, that mItemList is empty? + getNextItem: function() { + ++this.mCurrentItemIndex; + if (this.mCurrentItemIndex < this.mItemList.length) { + return this.mItemList[this.mCurrentItemIndex]; + } + else { + return null; + } style: } else { + setAttendeeStatus: function (attendeeID, status) { + // Note that this code forces the user to respond to both items in the same way. + // That is a current limitation of the spec. + attendeeID = "mailto:" + attendeeID; // prepend mailto The handling of "mailto:" looks rather fragile. What about the case, that mailto: is already prepended? + for (var i=0; i < this.mItemList.length; ++i) { + var anItem = this.mItemList[i]; + var attendee = anItem.getAttendeeById(attendeeID); + if (attendee) { + anItem.removeAttendee(attendee); + attendee.participationStatus = status; + anItem.addAttendee(attendee); + } Why is it necessary to call removeAttendee and addAttendee, isn't it enough to change the participation status?

Here are my comments for the rest of the patch. Index: base/public/calIITipResponder.idl =================================================================== +interface calIITipResponder : nsISupports In general I think the naming of the interface methods is suboptimal, but of course that's only a matter of taste. + void recommendNextITipState(inout calIITipItem iTipItem); An alternative method name would be setDefaultResponseMethod. + PRBool checkUserITipPref(in calIITipItem iTipItem); Alternative method names: getUserPreference, getUserITipPreference. + void handleITipAction(in calIITipItem iTipItem, in PRBool userOverride); Alternative method names: handleITipItem, performITipAction. Index: base/src/calITipResponder.h =================================================================== +#ifndef CAL_ITIPRESPONDER_H_ +#define CAL_ITIPRESPONDER_H_ I propose to use CALITIPRESPONDER_H_ (see other header files). +private: + PRBool mIgnorePrefs; + nsCString mAutoResponse; + nsCOMPtr<calICalendar> mTargetCalendar; + nsCString mRecvMethod; + nsCString mRespMethod; As the calITipResponder constructor doesn't take an calITipItem as parameter I assume that a calITipResponder handles several items. You use those members for caching only, that's of course error-prone. Index: base/src/calITipResponder.cpp =================================================================== The whole implementation of calITipResponder seems to be unfinished. Nevertheless I'm a friend of early integration, so that the base functionality can be tested early. Finetuning of the code can be done afterwards, especially as we're far away from a version 1.0. +NS_IMETHODIMP +calITipResponder::CheckUserITipPref(calIITipItem *iTipItem, PRBool *bIsUserInvolved) +{ + NS_ENSURE_ARG_POINTER(bIsUserInvolved); + *bIsUserInvolved = PR_TRUE; + + return NS_OK; +} I guess, the implementation of this method is not finished yet. A comment might be helpful. +NS_IMETHODIMP +calITipResponder::HandleITipAction(calIITipItem *iTipItem, PRBool bUserOverride) The style of this implementation looks a bit messy, please remove outcommented code, add XXX for your todo comments, etc. +nsresult calITipResponder::ProcessPublish(calIItemBase *calItem, + const nsACString &calItemType) Add a comment for TODO. +nsresult calITipResponder::ProcessRequest(calIItemBase *calItem, + const nsACString &calItemType) remove outcommented code + add todo comments +nsresult calITipResponder::VerifyITipStructure(calIItemBase *calItem, + const nsACString &calItemType, + PRInt32 reqState, + nsCString mRespMethod) add todo comment + case UPDATE_OP: + + if (searchallcalendars) { + // do something here + } + else { + //TODO just search targetcalendar and default calendar (if target and default are different) + nsCString id; + calItem->GetId(id); + rv = mTargetCalendar->GetItem(id.get(), calListener); + NS_ENSURE_SUCCESS(rv,rv); + + nsCOMPtr<calIItemBase> oldItem; + rv = calListener->GetItem(getter_AddRefs(oldItem)); How can you ensure, that the listener was already called, so that this call returns the old item? +PRBool calITipResponder::IsAllowableResponse() I would really prefer, if the receive method and the response method are paramters of this method. +/******************************************* +***** ***** +***** iTipCalendarListener Class ***** +***** ***** +********************************************/ In general I think the concept of the iTipCalendarListener is doubtful, especially it's use for UPDATE_OP. Index: base/src/iTipDefines.h =================================================================== +#ifndef ITIP_DEFINES_H_ +#define ITIP_DEFINES_H_ ITIPDEFINES_H_ +// TODO: Shouldn't these be defined somewhere? +#define VEVENT_LITERAL NS_LITERAL_CSTRING("VEVENT") +#define VTODO_LITERAL NS_LITERAL_CSTRING("VTODO") +#define VFREEBUSY_LITERAL NS_LITERAL_CSTRING("VFREEBUSY") +#define VJOURNAL_LITERAL NS_LITERAL_CSTRING("VJOURNAL") +#define CALENDAR_EXTENSION NS_LITERAL_CSTRING(".ics") I agree to your comment, those should be defined somewhere else. +// Operations for calendar action +#define ADD_OP 1 +#define UPDATE_OP 2 +#define DELETE_OP 3 I think those defines need a more speaking name, e.g. CALENDAR_ACTION_ADD.

Comment on attachment 235102 [details] [diff] [review] iTIP Responder processing interfaces only r1- due to the large amount of comments

Comment on attachment 235102 [details] [diff] [review] iTIP Responder processing interfaces only Going to rework this, removing the request for review

This is a block of code just added to this bug for safe keeping. It is not complete. Please do not try to build.

This at least compiles. It has a test function under the Calendar menu to attempt to email, and at this point it is outputting this: ************* Beginning Email Test ************************* HasXpcomMail true sendItems: Sending Email... ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "aItem has no properties" {file: "file:///Users/mwillis/Library/Thunderbird/Profiles/ru3nf0bc.test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calItipEmailService.js" line: 224}]' when calling method: [calIItipEmailService::sendItems]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://lightning/content/lightning-utils.js :: ltnTestEmail :: line 117" data: yes] ************************************************************ ERROR in Email test: [Exception... "'[JavaScript Error: "aItem has no properties" {file: "file:///Users/mwillis/Library/Thunderbird/Profiles/ru3nf0bc.test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calItipEmailService.js" line: 224}]' when calling method: [calIItipEmailService::sendItems]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://lightning/content/lightning-utils.js :: ltnTestEmail :: line 117" data: yes] !!!!!!!!!!!!! Ending Email Test: FAIL!!!!!!!!!!!!!!!!!!!!!!!!!!!

mvl mentioned in some drive-by comments that we didn't need to add .js components to calBaseCID.h, and that he didn't like the int-based preference keys. Both items have been fixed, and I fixed some UNIX/DOS line ending issues that were making the patch more difficult to read than necessary.

This begins wiring up the UI in the iMIP bar. We'll need to get ui-review, but for now this replaces the "Add to Calendar" button in the iMIP bar with (Accept) (Decline). The response isn't actually sent back to the organizer yet.

Requesting ui-review on the proposed iMIP bar UI change

Needs to be merged with attachment 249830 [details] [diff] [review]

still needs to be merged

I believe this merges the UI beginnings with the final email code pieces from the other patch.

Updated merge.

This snapshot now performs correctly given the following scenario: 1. User A uses iCal.app to schedule an event, and sends an invitation to user B. 2. User B, using the latest Thunderbird 2 with Lightning plus this patch receives the invite, and clicks "Accept", and clicks "Send" in the message window that appears. 3. User A receives the reply in iCal.app. There's more to do, as the imip-bar is always on once it appears, and there's lots of cleanup, but this actually interacts properly.

More cleanup and refinement. justdave asked if it worked with Zimbra, so we did a little testing and found two issues: 1. While iCal.app sends one email invite per attendee (with only 1 recipient per message), Zimbra sends one email to all attendees. msgHdr.getRecipients does NOT only give you the email address of the user reading the message, so I had to do a bunch of string manipulation to try and match our email against the defaultIdentity. 2. Zimbra adds RSVP=TRUE to the ATTENDEE on REQUESTs. This is fine, and in spec. However, we must not preserve that in our REPLY. Since we were reusing the same calIAttendee, the RSVP=TRUE remained, and I couldn't remove it with deleteProperty(). So instead I created a new calIAttendee, copied over the necessary info, and put that on the itipItem. 3. I couldn't figure out how to delete a property from a nsIWritablePropertyBag2, so the imip-bar stays up once it's been shown. The deleteProperty() method is only on nsIWritablePropertyBag (no 2).

This is the start of the review process. We want to get review in progress. As of this writing, the "Decline" button doesn't work, we don't track that you've already replied to this particular email, and the imip-bar will not disappear if you are viewing an invitation and click on a different mail folder. This is more to make sure we're not on crack and catch any major architectural errors that we're not seeing.

Here are my comments from a first pass. I tried to focus on the high-level/arch stuff, but I threw in a bunch of little things too. The main points were: -calIItipEmailService seems to be an interface for implementation details, and should probably go -the calIItipTransport __proto__ doesn't feel useful enough to exist separately. -the mapping between the method constants and strings feels awkward. -it'd be nice to find an abstraction level for an invite-interface, to work with the WCAP invitations, and perhaps an eVite provider, etc.

This patch addresses some of the first pass comments. The reworking of calIItipTransport/calIItipEmailService remains, as does looking at the "handleAuto" and friends preferences. The use of constants to represent the 8 iTIP methods was removed. This also allowed me to remove all the string<->int mapping code. This patch doesn't contain any more functionality than attachment 250502 [details] [diff] [review] but I think that was the point. :) More tomorrow.

Comment on attachment 250550 [details] [diff] [review] Addresses some of the first pass comments just drive-by comments, i'm not claiming i have read all of the patch) >Index: calendar/base/src/calItipItem.js >+ init: function ciiI(icalData) { >+ // Fill up the itemList > while (calComp) { The code in this loop doesn't look at exceptions. So any exception will be ignored or otherwise fail. It looks like you really want to use the (not yet checked in) ics parser component from bug 364841. >+ _getCalItemType: function cipGCIT(aCalItem) { Where does the underscore come from? It's not really calendar style, and it's meaningless to me. (the m or a prefixes at least are an abbreviation of their meaning)

Addresses Joey's big comment about the unworkable nature of calIItipEmailService as an interface. Now calIItipTransport is an interface, and there is a calItipEmailTransport that is an object that implements that interface. In light of the fact that we need an invitationManager (follow on bug and proposal to come on that) the imip-bar.js and the calIItipProcessor still instantiate the email piece directly. What those pieces should do instead is ask the invitationManager for the proper ItipTransport interface to use for responding to the given ItipItem.

This version takes into consideration many of jminta's and mvl's review comments. The use of leading underscores in function names is a toolkit style that denotes "private" functions. Whether we use it or not in calendar is subject for a more general discussion than in this bug; perhaps in a phone meeting.

jminta made a good point about the goofiness of the preferences. They may be needed in the final implementation, once we have all the bells and whistles, but we don't need them now, and they just confuse things and add unneccessary complexity at this point. This is _really_ for review. Note that I have commented out the "Decline" button as that isn't entirely working yet. "Accept" is however.

Review comments for attachment #251420 [details] [diff] [review].

>+ // getAttendeeById's matching is done in a case-insensitive manner to handle >+ // places where "MAILTO:" or similar properties are capitalized arbitrarily >+ // by different calendar clients. > calIAttendee getAttendeeById(in AUTF8String id); >I already commented once about how I think this logic belongs in the attendee's .id getter. I don't recall anyone giving me a use-case where someone wouldn't want .toLowerCase() calAttendee's id is now returned toLowerCase(). However, the comparison for getAttendeeById() is done in calItemBase, and so we still need to make sure the passed in id to check against is .toLowerCase()ed > /** > * Initializes the item with an ics string > * @arg - in parameter - AString of ical Data > */ >- void init(in AString icalData); >+ void init(in AUTF8String icalData); >For consistency, wouldn't this be better as > attribute AUTF8String icalString >That's how all of the other interfaces who are expressed in iCalendar data do it. We're leaving this as discussed in IRC. >+ >+ /** >+ * Used to set the address used to obtain invitations, if it is needed >+ * for this transport type. An example would be an IMAP server address. >+ */ >+ attribute AUTF8String incomingServer; >+ >+ /** >+ * Used to set the address used to send invitations and responses, if it >+ * is needed for this transport type. An example would be a SMTP server >+ * address. >+ */ >+ attribute AUTF8String outgoingServer; >This is all stuff for later email support in Sunbird, right? If we're not using it now, let's not include it in this patch. Reviewing code that isn't called and can't be tested is really hard. Removed. >+ * @param dateCutoff only finds emails received since this date. >+ * @return calIItipItem the items it finds in a calIItipItem object. >+ */ >+ calIItipItem checkForItems(in calIDateTime dateCutoff); >"cut off" to me sounds like a deadline/drop-dead date, while this is actually a starting date, as I read it Changed. >+ get type() { >+ return "email"; >+ }, >The idl indicates this is intended to be a user-facing string. It needs to be in a l10n file then. No, type is not user-facing. transportName was. It's been removed since we're not using it. > >+ // XXX Put these in stringbundles >+ var subject = "This is the subject."; >+ var body = "This is the message body."; >Do it. Fixed. We'll need to add more strings for other methods than reply. >+ checkForItems: function cietCFI(aDateCutoff) { >+ // We only need to do perform this check if we do not have XPCOM based >+ // mail interfaces available. >+ if (!this.mHasXpcomMail) { >+ throw Components.results.NS_ERROR_NOT_IMPLEMENTED; >+ } >+ }, >I don't understand this comment at all. The idl makes this method sound like a way for e.g. the front-end, to query this item for changes. In that case, you need to query no matter if you have mail or not. Something needs to be cleared up somewhere. Function renamed and comment clarified. >+ initEmailTransport: function cietIES() { >+ try { >+ if (!this.mAccountMgrSvc) { >+ this.mAccountMgrSvc = Cc["@mozilla.org/messenger/account-manager;1"]. >+ getService(Ci.nsIMsgAccountManager); >+ } >+ >initEmailTransport is only called once, and as early as possible, so all of these if() blocks will always be false. You should remove them. Removed. >+ msgAttachment.name = "calendar.ics"; //calGetString("calendar", "calendarAttachmentName"); >pick one. Done. >+ msgAttachment.url = "file://" + tempIcsFile; >woah... I assumed tempIcsFile would actually be an nsILocalFile. Please rename the variable. Done. >+ var id = calItem.id; >This really seems like superfluous assignment. The first calItem's id is used to generated the tempfile's name. How would you like this changed? > >+ var calendarStream = this._convertFromUnicode("UTF-8", calText); >+ >convertFromUnicode returns an ACString, not a stream of any kind. Variable name changed. >+ // XXX Need to fix before checkin >+ /*if (!getPrefSafe("calendar.itip.email.log", false)) { >+ return; >+ }*/ >Do it. Done. >+/** >+ * Module Registration - this implements the calIItipTransport interface >+ */ >+var calItipEmailTransportModule = { >+ mCID: Components.ID("{d4d7b59e-c9e0-4a7a-b5e8-5958f85515f0}"), >Aren't you already being registered in calItemModule? If you want to register yourself independently, you need to not be in EXTRA_SCRIPTS then. This doesn't need to be there. Removed. >+function NSGetModule(aCompMgr, aFileSpec) { >+ return calMemoryCalendarModule; >+} >That's just plain wrong. :-( Fixed. >>+ implementationLanguage: >+ Components.interfaces.nsIProgrammingLanguage.JAVASCRIPT, >You have Ci available, rather than wrapping to another line. Fixed. >+ var icsService = Cc["@mozilla.org/calendar/ics-service;1"]. >+ getService(Components.interfaces.calIICSService); >+ this.mRootComp = icsService.parseICS(icalData); >icsParser has landed, and you really want it here. Now using it. >+ setAttendeeStatus: function ciiSAS(aAttendeeId, aStatus) { >+ // Append "mailto:" to the attendee if it is missing it. >+ >+//dump("aAttendeeId: " + aAttendeeId + "\n"); >+ var attendeeId = aAttendeeId.toLowerCase(); >+ if (!attendeeId.match(/mailto:/i)) { >+ attendeeId = "mailto:" + attendeeId; >+ } >+//dump("attendeeId: " + attendeeId + "\n"); >+ for each (var item in this.mItemList) { >+ var attendee = item.getAttendeeById(attendeeId); > if (attendee) { >+//dump("updating attendee: " + attendeeId + "\n"); >I don't like landing code like this. Either turn the dump()s on and format them or remove them. dump()s removed. >+function calItipProcessor() { >+ this.wrappedJSObject = this; >+ //this.mIsInitialized = false; >+} >Why isn't mIsInitialized false? It doesn't have an init step, so it doesn't need that. Removed mIsInitialized altogether. >+var calItipProcessorClassInfo = { >Separating out the classInfo module breaks the reflexivity of QI. I can QI from a calIItipProcessor to nsIClassInfo, but not from nsIClassInfo to calIItipProcessor. Once I have the nsIClassInfo, I'm stranded and can't get back to the rest of the methods the object claims to support. If you want to implement nsIClassInfo, it needs to be done as part of the main object. I think I fixed these appropriately. >+ if (aIid.equals(Ci.nsISupports) || >+ aIid.equals(Ci.calIItipProcessor)) >+ { >+ return this; >+ } else { >This all fits on one line. There's no need to wrap or to put the brace in a weird spot. Fixed. >+ // Duplicate the passed itipItem so we have one we feel comfortable >+ // making changes to. >+ var respItipItem = aItipItem >I don't understand this comment at all. For objects, variable assignments are effectively pass-by-reference, so reassigning shouldn't make you feel more comfortable at all. Poor wording. Basically, don't modify passed in variables. Comment changed. > >+ // Sanity checks >+ this.mCalItem = respItipItem.getFirstItem(); >+ if (!this.mCalItem) { >+ return false; >+ } >From the code I've read, getFirstItem will throw, rather than return null, if there's nothing there. Removed. >+ this.mCalItemType = this._getCalItemType(this.mCalItem); >Half of your functions go ahead and read this.mCalItem directly, and half accept it as a parameter. Let's be consistent. I think I've fixed this. >+ while (this.mCalItem) { >+ // Sanity check that mRespMethod is a valid response per the spec. >+ if (!this._isValidResponseMethod()) { >+ return false; >+ } >I don't understand why this check needs to be done on each pass of the loop. Isn't the response method a property of aItipItem, not of any of its individual calIItemBases? Why would it change when we move to the next item? It doesn't. Moved out of the while loop. >+ case "PUBLISH": >+ case "REPLY": >+ case "REFRESH": >+ case "ADD": >+ case "CANCEL": >+ case "COUNTER": >+ case "DECLINECOUNTER": >+ throw Components.results.NS_ERROR_NOT_IMPLEMENTED; >It's not clear to me when you're choosing to simply return false and when you're choosing to throw an error. Throwing is more appropriate rather than dying silently. I'm working on >+ if (transport.type == "email") { >+ me = transport.defaultIdentity; >+ idPrefix = "mailto:"; >+ } else { >+ throw Components.results.NS_ERROR_NOT_IMPLEMENTED; >+ } >defaultIdentity is now a part of every transport, according to your idl, so this check needs to be reworked. How so? It seems to work for me. >+ _getTargetCalendar: function cipGTC() { >+ var calMgr = Components.classes["@mozilla.org/calendar/manager;1"] >+ .getService(Components.interfaces.calICalendarManager); >+ var cals = calMgr.getCalendars({}); >+ for each (var cal in cals) { >+ if (cal.name == this.mTargetCalendar) { >+ return cal; >+ } >+ } >+ >+ // We didn't find a match. >+ return null; >Your other copy of getTargetCalendar() returns the first calendar, this returns something different. Shouldn't you be consistent? Removed the one in processor. We'll use mTargetCalendar (which is a calICalendar) >+ (this.mCalItemType == Ci.calIEvent || >+ this.mCalItemType == Ci.calITodo)))) >Wait, we have _getCalItemType and mCalItemType? Seems redundant to me. _getCalItemType is used to SET mCalItemType. We use mCalItemType so we don't need to check the item type multiple times. >+ var id = aCalItem.GetId(); >+ retVal = targetCalInterface.getItem(id, calListener); >+ if (!retVal) { >+ return false; >+ } >+ >+ var oldItem = calListener.GetItem(); >Wow, this is what I meant about how hard it is to review code that can't be tested. Changed UPDATE and DELETE to throw NYI for now. >I really don't like the "do something here" bits of this code either. Removed. >+ if ((event.organizer) && >+ (event.organizer.commonName || event.organizer.id)) >+ { >I really don't like this. It's uber-hard to follow when your eye is trained to look for the brace at the end of a line everywhere else in the file. Changed to be on one line. >+ orgId = event.organizer.id.substring(7); >Magic numbers need comments. Reworked using .replace(). >+ // Remove the useless "Outlookism" squiggle. >+ var desc = eventDescription.split("*~*~*~*~*~*~*~*~*~*").join(""); >.replace() would be more efficient Fixed. > myFactory: { >- createInstance: function (outer, iid) { >- if (outer != null) >+ createInstance: function mfCI(aOuter, aIid) { >+ if (aOuter != null) { > throw Components.results.NS_ERROR_NO_AGGREGATION; >- >- return (new ltnMimeConverter()).QueryInterface(iid); >+ } >+ return (new ltnMimeConverter()).QueryInterface(aIid); > } > }, >Leak. Removed. >+// iTIP email debugging >+pref("calendar.itip.email.log", true); >No. This should be false. And you should add it to Sunbird pref's too, since you're shipping the code with both. Fixed.

I misunderstood jminta's comments regarding respItipItem = aItipItem; In order to truly not make changes to the passed in calItipItem, we need to clone it. This patch adds clone() to calItipItem and uses it. All previous fixes are also included.

As Joey and I discussed on IRC, this adds a calIOperationListener onto the ItipProcessor so that it can asynchronously notify the imip-bar of the status of the calendar add/update/delete. It also wires in the Decline button functionality, which we originally wanted for this patch.

Comment on attachment 251604 [details] [diff] [review] Adds Decline option and Listener on Processor Switching the r1 request to Daniel.

Comment on attachment 251604 [details] [diff] [review] Adds Decline option and Listener on Processor Clint, I did a quick skin-deep walk thru the patch and commented some code improvements, more error handling and the like. I suspect I am not the right one to judge whether the overall implementation design is good or not, because I am only an itip novice. So I assume the overall design has been reviewed/discussed before. >Index: calendar/base/public/calIItemBase.idl >+ >+ // getAttendeeById's matching is done in a case-insensitive manner to handle >+ // places where "MAILTO:" or similar properties are capitalized arbitrarily >+ // by different calendar clients. > calIAttendee getAttendeeById(in AUTF8String id); Please add docu style comments like /** so we may run doxygen or similar against it. >Index: calendar/base/public/calIItipProcessor.idl >+ * @arg in calIItipItem itipItem A calItipItem to process. I have seen only @param since now. Is @arg also supported by the mozilla docu generator? >Index: calendar/base/public/calIItipTransport.idl >+ /** >+ * Used to indicate an account identifier for this transport >+ * TODO: Is this needed or useful? >+ * TODO: Should this be readonly? >+ */ >+ attribute AUTF8String accountID; I am not the itip expert to judge on that... we should possibly leave it out until it is really needed? >Index: calendar/base/src/calAttendee.js >- icalatt.valueAsIcalString = this.id; >+ icalatt.valueAsIcalString = this.id.toLowerCase(); IMO we should preserve case when storing the attendee id, it may give a better representation for eMail addresses. >Index: calendar/base/src/calItemBase.js > getAttendeeById: function (id) { > var attendees = this.getAttendees({}); >- for (var i = 0; i < attendees.length; i++) >- if (attendees[i].id == id) >- return attendees[i]; >+ for each (var attendee in attendees) { >+ // This match must be case insensitive to deal with differing >+ // cases of things like MAILTO: >+ if (attendee.id == id.toLowerCase()) { >+ return attendee; >+ } >+ } preserving case, I would favor: id = id.toLowerCase(); for each (var attendee in attendees) { if (attendee.id.toLowerCase() == id) return attendee; } What's missing is removeAttendee: var attId = attendee.id.toLowerCase(); for (var i = 0; i < attendees.length; i++) { if (attendees[i].id.toLowerCase() != attId) newAttendees.push(attendees[i]); else found = true; } >Index: calendar/base/src/calItipEmailTransport.js >+ simpleSendResponse: function cietSSR(aItem) { ... >+ // Get my participation status >+ var attendees = item.getAttendees({}); >+ var myPartStat; >+ for (var i=0; i < attendees.length; i++) { >+ if (attendees[i].id == ()) { >+ myPartStat = attendees[i].participationStatus; >+ break; >+ } >+ } Why not use calIItemBase::getAttendeeById here? var att = item.getAttendeeById("mailto:" + this.mDefaultIdentity.email); if (!att) // missing error case, please be error-tolerant, e.g. quit this response... var myPartStat = att.participationStatus; >+ var subj = sb.formatStringFromName("itipReplySubject", >+ [item.getProperty("SUMMARY")], 1); What if SUMMARY property isn't in? Is it mandatory? Either case, please be error-tolerant. >+ var recipients = [item.organizer]; Again, please be error-tolerant and check for organizer property, e.g. quit this response... >+ this.sendItems(recipients.length, recipients, subj, body, aItem); >+ }, >+ _sendXpcomMail: function cietSXM(aCount, aToList, aSubject, aBody, aItem) { >+ msgAttachment.url = "file://" + tempIcsFileUrl; cleaner: let _createTempIcsFile return a real url >+ // Strip leading "mailto:" if it exists. ... if (toList.length > 0) // to avoid trailing comma >+ toList += rId + ","; >+ _createTempIcsFile: function cietCTIF(aItem) { ... >+ var profilePath = dirUtils.get("TmpD", Ci.nsIFile).path; profilePath seems to be a leftover from previous code iterations, isn't it? use the returned file object straight, no need to create an extra local file to be initialized. >+ file = Cc["@mozilla.org/file/local;1"]. >+ createInstance(Ci.nsILocalFile); >+ file.initWithPath(profilePath); so remove those two >+ file.append("tempfile" + id + ".ics"); file.append("itipTemp") // or similar and use file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0600); >+ var outputStream = Cc["@mozilla.org/network/file-output-stream;1"]. >+ createInstance(Ci.nsIFileOutputStream); >+ var unicodeCalText = this._convertFromUnicode("UTF-8", calText); >+ >+ // Let's write the file - constants from file-utils.js >+ const MODE_WRONLY = 0x02; >+ const MODE_CREATE = 0x08; >+ const MODE_TRUNCATE = 0x20; >+ outputStream.init(file, >+ MODE_WRONLY | MODE_CREATE | MODE_TRUNCATE, >+ 0600, 0); >+ outputStream.write(unicodeCalText, unicodeCalText.length); >+ outputStream.close(); >+ } catch (ex) { >+ LOG("createTempIcsFile failed! " + ex); >+ file = null; >+ } >+ LOG("createTempIcsFile file.path: " + file.path); >+ return file.path; - missing check for !file - why not use a converter-outputstream? - file.path is platform dependent! so better do if (!file) throw ... var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService); return ioService.newFileURI(file).spec; >Index: calendar/base/src/calItipItem.js >+ clone: function ciiC() { >+ // Copy over the exposed attributes. >+ newItem.receivedMethod = this.mReceivedMethod; >+ newItem.responseMethod = this.mResponseMethod; >+ newItem.autoResponse = this.mAutoResponse; >+ newItem.targetCalendar = this.mTargetCalendar; >+ newItem.localStatus = this.mLocalStatus; >+ newItem.isSend = this.mIsSend; I would prefer using the attribute getters here, ie newItem.isSend = this.isSend; Less coupling to the underlying implementation of attribute isSend avoids any changes here if eg mIsSend isn't used anymore; and once more the getter implementations are tested when clone() is called. >+ getFirstItem: function ciiGFI() { ... > this.mCurrentItemIndex = 0; >+ getNextItem: function ciiGNI() { ... > ++this.mCurrentItemIndex; ... As already noted in the idl: "Bug XXX 351761: Need to find a way to make this use an nsISimpleEnumerator" This interface cannot be used concurrently. You should change that early before any code relies on that interface. >+ setAttendeeStatus: function ciiSAS(aAttendeeId, aStatus) { >+ // Append "mailto:" to the attendee if it is missing it. >+ var attendeeId = aAttendeeId.toLowerCase(); You shouldn't introduce similar variable names, why not just if (!aAttendeeId.toLowerCase().match(/mailto:/i)) { aAttendeeId = "mailto:" + aAttendeeId; } ... var attendee = item.getAttendeeById(aAttendeeId); >+ // Replies should not have the RSVP property. >+ // Since attendee.deleteProperty("RSVP") doesn't work, we must >+ // create a new attendee from scratch WITHOUT the RSVP >+ // property and copy in the other relevant data. ... btw: I fixed calAttendee in bug 358498, Matt still has to review it. deleteProperty ought to work then. >Index: calendar/base/src/calItipProcessor.js >+// Operations on the calendar >+const ADD_OP = 1; >+const UPDATE_OP = 2; >+const DELETE_OP = 3; Due to the fact that this js file is loaded with lots of other base components, I would prefer to have these constants prefixed eg ITIP_PROC_ADD_OP or similar. >+function calItipProcessor() { >+ mIsUserInvolved: false, >+ mRecvMethod: null, >+ mRespMethod: null, >+ mAutoResponse: null, >+ mTargetCalendar: null, >+ mCalItem: null, >+ mCalItemType: null, The itip processor's implementation is stateful. Reading the idl, IMO users are tempted to use it as a (shared) service, which I would appreciate BTW. Moreover, I see no strong reason to keep that processing data as member of the processor object. All that helper (member) functions can get those by argument. >+ processItipItem: function cipPII(aItipItem, aListener) { >+ // Sanity check the input >+ if (!aItipItem) { >+ throw new Error ("processItipItem: " + >+ "Invalid or non-existant itipItem passed in."); better: throw new Components.Exception("processItipItem: " + "Invalid or non-existant itipItem passed in.", Components.results.NS_ERROR_INVALID_ARG); >+ // Clone the passed in itipItem like a sheep. Dolly! >+ var respCalItem = respItipItem.getFirstItem(); >+ while (respCalItem) { >+ var attendees = respCalItem.getAttendees({}); >+ >+ for each (var attendee in attendees) { >+ // Leave the ORGANIZER alone. >+ if (!attendee.isOrganizer) { >+ // example: mailto:joe@domain.com >+ var meString = idPrefix + me; >+ if (attendee.id.toLowerCase() != meString.toLowerCase()) { >+ respCalItem.removeAttendee(attendee); >+ } >+ } >+ } >+ >+ respCalItem = respItipItem.getNextItem(); >+ } Hmm. IMO there is no ORGANIZER in the calIItemBase attendee list; it is stored separately. So why not simplify the whole strip down to var meAtt = respCalItem.getAttendeeById(idPrefix + me); if (!meAtt) // error... respCalItem.removeAllAttendees(); respCalItem.addAttendee(meAtt); >+ _processCalendarAction: function cipPCA(aCalItem, aOperation, aListener) { >+ // XXX TODO: Implement a real listener that verifies the operation >+ // completed successfully. >+// var calListener = null; as the listener is passed in, this comment isn't valid anymore, isn't it? >+ _getReplyStatus: function cipGRS(aCalItem, aAttendeeId) { >+ var attendees = aCalItem.getAttendees({}); >+ var idPrefix = "mailto:"; >+ var replyStatus; >+ >+ for each (var attendee in attendees) { >+ // example: mailto:joe@domain.com >+ var idString = idPrefix + aAttendeeId; >+ if (attendee.id.toLowerCase() == idString.toLowerCase()) { >+ replyStatus = attendee.participationStatus; >+ break; >+ } >+ } use calIItemBase::getAttendeeById >Index: calendar/lightning/components/lightningTextCalendarConverter.js > function createHtmlTableSection(label, text) >+ if (event.organizer && (event.organizer.commonName || event.organizer.id)) { >+ // Trim any instances of "mailto:" for better readibility. >+ var orgId = event.organizer.id.replace(/mailto:/ig, ""); error if no organizer.id >+ set uri(aUri) { >+ this.mUri = aUri; >+ return this.mUri; >+ }, for brevity, just return (this.mUri = aUri); (same for lots of other attribute setters in the patch) >Index: calendar/lightning/content/imip-bar.js >+ // calIOperationListener so that we can properly return status to the >+ // imip-bar >+ var operationListener = { >+ onOperationComplete: >+ function (aCalendar, aStatus, aOperationType, aId, aDetail) { make unanonymous >Index: calendar/lightning/content/lightning.js >+ >+// iTIP email debugging >+pref("calendar.itip.email.log", false); IMO we shouldn't predefine debug/logging prefs. >Index: calendar/sunbird/app/profile/sunbird.js >+pref("calendar.itip.email.log", false); same here

(In reply to comment #38) > >Index: calendar/base/public/calIItemBase.idl > Please add docu style comments like /** so we may run doxygen or similar > against it. Since we only changed one attribute, I only fixed it on that one so as to not hose cvs blame. > >Index: calendar/base/public/calIItipProcessor.idl > >+ * @arg in calIItipItem itipItem A calItipItem to process. > I have seen only @param since now. Is @arg also supported by the mozilla docu > generator? Switched @arg to @param > >Index: calendar/base/public/calIItipTransport.idl > I am not the itip expert to judge on that... we should possibly leave it out > until it is really needed? Removed for now. > >Index: calendar/base/src/calAttendee.js > >- icalatt.valueAsIcalString = this.id; > >+ icalatt.valueAsIcalString = this.id.toLowerCase(); > > IMO we should preserve case when storing the attendee id, it may give a better > representation for eMail addresses. jminta asked for this to be done. I agree that email addresses should be in lowercase. > var att = item.getAttendeeById("mailto:" + this.mDefaultIdentity.email); > if (!att) // missing error case, please be error-tolerant, e.g. quit this > response... > var myPartStat = att.participationStatus; This is a more frequent issue that you might initially think. I've added comments to the code about why, and we can determine what we want to do in the future. For now, we'll just not send a response email. > >+ var subj = sb.formatStringFromName("itipReplySubject", > >+ [item.getProperty("SUMMARY")], 1); > What if SUMMARY property isn't in? Is it mandatory? Either case, please be > error-tolerant. It's not mandatory. Now error tolerant. > >+ var recipients = [item.organizer]; > Again, please be error-tolerant and check for organizer property, e.g. quit > this response... Organizer _is_ mandatory. > >+ _sendXpcomMail: function cietSXM(aCount, aToList, aSubject, aBody, aItem) { > >+ msgAttachment.url = "file://" + tempIcsFileUrl; > cleaner: let _createTempIcsFile return a real url Done. > >+ // Strip leading "mailto:" if it exists. > ... > if (toList.length > 0) // to avoid trailing comma > >+ toList += rId + ","; Fixed so we don't add unneccesary commas. > >Index: calendar/base/src/calItipItem.js > >+ clone: function ciiC() { > >+ // Copy over the exposed attributes. > >+ newItem.receivedMethod = this.mReceivedMethod; > >+ newItem.responseMethod = this.mResponseMethod; > >+ newItem.autoResponse = this.mAutoResponse; > >+ newItem.targetCalendar = this.mTargetCalendar; > >+ newItem.localStatus = this.mLocalStatus; > >+ newItem.isSend = this.mIsSend; > I would prefer using the attribute getters here, ie > newItem.isSend = this.isSend; > Less coupling to the underlying implementation of attribute isSend avoids any > changes here if eg mIsSend isn't used anymore; and once more the getter > implementations are tested when clone() is called. Done. > >+ setAttendeeStatus: function ciiSAS(aAttendeeId, aStatus) { > >+ // Append "mailto:" to the attendee if it is missing it. > >+ var attendeeId = aAttendeeId.toLowerCase(); > You shouldn't introduce similar variable names, why not just > > if (!aAttendeeId.toLowerCase().match(/mailto:/i)) { > aAttendeeId = "mailto:" + aAttendeeId; > } Doing so would modify a passed in parameter, which is verbotten. Instead I've changed the variable name to be less similar. > >Index: calendar/base/src/calItipProcessor.js > Due to the fact that this js file is loaded with lots of other base components, > I would prefer to have these constants prefixed eg ITIP_PROC_ADD_OP or similar. Done. > >+ processItipItem: function cipPII(aItipItem, aListener) { > >+ // Sanity check the input > >+ if (!aItipItem) { > >+ throw new Error ("processItipItem: " + > >+ "Invalid or non-existant itipItem passed in."); > better: throw new Components.Exception("processItipItem: " + > "Invalid or non-existant itipItem passed > in.", > Components.results.NS_ERROR_INVALID_ARG); Done. > >+ var respCalItem = respItipItem.getFirstItem(); > >+ while (respCalItem) { > >+ var attendees = respCalItem.getAttendees({}); > >+ > >+ for each (var attendee in attendees) { > >+ // Leave the ORGANIZER alone. > >+ if (!attendee.isOrganizer) { > >+ // example: mailto:joe@domain.com > >+ var meString = idPrefix + me; > >+ if (attendee.id.toLowerCase() != meString.toLowerCase()) { > >+ respCalItem.removeAttendee(attendee); > >+ } > >+ } > >+ } > >+ > >+ respCalItem = respItipItem.getNextItem(); > >+ } > Hmm. IMO there is no ORGANIZER in the calIItemBase attendee list; it is stored > separately. > So why not simplify the whole strip down to > > var meAtt = respCalItem.getAttendeeById(idPrefix + me); > if (!meAtt) // error... > respCalItem.removeAllAttendees(); > respCalItem.addAttendee(meAtt); The organizer is indeed in the attendee list, and is only special because they have isOrganizer set to true. > >+// var calListener = null; > as the listener is passed in, this comment isn't valid anymore, isn't it? Removed. > >Index: calendar/lightning/components/lightningTextCalendarConverter.js > > function createHtmlTableSection(label, text) > >+ if (event.organizer && (event.organizer.commonName || event.organizer.id)) { > >+ // Trim any instances of "mailto:" for better readibility. > >+ var orgId = event.organizer.id.replace(/mailto:/ig, ""); > error if no organizer.id Fixed. > >+ set uri(aUri) { > >+ this.mUri = aUri; > >+ return this.mUri; > >+ }, > for brevity, just > return (this.mUri = aUri); > (same for lots of other attribute setters in the patch) Fixed. > >Index: calendar/lightning/content/imip-bar.js > >+ var operationListener = { > >+ onOperationComplete: > >+ function (aCalendar, aStatus, aOperationType, aId, aDetail) { > make unanonymous Done. > >+pref("calendar.itip.email.log", false); > IMO we shouldn't predefine debug/logging prefs. Removed in both places. There were a couple other more major comments daniel made that I haven't addressed. I am planning to do so once I can discuss them with Clint and before this is submitted for r2. I'm attaching this more to get daniel's feedback (hence re-requesting r1) on the answers and changes I've made (and to back up the code) ;)

(In reply to comment #39) > Created an attachment (id=253426) [details] > Addresses many of daniel's comments > > >Index: calendar/base/src/calAttendee.js > > >- icalatt.valueAsIcalString = this.id; > > >+ icalatt.valueAsIcalString = this.id.toLowerCase(); > > > > IMO we should preserve case when storing the attendee id, it may give a better > > representation for eMail addresses. > jminta asked for this to be done. I agree that email addresses should be in > lowercase. Even if jminta asked for this, please provide a sensible reason why those eMail-addresses need to be modified. I would appreciate attendee entries with case as specified, e.g. "Daniel Boelzle <Daniel.Boelzle@Sun.COM>". Attendee id comparison can ignore case, so what's the problem? > > >+ setAttendeeStatus: function ciiSAS(aAttendeeId, aStatus) { > > >+ // Append "mailto:" to the attendee if it is missing it. > > >+ var attendeeId = aAttendeeId.toLowerCase(); > > You shouldn't introduce similar variable names, why not just > > > > if (!aAttendeeId.toLowerCase().match(/mailto:/i)) { > > aAttendeeId = "mailto:" + aAttendeeId; > > } > Doing so would modify a passed in parameter, which is verbotten. > Instead I've changed the variable name to be less similar. That's not true. String objects are immutable. The parameter can be reused without affecting the outer (initially referenced) string object. > > >+ var respCalItem = respItipItem.getFirstItem(); > > >+ while (respCalItem) { > > >+ var attendees = respCalItem.getAttendees({}); > > >+ > > >+ for each (var attendee in attendees) { > > >+ // Leave the ORGANIZER alone. > > >+ if (!attendee.isOrganizer) { > > >+ // example: mailto:joe@domain.com > > >+ var meString = idPrefix + me; > > >+ if (attendee.id.toLowerCase() != meString.toLowerCase()) { > > >+ respCalItem.removeAttendee(attendee); > > >+ } > > >+ } > > >+ } > > >+ > > >+ respCalItem = respItipItem.getNextItem(); > > >+ } > > Hmm. IMO there is no ORGANIZER in the calIItemBase attendee list; it is stored > > separately. > > So why not simplify the whole strip down to > > > > var meAtt = respCalItem.getAttendeeById(idPrefix + me); > > if (!meAtt) // error... > > respCalItem.removeAllAttendees(); > > respCalItem.addAttendee(meAtt); > The organizer is indeed in the attendee list, and is only special because they > have isOrganizer set to true. I consider this being a bug, because rfc2445 clearly separates ORGANIZER and ATTENDEE. Thus I wouldn't expect an calIAttendee with isOrganizer==true in the attendee list, but only in item.organizer. BTW: IMO the modelling of calIItemBase::organizer as calIAttendee is wrong and misleading; it should have been calIAddress (or similar), because ORGANIZER's and ATTENDEE's value type is "CAL-ADDRESS" (4.8.4.1). > I'm attaching this more to get daniel's feedback (hence re-requesting r1) on > the answers and changes I've made (and to back up the code) ;) Before I review the patch again, let's first sort out the above points.

This addresses the issues w.r.t. preserving the case of email addresses. The issue w.r.t. to parameter handling will remain unchanged in the patch (in order to maintain code consistency) We will file a follow-on bug for the organizer vs attendee issue.

By accident I found ctalbert's email address misspelled in the license header of calIItipItem.idl: + * Clint Talbert <ctalbert.mozt@gmail.com>

:( Hmm. there are quite lots of comments I made in the first review round that haven't been fixed nor explained/justified in the bug, e.g. buggy calItemBase::removeAttendee, temp file creation, ... Comment on these nits first, please; also if you think a nit is invalid. Moreover, due to the puggable architecture here for itip transports, shouldn't we put them into a different directory, e.g. calendar/itip-transports/email etc.

(In reply to comment #43) Daniel, I didn't realize this. I thought your comments had been addressed in the latest patch I modified. I apologize, and will address this.

We've decided this will block Ln 0.5

Daniel, this should address everything, except one part that I'm not convinced we need to do. Let me go through some specifics: > As already noted in the idl: "Bug XXX 351761: Need to find a way to make this > use an nsISimpleEnumerator" > This interface cannot be used concurrently. You should change that early before > any code relies on that interface. Point taken, however, on looking at that interface again, I saw no reason for it to return an nsISimpleEnumerator. It could just return a normal array. So, I had it return a normal array. That way we avoid the array to enumerator support that Joey worked out for us. Without a *really* good reason, I don't want to do that. And I could find no good reason why this couldn't just return an array. > ...temp file... I took all your ideas about the temp file stuff and implemented them. > ... directory for calIItipTransport implementations I spoke with mvl about this, and he concurred it would be a good idea to separate the calIItipTransport implementations into their own directories, but he thought that since these implementations are contained within one file, it should only be one directory off calendar. I suggested some names, and calendar/itip won. That directory has now been cvs added to the repository, and is contained in this diff. So the Email transport implementation lives at: mozilla/calendar/itip/calItipEmailTransport.js. Other transports will live at: m/c/itip/calItipOtherTransport.js > The itip processor's implementation is stateful. Reading the idl, IMO users > are > tempted to use it as a (shared) service, which I would appreciate BTW. > Moreover, I see no strong reason to keep that processing data as member of the > processor object. All that helper (member) functions can get those by > argument. Here is where we disagree. There is no way that a caller function can retrieve those attributes -- they are not defined in the IDL. The only public interface into the calIItipProcessor interface is ProcessItipItem. So, while the calItipProcessor does have internal states, I don't see how that matters w.r.t. calling clients. If another client were to call ProcessItipItem, that client would send in all this information encapsulated in a calItipItem and these variables would be overwritten with the new information. Perhaps this is something about JS that I'm not familiar with. Those variables are intended to be private members on the implementation. In C++ those variables were private member variables on the implementation. I think having them as such will help keep the functions simpler as we increase complexity by supporting more of the iTIP state machine. Elsewhere, I corrected the case comparisons for attendees, and tried to ensure that we are using getAttendeeById everywhere that it makes sense to do so. The reason that the "mailto" prefix has to be handled outside of calIItemBase right now is that we really need to look into how we want to handle non-email address attendees and address that. I think there may need to be a another "type" attribute on the attendees (maybe an X-PARAM?) so that we can tell what type of attendee ID we have. MAILTO is fine for email addresses, but if we used some other type then we would not want mailto prefixes. Since this will require some work across the product, we'll file a follow-on bug for that. There was also a recent post about this to the calsify mailing list. I'll follow up there as well to see what they decided.

(In reply to comment #47) > > The itip processor's implementation is stateful. Reading the idl, IMO users > > are > > tempted to use it as a (shared) service, which I would appreciate BTW. > > Moreover, I see no strong reason to keep that processing data as member of the > > processor object. All that helper (member) functions can get those by > > argument. > Here is where we disagree. There is no way that a caller function can retrieve > those attributes -- they are not defined in the IDL. The only public interface > into the calIItipProcessor interface is ProcessItipItem. So, while the > calItipProcessor does have internal states, I don't see how that matters w.r.t. > calling clients. If another client were to call ProcessItipItem, that client > would send in all this information encapsulated in a calItipItem and these > variables would be overwritten with the new information. Perhaps this is > something about JS that I'm not familiar with. Those variables are intended to > be private members on the implementation. > > In C++ those variables were private member variables on the implementation. I > think having them as such will help keep the functions simpler as we increase > complexity by supporting more of the iTIP state machine. My point wasn't about that clients could access that state information, but about that they are tempted to use the processor object in a shared fashion, which has been my first impression: a clean static stateless object I just get once via getService() and use forever. Any runtime parameters I pass to it upon processItipItem are thread-local. Moreover, I really didn't see any urging reason why to save the runtime parameters at the processor object, than just convenience /no need to explicitly pass them to the inner helper functions. My above comment is just a recommendation. If you stick to the current implementation, then please comment in the IDL that users must not use processors as a service; that the object is stateful.

Comment on attachment 257454 [details] [diff] [review] Addresses all of Daniel's comments >Index: calendar/base/public/calIItipItem.idl > attribute PRBool isSend; >Index: calendar/base/public/calIItipProcessor.idl >+ PRBool processItipItem(in calIItipItem itipItem, in calIOperationListener aListener); why PRBool in favor of boolean? >Index: calendar/base/public/calIItipTransport.idl >Index: calendar/base/src/calItemBase.js > getAttendeeById: function (id) { > var attendees = this.getAttendees({}); >- for (var i = 0; i < attendees.length; i++) >- if (attendees[i].id == id) >- return attendees[i]; >+ for each (var attendee in attendees) { >+ // This match must be case insensitive to deal with differing >+ // cases of things like MAILTO: >+ if (attendee.id.toLowerCase() == id.toLowerCase()) { >+ return attendee; >+ } >+ } minor opt: take var lowerCasedId = id.toLowerCase(); out of loop... > removeAttendee: function (attendee) { > this.modify(); > var found = false, newAttendees = []; > var attendees = this.getAttendees({}); > for (var i = 0; i < attendees.length; i++) { >- if (attendees[i].id != attendee.id) >+ if (attendees[i].id.toLowerCase() != attendee.id.toLowerCase()) same for attendee.id.toLowerCase() here >Index: calendar/lightning/content/imip-bar.js >+function doResponse(aLocalStatus) > { >- // XXX For now, just add the item to the calendar >+ // calIOperationListener so that we can properly return status to the >+ // imip-bar >+ var operationListener = { >+ onOperationComplete: >+ function ooc(aCalendar, aStatus, aOperationType, aId, aDetail) { >+ // Call finishItipAction to set the status of the operation >+ finishItipAction(aOperationType, aStatus, aDetail); >+ }, >+ >+ onGetResult: >+ function ogr(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) { >+ // no-op >+ } >+ }; >+ > // The spec is unclear if we must add all the items or if the > // user should get to pick which item gets added. > var cal = getTargetCalendar(); > >- var item = gItipItem.getFirstItem(); >- while (item != null) { >- if (eventStatus.length) >- item.status = eventStatus; >- cal.addItem(item, null); >- item = gItipItem.getNextItem(); >+ if (aLocalStatus != null) { >+ gItipItem.localStatus = aLocalStatus; > } >+ >+ var itipProc = Cc["@mozilla.org/calendar/itip-processor;1"]. >+ createInstance(Ci.calIItipProcessor); >+ >+ itipProc.processItipItem(gItipItem, operationListener); > } I am trying to understand whether it is ok not to wait for the addItem call to be finished before processing the itip item which causes an addItem call, too. I am thinking of the async providers. Maybe you can elaborate a bit on this. However (without further debugging), I think that [http://lxr.mozilla.org/mozilla/source/calendar/base/src/calIcsParser.js#133] is a bug worth to be filed. Does this cause harm to your itip items? I see the ics parser being used in init(). r=dbo; further fixes/improvements in upcoming bugs/patches.

This version fixes the two optimizations that Daniel pointed out. The code about addItem(calendar,null) is removed from the patch, and I debugged and did not see any issues with that area of calIcsParser and iTIP interaction. Carrying Daniel's r+ forward. We will be addressing several further iTIP issues/expanding iTIP support in follow on bugs.

Checked in on MOZILLA_1_8_BRANCH and trunk --> FIXED Filed Bug 373073 to address the statefulness issue w.r.t. calItipProcessor's implementation.

VERIFIED with Tb 2.0.0.4pre & Lightning 0.5pre (2007052205) on Windows XP.