Bug 1110183 - Displayed reminders can be improperly snoozed or duplicated when the calendar is refreshed. r=philipp
authorMatthew Mecca <matthew.mecca@gmail.com>
Wed, 23 Sep 2015 23:39:10 +0200
changeset 18427 243713ca559c073c6bc33e8c47a7d88a80807a7c
parent 18426 6b0dd3834439cdd74aef606eb9b119bebd1c9dc9
child 18428 579d2e9c7c2105b5c0c0bcb1367f13ce365ccfa9
push id11284
push userarchaeopteryx@coole-files.de
push dateThu, 24 Sep 2015 12:12:35 +0000
treeherdercomm-central@a54375d27b3f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersphilipp
bugs1110183
Bug 1110183 - Displayed reminders can be improperly snoozed or duplicated when the calendar is refreshed. r=philipp
calendar/base/content/dialogs/calendar-alarm-dialog.js
calendar/base/public/calIAlarmService.idl
calendar/base/src/calAlarmMonitor.js
calendar/base/src/calAlarmService.js
calendar/test/unit/test_alarmservice.js
--- a/calendar/base/content/dialogs/calendar-alarm-dialog.js
+++ b/calendar/base/content/dialogs/calendar-alarm-dialog.js
@@ -274,26 +274,21 @@ function removeWidgetFor(aItem, aAlarm) 
     setupTitle();
 }
 
 /**
  * Close the alarm dialog if there are no further alarm widgets
  */
 function closeIfEmpty() {
     let alarmRichlist = document.getElementById("alarm-richlist");
-    if (!alarmRichlist.hasChildNodes()) {
-        // check again in a short while since this removeWidgetFor call may be
-        // followed by an addWidgetFor call (e.g. when refreshing), and
-        // we don't want to close and open the window in that case.
-        function closer() {
-            if (!alarmRichlist.hasChildNodes()) {
-                window.close();
-            }
-        }
-        setTimeout(closer, 500);
+
+    // we don't want to close if the alarm service is still loading, as the
+    // removed alarms may be immediately added again.
+    if (!alarmRichlist.hasChildNodes() && !getAlarmService().isLoading) {
+        window.close();
     }
 }
 
 /**
  * Handler function called when an alarm entry in the richlistbox is selected
  *
  * @param event         The DOM event from the click action
  */
--- a/calendar/base/public/calIAlarmService.idl
+++ b/calendar/base/public/calIAlarmService.idl
@@ -36,26 +36,31 @@ interface calIAlarmServiceObserver : nsI
   void onRemoveAlarmsByCalendar(in calICalendar calendar);
 
   /**
    * Called when all alarms of a specific calendar are loaded.
    */
   void onAlarmsLoaded(in calICalendar calendar);
 };
 
-[scriptable,uuid(03669cf3-bf4f-4692-97a1-cca891964a1d)]
+[scriptable,uuid(42cfa9ce-49d6-11e5-b88c-5b90eedc1c47)]
 interface calIAlarmService : nsISupports
 {
   /**
    * This is the timezone that all-day events will be converted to in order to
    * determine when their alarms should fire.
    */
   attribute calITimezone timezone;
 
   /**
+   * Will return true while the alarm service is in the process of loading alarms
+   */
+  attribute boolean isLoading;
+
+  /**
    * Cause the alarm service to start up, create a list of upcoming
    * alarms in all registered calendars, add observers to watch for
    * calendar registration and unregistration, and setup a timer to
    * maintain that list and fire alarms.
    *
    * @note Will throw NS_ERROR_NOT_INITIALIZED if you have not previously set
    *       the timezone attribute.
    */
--- a/calendar/base/src/calAlarmMonitor.js
+++ b/calendar/base/src/calAlarmMonitor.js
@@ -165,10 +165,16 @@ calAlarmMonitor.prototype = {
 
             if (!ret && calAlarmWindow) { // window is open
                 calAlarmWindow.removeWidgetFor(thisItem, alarm);
             }
             return ret;
         });
     },
 
