352667 - After deleting calendar the last calendar in list is not displayed

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Andreas Treumann]

REPRODUCTION:
=============

- create a second calendar
- mark this calendar in at the calendar tabpage
- push the delete button or use the context menu


RESULT:
=======

- both calendar in the tabpage get lost

EXPECTED RESULT:
================

- only the deleted calendar should disappear

REPRODUCIBLE:
=============

- always

After restarting Thunderbird the not deleted calendar is back at the calendar tabpage

Comment 1

[Stefan Sitter [:ssitter]]

Confirmed. Regression range:

WORKS in Lightning/0.1+ (2006-09-05-06) and Thunderbird/1.5.0.5 (20060719)
FAILS in Lightning/0.1+ (2006-09-06-06) and Thunderbird/1.5.0.5 (20060719)

Checkins to mozilla/calendar: http://tinyurl.com/j6gwu

Additional Information:

Quick test showed that this is not a repaint problem. 
The problem is that the last calendar in list (after delete) is not displayed.

Calendars shown before: [Home] [2]                delete calendar [2]
Calendars shown after:  - none -

Calendars shown before: [Home] [2] [3] [4]        delete calendar [2]
Calendars shown after:  [Home] [3]

Calendars shown before: [Home] [2] [3] [4] [5]    delete calendar [3]
Calendars shown after:  [Home] [2] [4]

Calendars shown before: [Home] [2] [3] [4] [5]    delete calendar [5]
Calendars shown after:  [Home] [2] [3]

Comment 2

[Simon Paquet [:sipaq]]

I suspect that the fix for bug 296202 was the cause

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

I can't actually reproduce this problem in debug builds, though I do see it in 0.3RC1.  It's my understanding that layout does more complete consistency checking in debug builds, so I suspect that's the cause.  However, after spending lots of time reading code and buddying up to dump(), most of what's going on is pretty clear.

The ltnRemoveCalendar change that uncovered this bug makes us always delete a calendar after we unregister it in order to workaround the current unsubscribe/delete UI confusion.  This means that both the onCalendarUnregistering and onCalendarDeleting callbacks are being called, and they both attempt to remove the calendar from the tree and the composite calendar.

Doing this twice is clearly unnecessary, though in theory it shouldn't hurt us.  However, because removeCalendarFromTree doesn't check the result of getCalendars().indexOf(), this means that it attempts to call calTree.boxObject.rowCountChanged with an index of -1, which is clearly bogus.  We should file a spinoff bug to bulletproof removeCalendarFromTree.

Another thing that's relevant here is that calCompositeCalendar.calendars is not implemented, and to workaround that, getCalendars() and get rowCount() in this file get a bunch of information from the calendar manager that they _should_ be getting from the composite calendar.  That needs a spinoff bug too, and I postulate that we should make the composite provider and the calendar manager interfaces use the same mechanism to return calendar lists, whether that's an XPCOM array or an nsISimpleEnumerator.

This means onCalendarUnregistering calls removeCalendarFromTree, which calls rowCountChanged, which then causes layout to go re-fetch the rowcount from the rowCount() getter.  Except now we've got a problem: the rowCount getter is getting its number from the calendar manager, which hasn't actually done the unregistering yet, so the number doesn't match the hint we gave rowCountChanged.  Layout asserts, and then uses the bogus rowCount in some calculations, the consequences of which are not entirely clear to me.

So!  The best fix for this is to postpone updating the tree until the deleting callback, because by that time, the unregistration has already happened, which means that the rowCount getter returns a number consistent with what we told rowCountChanged, and layout therefore doesn't assert and use the wrong number in calcuations.  More importantly, we're only calling removeCalendarFromTree() once, and never with an index of -1.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: two-line patch

I postulate that this is a low-risk patch because of its simplicity, and because as far as I know, all calendar removal in Lightning goes through ltnRemoveCalendar(), so we're always guaranteed to execute the delete callback.
This risk could be cut further by someone verifying my assumption above, and by testing with an optimized build in addition to the debug build that I've tested with, but I'm tired enough that I need to sleep now. 

If, in fact, the bug is always or usually reproducible in debug builds, I think it's visible enough, and scary enough to the user that we should take this patch, even though there is a non-zero amount of risk.

