Closed Bug 471813 Opened 11 years ago Closed 11 years ago

Remove item attribute from calIAlarm to avoid cyclic references

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file)

Attached patch Fix - v1 β€” β€” Splinter Review
This patch gets rid of the item attribute and adapts the interface a bit. It also fixes the alarm unit tests.
Attachment #355063 - Flags: review?(daniel.boelzle)
Blocks: 353492
Comment on attachment 355063 [details] [diff] [review]
Fix - v1

r=dbo
Attachment #355063 - Flags: review?(daniel.boelzle) → review+
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
(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.
(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.
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.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.