-    onAlarmsLoaded: function cAM_onAlarmsLoaded(aCalendar) {}
+    onAlarmsLoaded: function cAM_onAlarmsLoaded(aCalendar) {
+        // the alarm dialog won't close while alarms are loading, check again now
+        let calAlarmWindow = peekAlarmWindow();
+        if (calAlarmWindow) {
+            calAlarmWindow.closeIfEmpty();
+        }
+    }
 };
--- a/calendar/base/src/calAlarmService.js
+++ b/calendar/base/src/calAlarmService.js
@@ -479,71 +479,92 @@ calAlarmService.prototype = {
         }
     },
 
     findAlarms: function cAS_findAlarms(aCalendars, aStart, aUntil) {
         let getListener = {
             QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calIOperationListener]),
             alarmService: this,
             addRemovePromise: PromiseUtils.defer(),
+            batchCount: 0,
+            results: false,
             onOperationComplete: function cAS_fA_onOperationComplete(aCalendar,
                                                                      aStatus,
                                                                      aOperationType,
                                                                      aId,
                                                                      aDetail) {
                 this.addRemovePromise.promise.then((aValue) => {
                     // calendar has been loaded, so until now, onLoad events can be ignored:
                     this.alarmService.mLoadedCalendars[aCalendar.id] = true;
 
                     // notify observers that the alarms for the calendar have been loaded
                     this.alarmService.mObservers.notify("onAlarmsLoaded", [aCalendar]);
-                }, function onReject(aReason) {
+                }, (aReason) => {
                     Components.utils.reportError("Promise was rejected: " + aReason);
+                    this.alarmService.mLoadedCalendars[aCalendar.id] = true;
+                    this.alarmService.mObservers.notify("onAlarmsLoaded", [aCalendar]);
                 });
+
+                // if no results were returned we still need to resolve the promise
+                if (!this.results) {
+                    this.addRemovePromise.resolve();
+                }
             },
             onGetResult: function cAS_fA_onGetResult(aCalendar,
                                                      aStatus,
                                                      aItemType,
                                                      aDetail,
                                                      aCount,
                                                      aItems) {
                 let promise = this.addRemovePromise;
+                this.batchCount++;
+                this.results = true;
+
                 cal.forEach(aItems, (item) => {
                     try {
                         this.alarmService.removeAlarmsForItem(item);
                         this.alarmService.addAlarmsForItem(item);
                     } catch (ex) {
                         promise.reject(ex);
                     }
-                }, function completed() {
-                    promise.resolve();
+                }, () => {
+                    if (--this.batchCount <= 0) {
+                        promise.resolve();
+                    }
                 });
             }
         };
 
         const calICalendar = Components.interfaces.calICalendar;
         let filter = calICalendar.ITEM_FILTER_COMPLETED_ALL |
                      calICalendar.ITEM_FILTER_CLASS_OCCURRENCES |
                      calICalendar.ITEM_FILTER_TYPE_ALL;
 
         for each (let calendar in aCalendars) {
             // assuming that suppressAlarms does not change anymore until refresh:
             if (!calendar.getProperty("suppressAlarms") &&
                 !calendar.getProperty("disabled")) {
+                this.mLoadedCalendars[calendar.id] = false;
                 calendar.getItems(filter, 0, aStart, aUntil, getListener);
+            } else {
+                this.mLoadedCalendars[calendar.id] = true;
+                this.mObservers.notify("onAlarmsLoaded", [calendar]);
             }
         }
     },
 
     initAlarms: function cAS_initAlarms(aCalendars) {
-        // Purge out all alarm timers belonging to the refreshed/loaded calendar:
+        // Purge out all alarm timers belonging to the refreshed/loaded calendars
         this.disposeCalendarTimers(aCalendars);
 
-        // Purge out all alarms from dialog belonging to the refreshed/loaded calendar:
-        this.mObservers.notify("onRemoveAlarmsByCalendar", aCalendars);
+        // Purge out all alarms from dialog belonging to the refreshed/loaded calendars
+        for (let calendar of aCalendars) {
+            this.mLoadedCalendars[calendar.id] = false;
+            this.mObservers.notify("onRemoveAlarmsByCalendar", [calendar]);
+        }
 
         // Total refresh similar to startup.  We're going to look for
         // alarms +/- 1 month from now.  If someone sets an alarm more than
         // a month ahead of an event, or doesn't start Lightning
         // for a month, they'll miss some, but that's a slim chance
         let start = nowUTC();
         let until = start.clone();
         start.month -= 1;
@@ -552,10 +573,17 @@ calAlarmService.prototype = {
     },
 
     alarmFired: function cAS_alarmFired(aItem, aAlarm) {
         if (!aItem.calendar.getProperty("suppressAlarms") &&
             !aItem.calendar.getProperty("disabled") &&
             aItem.getProperty("STATUS") != "CANCELLED") {
             this.mObservers.notify("onAlarm", [aItem, aAlarm]);
         }
+    },
+
+    get isLoading() {
+        for (let calId in this.mLoadedCalendars) {
+            if (!this.mLoadedCalendars[calId]) return true;
+        }
+        return false;
     }
 };