I'll leave it to those who are awake before the nightlies get kicked off to decide what to do here with regard to landing it or not...

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> If, in fact, the bug is always or usually reproducible in debug builds, I think

I meant "release builds" there, of course. 

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> This risk could be cut further by someone verifying my assumption above, and by
> testing with an optimized build in addition to the debug build that I've tested
> with, but I'm tired enough that I need to sleep now. 

Which is to say that it doesn't seem to break anything in my debug build.  Since I don't see the problem there, I can't be 100% sure that it actually fixes it, but I'd be very surprised if that were the case.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #6)
> Which is to say that it doesn't seem to break anything in my debug build. 

My MOZILLA_1_8_BRANCH debug build, that is.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 241428 [details] [diff] [review]
two-line patch

I think the patch does work, but it doesn't really make me happy about the code. IT would be more logical remove in the unregister callback, and not in the delete callback. It just happens to work now, because we call them both.
It also seems pretty wrong to me that removeCalendarFromTree tries to lookup the list of calendars. It can't really do that, because the calendar is alsready deleted. But what if removeCalendarFromTree would do the more sane thing, and loop over the items that are actually in the tree? That's where it should remove the item from in the end, so it seems to make sense to search for the item there.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #8)
> (From update of attachment 241428 [details] [diff] [review] [edit])
> I think the patch does work, but it doesn't really make me happy about the
> code. IT would be more logical remove in the unregister callback, and not in
> the delete callback. It just happens to work now, because we call them both.

From a strict purity of essence standpoint, that's sort of true.  However, from a user experience standpoint, there's another interesting argument: if the delete fails, the user should (after being alerted) have another chance to delete the calendar again later (if it was a network problem, for example, maybe in a future session), which is only really achieved if the calendar is left in the UI and re-registered.  That is, in the case where the user wants to actually blow away a calendar, unsubscribe and delete really should be atomic.

From a more pragmatic "how is the codebase really going to evolve?" standpoint, this is all sort of academic, because the delete / unsubscribe code is gonna get a pretty big whack once we clean up the UI for this.

> It also seems pretty wrong to me that removeCalendarFromTree tries to lookup
> the list of calendars. It can't really do that, because the calendar is
> alsready deleted. But what if removeCalendarFromTree would do the more sane
> thing, and loop over the items that are actually in the tree? That's where it
> should remove the item from in the end, so it seems to make sense to search for
> the item there.

Yes, totally.  This is what I was trying to get at with my proposal to switch from using the calendar manager list to using the composite calendar list.  One could arguably go one step further privately track a list for use by the view alone separate from the composite calendar list, but it's not clear to me if that really buys us anything, or if it's just one more data structure that we would need to manually keep in sync.

Comment 10

[Joey Minta]

Comment on attachment 241428 [details] [diff] [review]
two-line patch

This looks to me like very ugly but functional code.  It's not clear to me how often users actually delete calendars, and hence how likely they are to come across this bug.  If it is landed, it should be backed out immediately after the release, in order to facilitate a proper fix here.  (I don't think a follow-up bug is sufficient impetus for something this bad.)
r2=jminta, and i'll leave it up to active drivers to make a final call on landing.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 241428 [details] [diff] [review]
two-line patch

as i said, this code should work. r=mvl
But do file followup bugs on cleaning this whole file and the other things you found.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Landed on the 1.8 branch only; reassigning to myself as a blocker for backout after we ship.

Comment 13

[Matthew (lilmatt) Willis]

(In reply to comment #12)
> Landed on the 1.8 branch only; reassigning to myself as a blocker for backout
> after we ship.

Backed out on MOZILLA_1_8_BRANCH only. (This never landed on trunk)

Checking in calendar/lightning/content/calendar-management.js;
/cvsroot/mozilla/calendar/lightning/content/calendar-management.js,v  <--  calendar-management.js
new revision: 1.23.2.27; previous revision: 1.23.2.26

Comment 14

[Bob]

I have experienced this with 0.3RC and Win2k, with an additional twist.  Once all the calendars disappear from the list, I cannot create a new one.  The "New Calendar" wizard completes successfully but the new calendar never shows up in the list.  The result is I cannot create even a single calendar.  This persists even after uninstall/reinstall.

Comment 15

[Thomas Benisch]

Attachment: patch based on internal calendar list

In this patch the calendar tree view uses its own internal calendar list.
The list is initialized by getCalendarManger().getCalendars() and
updated with addCalendarToTree() and removeCalendarFromTree().
In addition, removeCalendarFromTree() is more tolerant, that means
the function returns without error, if a calendar was already removed
from the tree. This allows to call the function more than once on
the same calendar.

The described approach has two advantages:
1.) It solves the unregistering problem due to the fact, that the
    unregister event is fired before the calendar is actually removed
    from the calendar manager.
