There are currently two copies of the code to serialize and parse ics data. One in the ics provider, and one in the ics import/export. That's silly, and causes double work.

My first thought was to use the import/export code from the provider, but that didn't work. They just need slightly different things. For example, the provider needs to know the unparsed properties, in order to not loose them when serializing. The importer just drop those properties. So, I created new interfaces. That works quite OK. The messing with the output streams in serializing from the provider isn't really clean, but necko forces me to do it this way.

The patch might look large, but it is mostly copying code. Hopefully, reviewing will not be that hard. It should also fix bug 364388. I also found a but in the provider: when en error occurs on writing the ics file, sunbird closes. The patch also fixes that. (I did not create a new but and patch, because it would conflict with this one). That's why the inLastWindowClosingSurvivalArea related changes are in the patch.

Comment on attachment 249533 [details] [diff] [review] create and use new interfaces First off, I'd like to see the new files named with Ics rather than ICS. We spoke about this in IRC. Secondly, you'll need to add the new component files to /m/c/installer/windows/packages-static. >Index: import-export/calIcsImportExport.js >=================================================================== >+ var parser = Components.classes["@mozilla.org/calendar/ics-parser;1"]. >+ createInstance(Components.interfaces.calIICSParser); Put the trailing dot on the second line please. >+ var serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"]. >+ createInstance(Components.interfaces.calIICSSerializer); Put the trailing dot on the second line please. >+ serializer.addItems(aItems, aItems.length); >+ var str = serializer.serializeToStream(aStream); Why are we assigning this to the variable "str" but not using "str"? Is it because we don't care about what the serializer returns? >Index: base/public/calIICSParser.idl >=================================================================== >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen <mvl@exedo.nl>. Add another space before your name please. Otherwise gerv will come around and do it for you. :) >+ /** >+ * Parse an ics string into its items, and store top-level properties and >+ * components that are not interpreted. >+ * >+ * @param aICSString >+ * The ICS string to parse >+ */ >+ void parseString(in AString aICSString); Should this be AUTF8String since we assume ICS is always in UTF-8? >Index: base/public/calIICSSerializer.idl >=================================================================== >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen <mvl@exedo.nl>. Add another space before your name please. >+/** >+ * An interface for serializing calendar items into an ics string. >+ * Note that this is not a service. A new instance must be created for every new >+ * set of items to be serialized. >+ */ Capitalize ICS and move new to the next line. >+ /** >+ * Add a number of items to the items that are to be serialized. Can be called >+ * multiple times, and appends to the set on every call. "Add some items to the set of items that are to be serialized..." might be a little clearer. I had to read this a couple times to figure out what you meant. >+ /** >+ * Serialize the added items, properties and components into an ics string Capitalize ICS. >+ /** >+ * Serialize the added items, properties and components into an ics stream Capitalize ICS. >Index: base/src/calICSParser.js >=================================================================== >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen <mvl@exedo.nl>. Add another space before your name please. >+calICSParser.prototype.parseString = >+function ip_parseString(aICSString) { >+ icssrv = Components.classes["@mozilla.org/calendar/ics-service;1"] >+ .getService(Components.interfaces.calIICSService); I'd change this variable name to icsSvc. >+ >+ var rootComp = icssrv.parseICS(aICSString); >+ var calComp; >+ // libical returns the vcalendar component if there is just >+ // one vcalendar. If there are multiple vcalendars, it returns >+ // an xroot component, with those vcalendar childs. We need to >+ // handle both. Replace "childs" with "children". Re-wrap the comment if you'd like since it's indented less now. >+calICSParser.prototype.parseFromStream = >+function ip_parseFromStream(aStream) { >+ // Read in the string. Note that it isn't a real string at this point, because >+ // likely, the file is utf8. The multibyte chars show up as multiple 'chars' >+ // in this string. So call it an array of octets for now. >+ Trailing spaces on these 2 lines. ^^ >+ var octetArray = []; >+ var binaryInputStream = Components.classes["@mozilla.org/binaryinputstream;1"] >+ .createInstance(Components.interfaces.nsIBinaryInputStream); Align the dots please. >+ binaryInputStream.setInputStream(aStream); >+ octetArray = binaryInputStream.readByteArray(binaryInputStream.available()); binaryInputStream is also a very verbose variable name. binInputStream or bis seem to make my fingers hurt less. >+ >+ Trailing spaces on these 2 lines. ^^ >+ // Some other apps (most notably, sunbird 0.2) happily splits an utf8 character >+ // between the octets, and adds a newline and space between them, for ics >+ // folding. Unfold manually before parsing the file as utf8. >+ // This is utf8 safe, because octets with the first bit 0 are always one-octet >+ // characters. So the space or the newline never can be part of a multi-byte >+ // char. Rewrap this to be under 80 chars, capitalize Sunbird, ICS and UTF-8. >+ for (var i=octetArray.length-2; i>=0; i--) { Spaces around operators. >+ if (octetArray[i] == "\n" && octetArray[i+1] == " ") { >+ octetArray = octetArray.splice(i, 2); >+ } >+ } >+ >+ // Interpret the byte-array as an utf8 string, and convert into a "...as a UTF-8 string..." >+ // javascript string. >+ var unicodeConverter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); >+ // ics files are always utf8 Capitalize ICS and UTF-8 >Index: base/src/calICSSerializer.js >=================================================================== >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen <mvl@exedo.nl>. Indent your name by two spaces please. >+calICSSerializer.prototype.serializeToString = >+function is_serializeToString() { >+ icssrv = Components.classes["@mozilla.org/calendar/ics-service;1"] >+ .getService(Components.interfaces.calIICSService); >+ var calComp = icssrv.createIcalComponent("VCALENDAR"); Again, I'd use icsSvc as the variable here. >+ calComp.version = "2.0"; >+ calComp.prodid = "-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN"; >+ Trailing spaces on this line. ^^ >+ for each (var prop in this.mProperties) { >+ calComp.addProperty(prop); >+ } >+ for each (var comp in this.mComponents) { >+ calComp.addSubcomponent(comp); >+ } >+ >+ for each (var item in this.mItems) { >+ calComp.addSubcomponent(item.icalComponent); >+ var rec = item.recurrenceInfo; >+ if (rec != null) { >+ var exceptions = rec.getExceptionIds({}); >+ for each (var exid in exceptions) { >+ var ex = rec.getExceptionFor(exid, false); >+ if (ex != null) { >+ calComp.addSubcomponent(ex.icalComponent); >+ } >+ } >+ } >+ } >+ Trailing space on this line. ^^ >+ // do the actual serialization >+ return calComp.serializeToICS(); >+} >+ >+// not prototype.export. export is reserved. I'd replace this comment with "This can't be called 'prototype.export' as 'export' is reserved." >+calICSSerializer.prototype.serializeToStream = >+function is_serializeToStream(aStream) { >+ var str = this.serializeToString(); >+ >+ // Convert the javascript string to an array of bytes, using the >+ // utf8 encoder Capitalize UTF-8 >Index: providers/ics/calICSCalendar.js >=================================================================== >+ var parser = Components.classes["@mozilla.org/calendar/ics-parser;1"]. >+ createInstance(Components.interfaces.calIICSParser); Move the dot to the second line please. >+ // Create a pipe to convert the output stream from the >+ // serializer into an input stream for the upload channel >+ var pipe = Components.classes["@mozilla.org/pipe;1"]. >+ createInstance(Components.interfaces.nsIPipe); Move the dot to the second line and align them please. >+ const PR_UINT32_MAX = 4294967295; // signals "infinite-length" >+ pipe.init(true, true, 0, PR_UINT32_MAX, null); >+ Trailing spaces on this line. ^^ >+ listener.serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"]. >+ createInstance(Components.interfaces.calIICSSerializer); Move the dot to the second line and align them please. Nice work! This is almost entirely style and other nit-picky stuff. r=lilmatt