--- a/calendar/test/unit/test_alarmservice.js
+++ b/calendar/test/unit/test_alarmservice.js
@@ -23,24 +23,29 @@ let alarmObserver = {
     },
 
     onRemoveAlarmsByItem: function obs_onRemoveAlarmsByItem(aItem) {
         if (aItem.hashId in this.firedMap) {
             delete this.firedMap[aItem.hashId];
         }
     },
 
+    onRemoveAlarmsByCalendar: function() {},
+
     onAlarmsLoaded: function obs_onAlarmsLoaded(aCalendar) {
+        this.checkLoadStatus();
         if (aCalendar.id in this.pendingOps) {
             this.pendingOps[aCalendar.id].call();
         }
     },
 
     doOnAlarmsLoaded: function obs_doOnAlarmsLoaded(aCalendar, aOperation) {
-        if (aCalendar.id in this.service.mLoadedCalendars) {
+        this.checkLoadStatus();
+        if (aCalendar.id in this.service.mLoadedCalendars &&
+            this.service.mLoadedCalendars[aCalendar.id]) {
             // the calendar's alarms have already been loaded, do the callback now
             aOperation.call();
         } else {
             // the calendar hasn't been fully loaded yet, set as a pending operation
             this.pendingOps[aCalendar.id] = aOperation;
         }
     },
 
@@ -80,36 +85,52 @@ let alarmObserver = {
                     // only alarms expected as timers should exist in the service's timer map
                     do_check_xor(this.expectedMap[calId][id][icalString] != EXPECT_TIMER,
                                  !!this.getTimer(calId, id, icalString));
                 }
             }
         }
     },
 
+    checkLoadStatus: function obs_checkLoadStatus() {
+        for (let calId in this.service.mLoadedCalendars) {
+            if (!this.service.mLoadedCalendars[calId]) {
+                // at least one calendar hasn't finished loading alarms
+                ok(this.service.isLoading);
+                return;
+            }
+        }
+        ok(!this.service.isLoading);
+    },
+
     clear: function obs_clear() {
         this.firedMap = {};
         this.pendingOps = {};
         this.expectedMap = {};
     }
 };
 
 function run_test() {
     do_get_profile();
 
+    add_test(() => {
+        // initialization needs to be done within the first test in order for
+        // the subsequent tests to run properly
+        initializeAlarmService();
+        cal.getCalendarManager().startup({onResult: function() {
+            cal.getTimezoneService().startup({onResult: function() {
+                run_next_test();
+            }});
+        }});
+    });
     add_test(test_addItems);
     add_test(test_loadCalendar);
     add_test(test_modifyItems);
 
-    initializeAlarmService();
-    cal.getCalendarManager().startup({onResult: function() {
-        cal.getTimezoneService().startup({onResult: function() {
-            run_next_test();
-        }});
-    }});
+    run_next_test();
 }
 
 function initializeAlarmService() {
     alarmObserver.service = Components.classes["@mozilla.org/calendar/alarm-service;1"]
                                        .getService(Components.interfaces.calIAlarmService)
                                        .wrappedJSObject;
     ok(!alarmObserver.service.mStarted);