486676 - Unnecessary alarm load on startup for all subscribed calendars

Status: RESOLVED FIXED

(Calendar :: Provider: WCAP, defect)

Description

[Daniel Boelzle [:dbo]]

Attachment: fix - v1

Re-adding capabilities.alarms.popup.supported to WCAP which serves as a default for suppressAlarms. Moreover another minor fix: MODIFICATION_FALIED events in case nothing has changed (server returns empty VCALENDARs).

Comment 1

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

Comment on attachment 370854 [details] [diff] [review]
fix - v1

I'd rather see capabilities.alarms.popup.supported go away fully. The alternative is to check if the current alarm capabilities support DISPLAY (popup) alarms, i.e:

let alarmValues = this.getProperty("capabilities.alarms.actionValues") || [];
if (alarmValues.indexof("DISPLAY") < 0) {
    // If popup alarms are not supported, automatically suppress alarms
    ret = true;
}

Aside from that, we should evaluate what exactly suppressAlarms should do. Some points to consider:

* If alarms are suppressed, should the server not notify alarms too?
  - If so, what if the server doesn't support this?

* We don't have icons for suppressed email and sms alarms.
  - Not needed right now since we don't have a client email alarm service
  - OTOH needed if we decide that server alarms should be suppressed too.

* Looking at the current users of supressAlarms (the most important is the alarm service) I think its feasible to rewrite the conditions to check for (suppressAlarms || !supportsDISPLAYalarms) in the alarm service and possibly fit the conditions in other places.

r- for now, sorry.

Comment 2

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

with "fit the conditions in other places" I mean to not automatically set suppressAlarms = true just because we are not dealing with display alarms.

Comment 3

[Daniel Boelzle [:dbo]]

This is a WCAP regression bug/fix, because WCAP broke when revisiting the alarms. Before reworking that stuff further, I'd prefer we simply fix this WCAP regression now (i.e. patch). Your points are valid, but I don't think it's sensible to mix in other code that still deals with capabilities.alarms.popup.supported and suppressAlarms.

Comment 4

[Daniel Boelzle [:dbo]]

(In reply to comment #1)
> * If alarms are suppressed, should the server not notify alarms too?
>   - If so, what if the server doesn't support this?
I think we should scope suppressAlarms (aka Properties "Show Alarms" to the local app.

> * We don't have icons for suppressed email and sms alarms.
>   - Not needed right now since we don't have a client email alarm service
>   - OTOH needed if we decide that server alarms should be suppressed too.
I don't think we should widen suppressAlarms' meaning to anything else than DISPLAY alarms.

Comment 5

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

Comment on attachment 370854 [details] [diff] [review]
fix - v1

(In reply to comment #3)
> This is a WCAP regression bug/fix, because WCAP broke when revisiting the
> alarms. Before reworking that stuff further, I'd prefer we simply fix this WCAP
> regression now (i.e. patch). Your points are valid, but I don't think it's
> sensible to mix in other code that still deals with
> capabilities.alarms.popup.supported and suppressAlarms.

Sounds plausible. Please file followup bugs though.

Comment 6

[Daniel Boelzle [:dbo]]

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

-> FIXED

follow-up bug 486700 filed

Comment 7

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".