471813 - Remove item attribute from calIAlarm to avoid cyclic references

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Attachment: Fix - v1

This patch gets rid of the item attribute and adapts the interface a bit. It also fixes the alarm unit tests.

Comment 1

[Daniel Boelzle [:dbo]]

Comment on attachment 355063 [details] [diff] [review]
Fix - v1

r=dbo

Comment 2

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

I think we should start revving iid's when changing interfaces.
Looking at the patch, you changed the meaning of alarmDate. To make it clear, you could change the name also, so extension authors can't miss the changed meaning. I'm surprised that no calling sites have to be changed. On a quick look, the alarmService might need changes. For example, [1] seems to call .startDate and .alarmRelated on the same aItem (which is an calIEvent or calITodo, as far as I can see) This can't work, can it?

[1] http://mxr.mozilla.org/comm-central/source/calendar/base/src/calAlarmService.js#376

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> I think we should start revving iid's when changing interfaces.
> Looking at the patch, you changed the meaning of alarmDate. To make it clear,
> you could change the name also, so extension authors can't miss the changed
> meaning. I'm surprised that no calling sites have to be changed. On a quick
I don't think we should do that for premature API unless it's some kind of final and complete. IMO the earliest date doing so would be a 1.0 release.
Nailing interfaces too early is a major burden to support them later on, cripples names etc. Moreover I don't see a real benefit for extension authors except for that we preserve older (working) implementations of those interfaces, which we for sure don't have resources for.

> look, the alarmService might need changes. For example, [1] seems to call
> .startDate and .alarmRelated on the same aItem (which is an calIEvent or
> calITodo, as far as I can see) This can't work, can it?
I don't see a problem here, there's no use of calIAlarm. AFAIK, Philipp's new alarm API is still under development, and only unit tests address those objects yet.

Comment 4

[Simon Paquet [:sipaq]]

(In reply to comment #3)
>> I think we should start revving iid's when changing interfaces.
>> Looking at the patch, you changed the meaning of alarmDate. To make it 
>> clear, you could change the name also, so extension authors can't miss the 
>> changed meaning. I'm surprised that no calling sites have to be changed. 
>
> I don't think we should do that for premature API unless it's some kind of
> final and complete. IMO the earliest date doing so would be a 1.0 release.
> Nailing interfaces too early is a major burden to support them later on,
> cripples names etc. 

I don't think revving the IID of an interface in any way implies that the interface is considered frozen. Just the contrary is the case IMO, as you 
would only change an unfrozen interface and indicate your change by revving 
the IID.

If you look at the larger mozilla codebase, that is what people do. Changes to 
unfrozen interfaces always require revving the IID AFAIK.

Comment 5

[Daniel Boelzle [:dbo]]

My comment was more specifically referring to "To make it clear, you could change the name...". We should be liberal about changes and don't fall into producing different names unless we think something is mature. I am OK with bumping interface ids, but in this case the interface isn't used anywhere except for the unit tests; I don't see a need yet.

Comment 6

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

Sorry about the confusion. I like the idea of changing the interface id and even attribute names, but as noted this interface is not used yet so I doubt its really needed. I have older patches in bug 353492 that do more changes (i.e to the alarm service), but given that patch was way too large to review in a timely fashion, I'm slowly splitting up things. This is part of it.



Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/2a3a53dd2321>

-> FIXED