Closed Bug 353051 Opened 13 years ago Closed 13 years ago

'Dismiss All' doesn't dismiss all alarms (skips every second)

Categories

(Calendar :: Alarms, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: ssitter)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

'Dismiss All' doesn't dismiss all alarms (skips every second)

Steps to Reproduce:
1. Create four events with alarms set (e.g. event '1', '2', '3' and '4')
2. Wait until all four alarms are fired and press 'Dismiss All'
3. Restart Sunbird

Actual Results:
Alarm popup for event '2' and '4' are displayed.

Expected Results:
No alarm popup because all alarms were dismissed.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060917 Calendar/0.3a2+

Console output: During alarm creation:

    considering alarm for item:1
     offset:PT15M, which makes alarm time:2006/09/11 11:30:00 UTC
    now is 2006/09/17 16:33:40 UTC
    Last ack was:null
    alarm is in the past, and unack'd, firing now!
    snooze time is:null
    considering alarm for item:2
     offset:PT15M, which makes alarm time:2006/09/12 11:30:00 UTC
    now is 2006/09/17 16:34:00 UTC
    Last ack was:null
    alarm is in the past, and unack'd, firing now!
    snooze time is:null
    considering alarm for item:3
     offset:PT15M, which makes alarm time:2006/09/13 11:45:00 UTC
    now is 2006/09/17 16:34:16 UTC
    Last ack was:null
    alarm is in the past, and unack'd, firing now!
    snooze time is:null
    considering alarm for item:4
     offset:PT15M, which makes alarm time:2006/09/14 11:45:00 UTC
    now is 2006/09/17 16:34:20 UTC
    Last ack was:null
    alarm is in the past, and unack'd, firing now!

After pressing Dismiss All:

    snooze time is:null
    considering alarm for item:1
     offset:PT15M, which makes alarm time:2006/09/11 11:30:00 UTC
    now is 2006/09/17 16:35:08 UTC
    Last ack was:2006/09/17 16:35:08 UTC
    1 - alarm previously ackd2
    snooze time is:null
    considering alarm for item:3
     offset:PT15M, which makes alarm time:2006/09/13 11:45:00 UTC
    now is 2006/09/17 16:35:08 UTC
    Last ack was:2006/09/17 16:35:08 UTC
    3 - alarm previously ackd2
Regression range:

WORKS in Sunbird/0.3a2+ (2006-08-31-06)
FAILS in Sunbird/0.3a2+ (2006-08-31-21)
Keywords: regression
This was regressed by the checkin for Bug 298358 to
mozilla/calendar/base/content/calendar-alarm-dialog.js

After that patch onDismissAll() calls onDismissWidget() instead of dismissing each alarm itself. But onDismissWidget() will also remove the widget from dialog. Therefore the for-each-loop in onDismissAll() works only on every second item.
Attached patch loop in reverse order (obsolete) — Splinter Review
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Attachment #238906 - Flags: second-review?(jminta)
Attachment #238906 - Flags: first-review?(mattwillis)
Comment on attachment 238906 [details] [diff] [review]
loop in reverse order

>-  for each (kid in box.childNodes) {
>+  for (var i = box.childNodes.length-1; i >= 0; i--) {
>+    var kid = box.childNodes[i];
I _think_ you need to declare var kid outside the for loop, or you'll run into javascript strict warnings.

Otherwise, nice fix. r=lilmatt with var above for.
Attachment #238906 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 238906 [details] [diff] [review]
loop in reverse order

-  for each (kid in box.childNodes) {
+  for (var i = box.childNodes.length-1; i >= 0; i--) {
+    var kid = box.childNodes[i];
     if (!kid || !kid.item) {
         continue;
     }
If you're explicitly looping through childNodes here, the kid/kid.item checks ought to be superfluous.  They were a result of using forEach.  Yes?
Include changes to address Comment #4 and Comment #5. 
The |kid| check is not necessary with this loop. This also means there is no need for |kid| at all. Also removes obsolete code in that function.
Attachment #238906 - Attachment is obsolete: true
Attachment #239049 - Flags: second-review?(jminta)
Attachment #239049 - Flags: first-review?(mattwillis)
Attachment #238906 - Flags: second-review?(jminta)
Comment on attachment 239049 [details] [diff] [review]
loop in reverse order, v2

Yay for code removal. r2=jminta
Attachment #239049 - Flags: second-review?(jminta) → second-review+
Comment on attachment 239049 [details] [diff] [review]
loop in reverse order, v2

r1=lilmatt
Attachment #239049 - Flags: first-review?(mattwillis) → first-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Sunbird/0.3
Status: RESOLVED → VERIFIED
Whiteboard: [litmus testcase wanted]
Litmus testcase 2695 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.