Comment on attachment 249533 [details] [diff] [review] create and use new interfaces + var serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"]. + createInstance(Components.interfaces.calIICSSerializer); + serializer.addItems(aItems, aItems.length); + var str = serializer.serializeToStream(aStream); return; Why assign to str? +calICSParser.prototype.parseFromStream = +function ip_parseFromStream(aStream) { ... + unicodeConverter.charset = "UTF-8"; + var str = unicodeConverter.convertFromByteArray(octetArray, octetArray.length); +} + Don't you want a parseString() call in here?

patch checked in (with fixes for the review comments)

Since bug 387559 came up I started searching for possible causes in this bug. However, I'm no programmer, just pointing out things which seem weird to me... Want to get bug 387559 solved... You create a pipe: - // do the actual serialization - var icsStream = calComp.serializeToICSStream(); + // Create a pipe to convert the output stream from the + // serializer into an input stream for the upload channel + var pipe = Components.classes["@mozilla.org/pipe;1"]. + createInstance(Components.interfaces.nsIPipe); + const PR_UINT32_MAX = 4294967295; // signals "infinite-length" + pipe.init(true, true, 0, PR_UINT32_MAX, null); + + // Serialize + var icsStream = this.serializer.serializeToStream(pipe.outputStream); Here you define icsStream from the output of the pipe, but why do you try to upload the input of the pipe? Shouldn't you upload the icsStream which was outputted by the pipe? + + // Upload + uploadChannel.setUploadStream(pipe.inputStream, + "text/calendar", -1); - uploadChannel.setUploadStream(icsStream, "text/calendar", - -1); appStartup.enterLastWindowClosingSurvivalArea(); In the code above inLastWindowClosingSurvivalArea was set to false before you try to upload which seems good. Then after the upload goes ok, inLastWindowClosingSurvivalArea is set to true and you issue a exitLastWindowClosingSurvivalArea. But what if the upload throws an exception, inLastWindowClosingSurvivalArea is still false isn't it? Then the exitLastWindowClosingSurvivalArea isn't called, which leads to a hanging process? + var inLastWindowClosingSurvivalArea = false; try { ... + inLastWindowClosingSurvivalArea = true; channel.asyncOpen(savedthis, savedthis); } catch (ex) { - appStartup.exitLastWindowClosingSurvivalArea(); + if (inLastWindowClosingSurvivalArea) { + appStartup.exitLastWindowClosingSurvivalArea(); + } As I said, I'm no programmer.... Also Shouldn't this bug be reopenend?

There is no need for this bug to be reopened. If there are regressions, track them in a separate bug. about the streams: the assignment to icstream is bogus, because the serializeToStream does not return anything. Other than that, I don't see any problems with the code. Note that inputstreams and outputstreams are not as easy to understand as you might think. If setUploadStream itself throws and exception, inLastWCSA is still false, and that's correct. enterLastWCSA was never called. No need to call exitLastWCSA. Note that setUploadStream doesn't actually start the upload. If asyncopen throws, the reverse holds, and that is still what you want. Again, I don't see any problems here.