341776 - calICalendar::superCalendar attribute for calendar composition

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Daniel Boelzle [:dbo]]

Currently the ICS provider caches its items using a memory calendar to speed-up reading. The memory calendar has a (private) property called "calendarToReturn", so all items it posts to listeners point to the (logically) correct ICS provider.
Generalizing this concept, I propose to add an attribute calICalendar::superCalendar.

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: adding superCalendar attribute

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Chances are reasonable that once we move to the calICachingCalendar architecture, the ICS calendar will no longer have an inner memory calendar.  That aside, what is useful about exposing this via public interface, rather than leaving it as a private implementation detail?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Ah, I see.  This is essentially a calICachingCalendar piece.

Comment 4

[Daniel Boelzle [:dbo]]

Dan,
I have (prototypically) implemented a caching mechanism for the WCAP stuff to reduce network traffic, sync'ing in changes into a local storage calendar. Similar to the ICS provider, two calendars have to logically act as one. So I thought it would be time to generalize this.
So far, the sync'ing works, but I am experiencing major performance problems when my local storage cal gets fat (getItems() then takes 10 seconds or more); I am going to profile this soon. Because, it is only a prototype, this issue does not really block issue 340949.

Comment 5

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

Comment on attachment 225879 [details] [diff] [review]
adding superCalendar attribute

I only found this patch just now... I like the idea, but unfortunately, the patch doesn't apply anymore. Quite a few conflicts. We need this patch for caching etc, I think.

>+  /**
>+   * Multiple calendar instances may be composited, logically acting as a
>+   * single calendar, e.g. for caching puorposing.
>+   * This attribute determines the super calendar instance that all provided
>+   * items belong to, ensuring calls enter the super instance first.
>+   * @see calIItemBase::calendar
>+   */
>+  attribute calICalendar superCalendar;

The comment could use some more explanation about what this implies for implementations of the interface. You should mention here and/or at getItem(s) that the calendar returned must be the superCalendar (if set).
Also, comment about what the attribute will be when it is not set.

r-, because it is bitrotten too much.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
Before I create a new patch, we should first come to a conclusion whether this approach (and the name superCalendar) is ok.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for not getting back to you on this much sooner.  This seems like a reasonable strategy.  Is there really anything other than caching that we might use this for?  If not, I'd be inclined to suggest calling it cachedCalendar and having any providers that we don't intend to test caching with (e.g. everything but memory and storage, I suspect) throw NS_ERROR_NOT_IMPLEMENTED for the getters and setters.

Comment 8

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

I don't really like the suggestion of naming it 'cachedCalendar'. There might be other, not caching related, cases where you want to use this attribute. So I vote for the more general 'superCalendar'.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: adding superCalendar attribute - v2

Unbitrotted patch, but depends on the patch in bug 404085 being applied.

Comment 10

[Daniel Boelzle [:dbo]]

Comment on attachment 289182 [details] [diff] [review]
adding superCalendar attribute - v2

>+    // attribute calICalendar superCalendar;
>+    get superCalendar() {
>+        // If we have a superCalendar, check this calendar for a superCalendar.
>+        // This will make sure the topmost calendar is returned
>+        return this.mSuperCalendar && this.mSuperCalendar.superCalendar || this;
>+    },
Please rewrite the above like

if (this.mSuperCalendar) {
    return this.mSuperCalendar.superCalendar;
}
return this;

for me ;)

r=dbo

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED