Comment 20 for bug 320618

Comment on attachment 481757
fix v2

> calICSCalendar.prototype = {
> __proto__: cal.ProviderBase.prototype,
>
> mObserver: null,
> locked: false,
>+ forceRefresh: false,
My hopes were we could get rid of this member, see below.

>+ reLoad: function calICSCalendar_reLoad() {
>+ this.queue.push({action: 'refresh', forceRefresh: true});
>+ this.processQueue();
>+ },
I'd prefer to call this method forceRefresh(), we'll need to get rid of the member for that.

>@@ -172,38 +190,44 @@ calICSCalendar.prototype = {
> // Lock other changes to the item list.
> this.lock();
>
> try {
> streamLoader.init(this);
> channel.asyncOpen(streamLoader, this);
Currently we pass |this| as a second parameter but don't use it in the stream listener. Instead, you could probably use nsISupportsPRBool to pass a boolean value for the refresh flag. This way you can eliminate the need for forceRefresh as a member of the prototype. See http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/tests/SimpleTest/quit.js#75 for how to create one.

> onStreamComplete: function(loader, ctxt, status, resultLength, result)
> {
>- // No need to do anything if there was no result
>+ // Clear any existing events if there was no result
...then later in onStreamComplete, you can do |let forceRefresh = ctxt.QueryInterface(Components.interfaces.nsISupportPRBool).data;|. This way you don't need forceRefresh to be a member.

>
>- doRefresh: function calICSCalendar_doRefresh() {
>+ createMemoryCalendar: function calICSCalendar_createMemoryCalendar() {
>+ // Create a new calendar, to get rid of all the old events
>+ // Don't forget to remove the observer
>+ this.mMemoryCalendar.removeObserver(this.mObserver);
What happens if the memory calendar hasn't been initialized? I'd suggest adding if (this.mMemoryCalendar) here.
Also, does it make sense to add the new observer in this function already? I had the feeling you're adding the observer outside of the call each time createMemoryCalendar is called.

> // File not found: a new calendar. No problem.
>+ this.createMemoryCalendar();
>+ this.mMemoryCalendar.addObserver(this.mObserver);
>+ this.mObserver.onLoad(this);
> this.unlock();
Lets hope this doesn't cause dataloss in certain situations. While it totally makes sense when the file is really empty, do you think there are situations where there is no content because of an error, that would trigger this code? I'm not sure.

r- to resolve above comments, in general I'm happy about the patch though :-)