2.) Performance Improvements.
    When working on WCAP subscriptions I encountered severe performance
    problems, which were caused by the fact, that the view calls
    very often getCalendarManager().getCalendars(), e.g. when selecting
    a tree list entry.

Some additional remarks for the reviewer:
- The composite calendar cannot be used instead of the own calendar list
  (as proposed in comment #3), because only those calendars of the tree
  which have a checked checkbox are part of the composite calendar.
- Using the nsISupportsInterfacePointer in getIndexForCalendar() didn't
  work correctly for new registered calendars. If I understand it
  correctly, then two objects can be compared with the help of
  the nsISupportsInterfacePointer, see e.g. compareObjects() in
  calendar/base/src/calUtils.js, but then the nsISupportsInterfacePointer
  must be used for both objects. In getIndexForCalendar() the
  nsISupportsInterfacePointer was only used for one object.
  In principal one can get this working by looping over the calendar
  list and using nsISupportsInterfacePointer for each entry in addition,
  but as the rest of the calendar code always compares calendar.uri,
  I decided to go this way.
- The ltnSetTreeView callback in ltnNewCalendar is unnecessary and
  was actually never called.

Comment 16

[Joey Minta]

(In reply to comment #15)
> 2.) Performance Improvements.
>     When working on WCAP subscriptions I encountered severe performance
>     problems, which were caused by the fact, that the view calls
>     very often getCalendarManager().getCalendars(), e.g. when selecting
>     a tree list entry.
I don't understand this statement.  Given that (a) I can't imagine our target user having more than a dozen calendars and (b) the calCalendarManager already caches its calendars, where is the performance impact?  Yes, there's a small cost to cross the xpcom boundary, but since this isn't a perf-critical area, I can't see how that small cost hurts us.  I've never seen the calendar list be anything but instantaneous.  This just seems to create another in-memory list of calendars, duplicating the existing one in the manager.  That's a cost I'd rather not impose on the user unless necessary.

Comment 17

[Matthew (lilmatt) Willis]

I want to wait until the question in comment #16 is answered and that jminta/dmose/mvl are all satisfied that this is the way we want to fix this bug before reviewing.

Comment 18

[Thomas Benisch]

(In reply to comment #16)
> (In reply to comment #15)
> > 2.) Performance Improvements.
> >     When working on WCAP subscriptions I encountered severe performance
> >     problems, which were caused by the fact, that the view calls
> >     very often getCalendarManager().getCalendars(), e.g. when selecting
> >     a tree list entry.
> I don't understand this statement.  Given that (a) I can't imagine our target
> user having more than a dozen calendars and (b) the calCalendarManager already
> caches its calendars, where is the performance impact?  Yes, there's a small
> cost to cross the xpcom boundary, but since this isn't a perf-critical area, I
> can't see how that small cost hurts us.  I've never seen the calendar list be
> anything but instantaneous.  This just seems to create another in-memory list
> of calendars, duplicating the existing one in the manager.  That's a cost I'd
> rather not impose on the user unless necessary.

I think there's some misunderstanding, which I'll try to clarify.

First of all, there are no performance problems in the calendar tree list.
I only stumbled about severe performance problems with the current
implementation, after I modified the calCalendarManager code for WCAP
subscriptions. Those problems can be solved also in another
way, therefore I think we should leave WCAP for further discussions
out of the game.

Nevertheless I think that using an internal calendar list and avoiding
the frequent calls to getCalendarManager().getCalendars() is the
right approach.

Joey, you're right, that the calCalendarManager caches its calendars.
But getCalendars doesn't simply return the list of cached calendars,
but always accesses the database, reads out all stored calendars,
checks if a stored calendar is not in the cache, and if so, creates
a new calendar. Only after that, the list of cached calendars is
returned. 

In addition I don't understand what's so problematic with an additional
tree internal list. E.g. in Sunbird a listbox (list-calendars-listbox)
is used for the calendars tab. Each listitem of this listbox stores
the corresponding calendar. This means the listbox has its own
list of calendars.

Comment 19

[Thomas Benisch]

Attachment: patch based on internal calendar list v2

new version of patch with a small bug fix

Comment 20

[Matthew (lilmatt) Willis]

jminta/mvl/dmose:
Does comment #18 satisfy you guys, or do we need more discussion?

Comment 21

[Matthew (lilmatt) Willis]

Comment on attachment 250440 [details] [diff] [review]
patch based on internal calendar list v2

Moving review to jminta.

I don't feel as if I have enough of a handle on the original concerns to adequately determine if this addresses them.

Comment 22

[Joey Minta]

Comment on attachment 250440 [details] [diff] [review]
patch based on internal calendar list v2

This is another case of a patch that tries to do more than fix the bug, which really makes it hard to review.

+    gCalendars.push(aCalendar);
     var boxobj = document.getElementById("calendarTree").treeBoxObject;
-    boxobj.rowCountChanged(getIndexForCalendar(aCalendar), 1);
+    boxobj.rowCountChanged(gCalendars.length-1, 1);
 
This relies on assumptions about the internal working of calCalendarManager (that it always adds calendars to the end of its list).  I know we want to support re-ordering the calendar list, so this assumption isn't a good one to build into the code.

+    if (index == -1) {
+        return;
+    }
+
This looks like something we should ASSERT for.  I'm not convinced of the value of making this 'more error tolerant'.  Rather, it feels like we're hiding things that should legitimately be bugs.  Why would you call this function more than once?

-    if (index == calTree.view.rowCount-1) {
-        index--;
+    if (index >= calTree.view.rowCount) {
+        index = calTree.view.rowCount - 1;
     }
This is confusing due to the other changes.  Correct me if I'm wrong.  Because you've changed the structure of .rowCount to rely on gCalendars, we're currently in a situation where gCalendar.length != calICalendarManager.getCalendars({}).length, which requires these changes?

+    for (var i = 0; i < gCalendars.length; ++i) {
+        if (gCalendars[i].uri.equals(aCalendar.uri)) {
+            return i;
+        }
+    }
+    return -1;
This is another assumption you can't make.  uri was never intended to be unique, and callers relying on it being so are bugs that should be fixed.

I'd like to reiterate that the fastest way to move forward with this is to separate out the front-end calendar-list caching from the tree-redraw bug. I'm still not convinced the caching is a change we want, but I am convinced we want the tree-bug fixed.

Comment 23

[Thomas Benisch]

(In reply to comment #22)
> (From update of attachment 250440 [details] [diff] [review])
> This is another case of a patch that tries to do more than fix the bug, which
> really makes it hard to review.

I don't agree here. What exactly does the patch more than trying to fix
the bug?

> +    gCalendars.push(aCalendar);
>      var boxobj = document.getElementById("calendarTree").treeBoxObject;
> -    boxobj.rowCountChanged(getIndexForCalendar(aCalendar), 1);
> +    boxobj.rowCountChanged(gCalendars.length-1, 1);
> 
> This relies on assumptions about the internal working of calCalendarManager
> (that it always adds calendars to the end of its list).  I know we want to
> support re-ordering the calendar list, so this assumption isn't a good one to
> build into the code.

In this patch the sorting order of the calendar tree is defined by the order of
the gCalendars array. At the moment this order is identical to the
calCalendarManager implementation. If one wants to sort the calendar list, then
that's a UI feature and should be handled in the UI code, e.g. the calendar tree
list. If one wants to sort the calCalendarManager list, then that's the wrong
approach. You will also get problems in Sunbird's calendar listbox.

> +    if (index == -1) {
> +        return;
> +    }
> +
> This looks like something we should ASSERT for.  I'm not convinced of the value
> of making this 'more error tolerant'.  Rather, it feels like we're hiding
> things that should legitimately be bugs.  Why would you call this function more
> than once?

You're right, those lines can either be removed or asserted.

> -    if (index == calTree.view.rowCount-1) {
> -        index--;
> +    if (index >= calTree.view.rowCount) {
> +        index = calTree.view.rowCount - 1;
>      }
> This is confusing due to the other changes.  Correct me if I'm wrong.  Because
> you've changed the structure of .rowCount to rely on gCalendars, we're
> currently in a situation where gCalendar.length !=
> calICalendarManager.getCalendars({}).length, which requires these changes?

Also here I'm just more error tolerant. In principal the old code also works
and is probably the better choice.
With or without this patch, it is never safe to rely on
calICalendarManager.getCalendars({}).length, because this is not necessarily
the number of calendars displayed in the tree. The reason is the 
onCalendarUnregistering notification, which happens before the calendar is 
removed from the calendar manager.
This was also discussed in previous comments.

> +    for (var i = 0; i < gCalendars.length; ++i) {
> +        if (gCalendars[i].uri.equals(aCalendar.uri)) {
> +            return i;
> +        }
> +    }
> +    return -1;
> This is another assumption you can't make.  uri was never intended to be
> unique, and callers relying on it being so are bugs that should be fixed.

AFAIK we have no calendar identities yet. calendar.uri is used in several places
in the calendar code. So what's wrong if I do the same?

> I'd like to reiterate that the fastest way to move forward with this is to
> separate out the front-end calendar-list caching from the tree-redraw bug. I'm
> still not convinced the caching is a change we want, but I am convinced we want
> the tree-bug fixed.

My approach for fixing the tree bug is exactly to use an internal calendar list.
I have no idea how to solve the onCalendarUnregistering notification problem
otherwise, apart from sending the unregister event after a calendar has been
unregistered. Therefore it makes no sense to separate two things which belong
together.

If you have other ideas of how to fix (not hack) this bug, please let me know.

Comment 24

[Thomas Benisch]

Comment on attachment 250440 [details] [diff] [review]
patch based on internal calendar list v2

I'm asking for 2nd review because I like to have another opinion
on the approach of using an internal calendar list.

As already mentioned, Sunbird uses a listbox and each list item
holds a reference to the corresponding calendar it belongs to.
This is de facto nothing else than an internal calendar list.

Please note, that this issue is targeted for the 0.5 release
and code freeze is scheduled for March 24th, 2007.

Comment 25

[Matthew (lilmatt) Willis]

Moving from dmose to tbe

Comment 26

[Matthew (lilmatt) Willis]

This is really ugly and affects usability in a pretty bad way.  Marking it as blocking.

Comment 27

[Matthew (lilmatt) Willis]

Comment on attachment 250440 [details] [diff] [review]
patch based on internal calendar list v2

Clearing jminta's review request.  mvl is enough, and this is architecture-related such that I'd rather mvl take it (over me).

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 250440 [details] [diff] [review]
patch based on internal calendar list v2

> function getCalendars()
> {
>+    return gCalendars;
> }

If this function does nothing anymore, you could just change the callers to use gCalendars directly. (if there are any callers left. If not, remove the function)

> function getIndexForCalendar(aCalendar) {
>-    // Special trick to compare interface pointers, since normal, ==
>-    // comparison can fail due to javascript wrapping.
>-    var sip = Components.classes["@mozilla.org/supports-interface-pointer;1"]
>-                        .createInstance(Components.interfaces.nsISupportsInterfacePointer);
>-    sip.data = aCalendar;
>-    sip.dataIID = Components.interfaces.calICalendar;
>-    return getCalendars().indexOf(sip.data);
>+    for (var i = 0; i < gCalendars.length; ++i) {
>+        if (gCalendars[i].uri.equals(aCalendar.uri)) {
>+            return i;
>+        }
>+    }
>+    return -1;

I agree with joey here. There is work going on to have add calICalendar.id. When that's done, you want to use that, because at that point, the uniqueness of the url is no longer guaranteed. I don't think we should change this code now, when we know it need to be changed in the near future anyway, and the current change does not fix a bug or anything.

r2=mvl with that

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #23)
> If one wants to sort the calendar list, then
> that's a UI feature and should be handled in the UI code, e.g. the calendar
> tree
> list. If one wants to sort the calCalendarManager list, then that's the wrong
> approach. You will also get problems in Sunbird's calendar listbox.

Agreed. Sorting is a UI thing, not a backend thing.

Comment 30

[Thomas Benisch]

(In reply to comment #28)
> (From update of attachment 250440 [details] [diff] [review])
> > function getCalendars()
> > {
> >+    return gCalendars;
> > }
> 
> If this function does nothing anymore, you could just change the callers to use
> gCalendars directly. (if there are any callers left. If not, remove the
> function)

getCalendars() is called in the function ltnSelectedCalendar() in
messenger-overlay-sidebar.js, that means the caller is in a different file.
That's the reason why I kept the getCalendars() function.
But there's no problem to use gCalendars instead.

> > function getIndexForCalendar(aCalendar) {
> >-    // Special trick to compare interface pointers, since normal, ==
> >-    // comparison can fail due to javascript wrapping.
> >-    var sip = Components.classes["@mozilla.org/supports-interface-pointer;1"]
> >-                        .createInstance(Components.interfaces.nsISupportsInterfacePointer);
> >-    sip.data = aCalendar;
> >-    sip.dataIID = Components.interfaces.calICalendar;
> >-    return getCalendars().indexOf(sip.data);
> >+    for (var i = 0; i < gCalendars.length; ++i) {
> >+        if (gCalendars[i].uri.equals(aCalendar.uri)) {
> >+            return i;
> >+        }
> >+    }
> >+    return -1;
> 
> I agree with joey here. There is work going on to have add calICalendar.id.
> When that's done, you want to use that, because at that point, the uniqueness
> of the url is no longer guaranteed. I don't think we should change this code
> now, when we know it need to be changed in the near future anyway, and the
> current change does not fix a bug or anything.

There is some need to change the current implementation of getIndexForCalendar().
The reason is that the current implementation doesn't work.
Please see comment #15:

- Using the nsISupportsInterfacePointer in getIndexForCalendar() didn't
  work correctly for new registered calendars. If I understand it
  correctly, then two objects can be compared with the help of
  the nsISupportsInterfacePointer, see e.g. compareObjects() in
  calendar/base/src/calUtils.js, but then the nsISupportsInterfacePointer
  must be used for both objects. In getIndexForCalendar() the
  nsISupportsInterfacePointer was only used for one object.
  In principal one can get this working by looping over the calendar
  list and using nsISupportsInterfacePointer for each entry in addition,
  but as the rest of the calendar code always compares calendar.uri,
  I decided to go this way.

Therefore I think there are only 2 alternatives:
a) implement getIndexForCalendar() based on calendar.uri and change
   the implementation as soon as we have calendar ids
b) loop over calendar list and use nsISupportsInterfacePointer as
   described above

Comment 31

[Michiel van Leeuwen (email: mvl+moz@)]

> Therefore I think there are only 2 alternatives:
> a) implement getIndexForCalendar() based on calendar.uri and change
>    the implementation as soon as we have calendar ids

calendar.id has been added now, so you can use that now.

Comment 32

[Thomas Benisch]

(In reply to comment #31)
> calendar.id has been added now, so you can use that now.

O.K., I will use calendar.id.

What about gCalendars versus getCalendars()?

Do I need another review?

Comment 33

[Thomas Benisch]

Attachment: patch based on internal calendar list v3

This patch uses calendar.id in getIndexForCalendar().
Still open is the gCalendars versus getCalendars() question.

Comment 34

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 258422 [details] [diff] [review]
patch based on internal calendar list v3

You might add a comment to getCalendar() why it is still needed. It's ok to use the internal gCalendar variable in the internal functions.

Patch looks good. r=mvl

Comment 35

[Thomas Benisch]

(In reply to comment #34)
> (From update of attachment 258422 [details] [diff] [review])
> You might add a comment to getCalendar() why it is still needed. It's ok to use

done

Comment 36

[Thomas Benisch]

patch checked in on HEAD and MOZILLA_1_8_BRANCH

Comment 38

[Stefan Sitter [:ssitter]]

Verified with Lightning/0.5pre (2007032003) and Thunderbird/1.5.0.10 (20070221)