After the patch for bug 311405 landed, I started seeing occasional assertions and failures to upload as documented in comment 5 of that bug. It turns out that in an earlier review, I recommended using nsIPipe without reading the comments in nsIUploadChannel carefully enough. It turns out that nsIUploadChannel requires stream implementors that feed it to implement Seek. Rather than going back to an nsIStorageStream, I think I see a way to avoid this data going through JS at all, which should save us at least one copy and a couple of conversions. With luck this will fix the above-mentioned regression as well as improve performance. Patch forthcoming.
Created attachment 199509 [details] [diff] [review] avoid unnecessary conversions and copies Seems to work. Sorry for the original bum review, mvl!
Comment on attachment 199509 [details] [diff] [review] avoid unnecessary conversions and copies >Index: base/src/calICSService.cpp >+ // this copies the string into the input stream that's handed back, >+ // because we don't really own icalstr; it's one of libical's ring >+ // buffers Can you explain what copies? you use a dependent string, which doesn't copy. That confused me. I assume the imputstream copies. >+ return NS_NewCStringInputStream(aStreamResult, >+ nsDependentCString(icalstr)); tabs.. >+ // The return values is calIError match with ical errors, >+ // so no need for a conversion table or anything. That sentence has some grammar errors in it. (I think i once wrote it, so it's very likely to not be correct :) ) >Index: providers/ics/calICSCalendar.js >- this.unmappedProperties.push(prop); >+ this.unmappedProperties.push(prop); More tabs...
Created attachment 199599 [details] [diff] [review] patch, v2 Patch, v2. Addresses mvl's comments, and handles failure more gracefully.
Attachment #199509 - Attachment is obsolete: true
Comment on attachment 199599 [details] [diff] [review] patch, v2 r=mvl
Attachment #199599 - Flags: first-review+
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.