Unnecessary alarm load on startup for all subscribed calendars

RESOLVED FIXED in 1.0b1

Status

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

({regression})

Details

Attachments

(1 attachment)

Assignee

Description

10 years ago
Posted patch fix - v1Splinter Review
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).
Attachment #370854 - Flags: review?(philipp)
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.
Attachment #370854 - Flags: review?(philipp) → review-
with "fit the conditions in other places" I mean to not automatically set suppressAlarms = true just because we are not dealing with display alarms.
Assignee

Comment 3

10 years ago
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.
Assignee

Updated

10 years ago
Keywords: regression
Assignee

Comment 4

10 years ago
(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 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.
Attachment #370854 - Flags: review- → review+
Assignee

Comment 6

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

-> FIXED

follow-up bug 486700 filed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.