366560 - Merge calendarUtils.js and calUtils.js

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Follow-up from bug 366145.  We want to merge these files into one, and just ship it twice, once as an extra script and once in a jar.

Comment 1

[Joey Minta]

Attachment: lean and mean

This patch merges calendarUtils.js into calUtils.js.  I chose to merge in this direction because (a) the prevailing direction is resources->base, not vice versa and (b) calUtils was cleaner.  Most functions that existed in calendarUtils.js were simply copied straight to calUtils.js.  In most cases, documentation was added to these functions.  A few functions, which originally had more widespread use, but now only were used in isolated spots, were moved to files where they were used.  Lastly, I used the utils to fix a bunch of > 80 char lines and ugly line-wrapping spots in the views.

Further possible cleanup: 
-remove cruft at the top of calItemModule.js (the util functions do much the same as these)
-continue use of util functions in a variety of other places, besides the views

Note that some functions can now be called by components that will actually throw if called.  (all the openDialog functions)  I wasn't sure whether we should be checking if this was the case and ASSERTing or not.

Comment 2

[Matthew (lilmatt) Willis]

Comment on attachment 251246 [details] [diff] [review]
lean and mean

>Index: calendar/base/jar.mn
>===================================================================
>     content/calendar/calendar-view-core.xml      (content/calendar-view-core.xml)
>     content/calendar/calendar-views.js           (content/calendar-views.js)
>+    content/calendar/calendarUtils.js            (src/calUtils.js)
>     content/calendar/calErrorPrompt.xul          (content/calErrorPrompt.xul)
Let's add a comment explaining that this is not a typo and allows us to share one file between components and chrome.


>Index: calendar/base/content/calendar-item-editing.js
>===================================================================
>+    if (isEvent(aItem)) {
>+        try {
>+            if (alarmsBranch.getIntPref("onforevents") == 1) {
>+                var alarmOffset = Components.classes["@mozilla.org/calendar/duration;1"]
>+                                            .createInstance(Components.interfaces.calIDuration);
>+                try {
>+                    var units = alarmsBranch.getCharPref("eventalarmunit");
>+                    alarmOffset[units] = alarmsBranch.getIntPref("eventalarmlen");
>+                    alarmOffset.isNegative = true;
>+                } catch(ex) {
>+                    alarmOffset.minutes = 15;
>+                }
>+                aItem.alarmOffset  = alarmOffset;
>+                aItem.alarmRelated = Components.interfaces.calIItemBase.ALARM_RELATED_START;
I'd rather not align the = here...

>+    } else if (isToDo(aItem)) {
>+        try {
>+            if (alarmsBranch.getIntPref("onfortodos") == 1) {
>+                // You can't have an alarm if the entryDate doesn't exist.
>+                if (!aItem.entryDate) {
>+                    aItem.entryDate = getSelectedDay().clone();
>+                }
>+                var alarmOffset = Components.classes["@mozilla.org/calendar/duration;1"]
>+                                            .createInstance(Components.interfaces.calIDuration);
>+                try {
>+                    var units = alarmsBranch.getCharPref("todoalarmunit");
>+                    alarmOffset[units] = alarmsBranch.getIntPref("todoalarmlen");
>+                    alarmOffset.isNegative = true;
>+                } catch(ex) {
>+                    alarmOffset.minutes = 15;
>+                }
>+                aItem.alarmOffset  = alarmOffset;
>+                aItem.alarmRelated = Components.interfaces.calIItemBase.ALARM_RELATED_START;
...or here


>Index: calendar/base/content/calendar-month-view.xml
>===================================================================

We should really change this to the American spelling at some point since that's what nsIFlavorDataProvider uses, but I digress...
>         var flavourProvider = {
>             QueryInterface: function(aIID) {
>-                if (aIID.equals(Components.interfaces.nsIFlavorDataProvider) ||
>-                    aIID.equals(Components.interfaces.nsISupports)) {
>+                if (aIID.equals(Ci.nsIFlavorDataProvider) ||
>+                    aIID.equals(Ci.nsISupports)) {
>                     return this;
>                 }
THIS is where I move the opening curly brace to the next line.  When the inner statement aligns with the conditionals, it helps readibility to set them apart. Please do that here.

And we now move from 4-space to 2-space indenting world...
If we fix the flavour-flav thing, we should bump these lines to 4-space since we'll be touching one of them anyway.

>       <!-- our private observers and listeners -->
> 
>       <field name="mOperationListener"><![CDATA[
>         ({
>           calView: this,
> 
>           QueryInterface: function (aIID) {
>-            if (!aIID.equals(Components.interfaces.calIOperationListener) &&
>-                !aIID.equals(Components.interfaces.nsISupports)) {
>+            if (!aIID.equals(Ci.calIOperationListener) &&
>+                !aIID.equals(Ci.nsISupports)) {
>               throw Components.results.NS_ERROR_NO_INTERFACE;
>             }
Here's another place to move the { to the next line, and can you convert to Cr here?

>@@ -1421,19 +1420,19 @@
>       ]]></field>
> 
>       <field name="mObserver"><![CDATA[
>         // the calIObserver, and calICompositeObserver
>         ({
>           calView: this,
> 
>           QueryInterface: function (aIID) {
>-            if (!aIID.equals(Components.interfaces.calIObserver) &&
>-                !aIID.equals(Components.interfaces.calICompositeObserver) &&
>-                !aIID.equals(Components.interfaces.nsISupports)) {
>+            if (!aIID.equals(Ci.calIObserver) &&
>+                !aIID.equals(Ci.calICompositeObserver) &&
>+                !aIID.equals(Ci.nsISupports)) {
>               throw Components.results.NS_ERROR_NO_INTERFACE;
>             }
Same deal here. { and Cr.

>@@ -1447,60 +1446,56 @@
>           onLoad: function() {
>             this.calView.refresh();
>           },
>           onAddItem: function (aItem) {
>             if (this.mBatchCount) {
>                 return;
>             }
> 
>-            if (aItem instanceof Components.interfaces.calITodo &&
>-                !this.calView.mTasksInView)
>+            if (isToDo(aItem) && !this.calView.mTasksInView)
>                 return;
Since you're touching this line, add the curly braces.

>           onDeleteItem: function (aItem) {
>             if (this.mBatchCount) {
>                 return;
>             }
> 
>-            if (aItem instanceof Components.interfaces.calITodo &&
>-                !this.calView.mTasksInView)
>+            if (isToDo(aItem) && !this.calView.mTasksInView)
>                 return;
Same thing here. Add curly braces.


>Index: calendar/base/content/calendar-multiday-view.xml
>===================================================================
>@@ -149,18 +148,18 @@
>               theMin += dur;
>               if (dur == 0) dur = 60;
> 
>               var box;
>               if (dur != 60) {
>                   box = makeTimeBox("", dur * this.mPixPerMin);
>               } else {
>                   timeString = formatter.FormatTime("",
>-                               Components.interfaces.nsIScriptableDateFormat
>-                               .timeFormatNoSeconds, theHour, 0, 0);
>+                               Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
>+                               theHour, 0, 0);
Man, this is just ugly no matter how you align it. I think we want Ci to align beneath "" and deal with the line overage just to improve the readibility.

>@@ -1253,24 +1252,23 @@
>-          var formatter = 
>-              Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
>-              .getService(Components.interfaces.nsIScriptableDateFormat);
>+          var formatter = Cc["@mozilla.org/intl/scriptabledateformat;1"].
>+                          getService(Ci.nsIScriptableDateFormat);
>           var startstr = formatter.FormatTime("", 
>-                             Components.interfaces.nsIScriptableDateFormat.timeFormatNoSeconds,
>+                             Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
>                              starthr, startmin, 0);
>           var endstr = formatter.FormatTime("", 
>-                           Components.interfaces.nsIScriptableDateFormat.timeFormatNoSeconds,
>+                           Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
>                            endhr, endmin, 0);
The same thing here. Align Ci beneath the "".

>@@ -1716,19 +1714,19 @@
>             QueryInterface: function QueryInterface(aIID) {
>-                if (!aIID.equals(Components.interfaces.calIObserver) &&
>-                    !aIID.equals(Components.interfaces.calICompositeObserver) &&
>-                    !aIID.equals(Components.interfaces.nsISupports)) {
>+                if (!aIID.equals(Ci.calIObserver) &&
>+                    !aIID.equals(Ci.calICompositeObserver) &&
>+                    !aIID.equals(Ci.nsISupports)) {
>                     throw Components.results.NS_ERROR_NO_INTERFACE;
>                 }
Move the { to the next line.

>@@ -1744,22 +1742,20 @@
>             onAddItem: function onAddItem(aItem) {
>                 if (this.mBatchCount) {
>                     return;
>                 }
> 
>-                if (aItem instanceof Components.interfaces.calITodo && 
>-                    (!aItem.entryDate || !aItem.dueDate))
>+                if (isToDo(aItem) && (!aItem.entryDate || !aItem.dueDate))
>                     return;
> 
>-                if (aItem instanceof Components.interfaces.calITodo &&
>-                    !this.calView.mTasksInView)
>+                if (isToDo(aItem) && !this.calView.mTasksInView)
>                     return;
Add curly braces to both of those.

>@@ -1767,33 +1763,32 @@
>             onModifyItem: function onModifyItem(aNewItem, aOldItem) {
>                 if (this.mBatchCount) {
>                     return;
>                 }
> 
>-                if (aNewItem instanceof Components.interfaces.calITodo &&
>-                    aOldItem instanceof Components.interfaces.calITodo &&
>+                if (isToDo(aNewItem) && isToDo(aOldItem) &&
>                     !this.calView.mTasksInView)
>                     return;
> 
>                 var occs;
> 
>-                if (!(aOldItem instanceof Components.interfaces.calITodo) ||
>+                if (!isToDo(aOldItem) ||
>                     (aOldItem.entryDate && aOldItem.dueDate)) {
>                     occs = aOldItem.getOccurrencesBetween(this.calView.startDate,
>                                                           this.calView.queryEndDate,
>                                                           {});
>                     for each (var occ in occs) {
>                         this.calView.doDeleteEvent(occ);
>                     }
>                 }
>-                if (aNewItem instanceof Components.interfaces.calITodo && 
>+                if (isToDo(Ci.calITodo) && 
Trailing space on this line ^^^

>@@ -1805,22 +1800,20 @@
>             onDeleteItem: function onDeleteItem(aItem) {
>                 if (this.mBatchCount) {
>                     return;
>                 }
> 
>-                if (aItem instanceof Components.interfaces.calITodo &&
>-                    !this.calView.mTasksInView)
>+                if (isToDo(Ci.calITodo) && !this.calView.mTasksInView)
>                     return;
> 
>-                if (aItem instanceof Components.interfaces.calITodo && 
>-                    (!aItem.entryDate || !aItem.dueDate))
>+                if (isToDo(Ci.calITodo) && (!aItem.entryDate || !aItem.dueDate))
>                     return;
Add curly braces to these two as well.


>Index: calendar/base/content/calendar-views.js
>===================================================================
>+/**
>+ * Returns the selected day in the views in a platform (Sunbird vs. Lightning)
>+ * neutral way
>+ */
>+function getSelectedDay() {
I'd change this comment s/platform/app/ as "platform" generally refers to Mac/Win/Unix


>Index: calendar/base/src/calItemModule.js
>===================================================================
> const componentData =
>     [
>      /* calItemBase must be first: later scripts depend on it */
>     {cid: null,
>      contractid: null,
>      script: "calItemBase.js",
>      constructor: null},
> 
>+    {cid: null,
>+     contractid: null,
>+     script: "calUtils.js",
>+     constructor: null},
>+
You probably want a similar comment for calUtils, as order is now important.


>Index: calendar/base/src/calUtils.js
>===================================================================
>+/**
>+ * We're going to do everything in our power, short of rumaging through the
>+ * user's actual file-system, to figure out the time-zone they're in.  The
>+ * deciding factors are the offsets given by (northern-hemisphere) summer and 
>+ * winter JSdates.  However, when available, we also use the name of the 
Trailing spaces on these two lines ^^^

>+ * timezone in the JSdate, or a string-bundle term from the locale.
>+ *
>+ * @returns  a ICS timezone string.
>+*/
>+function guessSystemTimezone() {
This function suffers from a severe lack of vertical whitespace.

>+    var probableTZ = null;
>+    var TZname1 = null;
>+    var TZname2 = null;
Name these variables probableTz, TzName1, and TzName2

>+    var Date1 = (new Date(2005,6,20)).toString();
>+    var Date2 = (new Date(2005,12,20)).toString();
>+    var nameData1 = Date1.match(/[^(]* ([^ ]*) \(([^)]+)\)/);
>+    var nameData2 = Date2.match(/[^(]* ([^ ]*) \(([^)]+)\)/);
>+    if (nameData1 && nameData1[2]) 
Trailing space on this line ^^
>+        TZname1 = nameData1[2];
>+    if (nameData2 && nameData2[2])
>+        TZname2 = nameData2[2];
Add curly braces to these two if statements

>+
>+    var index = Date1.indexOf('+');
>+    if (index < 0)
>+        index = Date2.indexOf('-');
Add curly braces

>+    // the offset is always 5 characters long
>+    var TZoffset1 = Date1.substr(index, 5);
Name this variable TzOffset1

>+    index = Date2.indexOf('+');
>+    if (index < 0)
>+        index = Date2.indexOf('-');
Add curly braces

>+    // the offset is always 5 characters long
>+    var TZoffset2 = Date2.substr(index, 5);
Name this variable TzOffset1

>+
>+    dump("Guessing system timezone:\n");
>+    dump("TZoffset1: " + TZoffset1 + "\nTZoffset2: " + TZoffset2 + "\n");
>+    if (TZname1 && TZname2)
>+        dump("TZname1: " + TZname1 + "\nTZname2: " + TZname2 + "\n");
Add curly braces
Do all these dumps want to be LOG()s instead?

>+
>+    var icssrv = Cc["@mozilla.org/calendar/ics-service;1"].
>+                 getService(Ci.calIICSService);
Name this variable icsSvc

>+
>+    // returns 0=definitely not, 1=maybe, 2=likely
>+    function checkTZ(someTZ)
Name this checkTz(aTz)

>+    {
We're putting the { on the same line as function up top. Pick one.

>+        var comp = icssrv.getTimezone(someTZ);
>+        var subComp = comp.getFirstSubcomponent("VTIMEZONE");
>+        var standard = subComp.getFirstSubcomponent("STANDARD");
>+        var standardTZOffset = standard.getFirstProperty("TZOFFSETTO").valueAsIcalString;
Name this standardTzOffset

>+        var standardName = standard.getFirstProperty("TZNAME").valueAsIcalString;
>+        var daylight = subComp.getFirstSubcomponent("DAYLIGHT");
>+        var daylightTZOffset = null;
Name this daylightTzOffset

>+        var daylightName = null;
>+        if (daylight) {
>+            daylightTZOffset = daylight.getFirstProperty("TZOFFSETTO").valueAsIcalString;
>+            daylightName = daylight.getFirstProperty("TZNAME").valueAsIcalString;
>+        }
>+        if (TZoffset2 == standardTZOffset && TZoffset2 == TZoffset1 &&
>+           !daylight) {
Move the { to the next line

>+            if(!standardName || standardName == TZname1)
>+                return 2;
Add curly braces and a space between if and (

>+            return 1;
>+        }
>+        if (TZoffset2 == standardTZOffset && TZoffset1 == daylightTZOffset) {
>+            if ((!standardName || standardName == TZname1) &&
>+                (!daylightName || daylightName == TZname2))
>+                return 2;
Add curly braces, with the { on the next line.

>+            return 1;
>+        }
>+
>+        // Now flip them and check again, to cover the southern hemisphere case
>+        if (TZoffset1 == standardTZOffset && TZoffset2 == TZoffset1 &&
>+           !daylight) {
Move the { to the next line

>+            if(!standardName || standardName == TZname2)
>+                return 2;
Add curly braces and a space between if and (

>+            return 1;
>+        }
>+        if (TZoffset1 == standardTZOffset && TZoffset2 == daylightTZOffset) {
>+            if ((!standardName || standardName == TZname2) &&
>+                (!daylightName || daylightName == TZname1))
>+                return 2;
Add curly braces, with the { on the next line.

>+            return 1;
>+        }
>+        return 0;
>+    }
>+    try {
Add a blank line before the try

>+        var stringBundleTZ = gCalendarBundle.getString("likelyTimezone");
Name this variable stringBundleTz

>+
>+        if (stringBundleTZ.indexOf("/mozilla.org/") == -1) {
>+            // This happens if the l10n team didn't know how to get a time from
>+            // tzdata.c.  To convert an Olson time to a ics-timezone-string we
>+            // need to append this prefix.
>+            stringBundleTZ = "/mozilla.org/20050126_1/" + stringBundleTZ;
>+        }
>+
>+        switch (checkTZ(stringBundleTZ)) {
>+            case 0: break;
Put the break on the next line.

>+            case 1: 
Trailing space on this line ^^^

>+                if (!probableTZ)
>+                    probableTZ = stringBundleTZ;
Add curly braces.

>+                break;
>+            case 2:
>+                return stringBundleTZ;
>+        }
>+    }
>+    catch(ex) { } //Oh well, this didn't work, next option...
Add a space between catch and (, put the } on the next line, and put the comment (with a space between // and Oh) in between the braces.

>+        
Trailing spaces on this line ^^

>+    var tzIDs = icssrv.timezoneIds;
Name this variable tzIds

>+    while (tzIDs.hasMore()) {
>+        var theTZ = tzIDs.getNext();
Name this variable theTz

>+        switch (checkTZ(theTZ)) {
>+            case 0: break;
Put the break on the next line.

>+            case 1: 
Trailing space on this line ^^^

>+                if (!probableTZ)
>+                    probableTZ = theTZ;
Add curly braces.

>+                break;
>+            case 2:
>+                return theTZ;
>+        }
>+    }
>+    // If we get to this point, should we alert the user?
Add a blank line before the comment.

>+    if (probableTZ)
>+        return probableTZ;
Add curly braces and a blank line after.

>+    // Everything failed, so this is our only option.
>+    return "floating";
>+}
>+
>+/**
>+ * Shared dialog functions
>+ */
>+
>+/**
>+ * Opens the Create Calendar wizard 
Trailing space on this line ^^

>+ *
>+ * @param aCallback  a function to be performed after calendar creation
>+ */
>+function openCalendarWizard(aCallback) {
>+    openDialog("chrome://calendar/content/calendarCreation.xul", "caEditServer",
We should change "caEditServer" to Calendar:CalendarProperties.  We need to add windowtype="Calendar:CalendarProperties" to calendarProperties.xul to make it work correctly though.

>+               "chrome,titlebar,modal", aCallback);
>+}
>+
>+/**
>+ * Opens the calendar properties window for aCalendar
>+ *
>+ * @param aCalendar  the calendar whose properties should be displayed
>+ * @param aCallback  function that should be run when the dialog is accepted
>+ */
>+function openCalendarProperties(aCalendar, aCallback) {
>+    openDialog("chrome://calendar/content/calendarProperties.xul",
>+               "caEditServer", "chrome,titlebar,modal",
See the comment above.

>+               {calendar: aCalendar, onOk: aCallback});
>+}
>+
>+/**
>+ * Opens the print dialog
>+ */
>+function calPrint() {
>+    openDialog("chrome://calendar/content/printDialog.xul", "Print",
Same deal here. We should add windowtype="Calendar:Print" to printDialog.xul, and use "Calendar:Print" here.  Doing so will bring an already open print window to the foreground if the user selects print again, preventing us from having multiple print dialog windows open at any one time.

>+               "centerscreen,chrome,resizable");
>+}
>+
>+/**
>+ * Other functions
>+ */
>+
>+/**
>+ * Takes a string and returns an nsIURI
>+ *
>+ * @param aUriString  the string of the address to for the spec of the nsIURI
>+ *
>+ * @returns  an nsIURI whose spec is aUriString
>+ */
>+function makeURL(aUriString) {
>+    var ioservice = Cc["@mozilla.org/network/io-service;1"].
>+                    getService(Ci.nsIIOService);
>+    return ioservice.newURI(aUriString, null, null);
Name the var ioSvc

>+}
>+
>+/**
>+ * Returns a calIDateTime that corresponds to the current time in the user's
>+ * default timezone.
>+ */
>+function now() {
>+    var d = createDateTime();
>+    d.jsDate = new Date();
>+    return d.getInTimezone(calendarDefaultTimezone());
>+}
>+
>+/**
>+ * Returns a calIDateTime corresponding to a javascript Date
>+ *
>+ * @param aDate  a javascript date
>+ * @returns      a calIDateTime whose jsDate is aDate
>+ *
>+ * @warning  Use of this function is strongly discouraged.  calIDateTime should
>+ *           be used directly whenever possible.
>+ */
>+function jsDateToDateTime(aDate) {
>+    var newDate = createDateTime();
>+    newDate.jsDate = aDate;
>+    return newDate;
>+}
>+
>+/**
>+ * Returns a calIDateTime with a floating timezone and each field the same as
>+ * the equivalent field of the javascript date aDate
>+ *
>+ * @param aDate a javascript date
>+ * @returns     a calIDateTime with all of the fields the same as aDate
>+ *
>+ * @warning  Like jsDateToDateTime, use of the function is strongly discouraged.
>+ */
>+function jsDateToFloatingDateTime(aDate) {
>+    var newDate = createDateTime();
>+    newDate.timezone = "floating";
>+    newDate.year = aDate.getFullYear();
>+    newDate.month = aDate.getMonth();
>+    newDate.day = aDate.getDate();
>+    newDate.hour = aDate.getHours();
>+    newDate.minute = aDate.getMinutes();
>+    newDate.second = aDate.getSeconds();
>+    newDate.normalize();
>+    return newDate;
>+}
>+
>+/**
>+ * Selects an item with id aItemId in the radio group with id aRadioGroupId
>+ *
>+ * @param aRadioGroupId  the id of the radio group which contains the item
>+ * @param aItemId        the item to be selected
>+ */
>+function calRadioGroupSelectItem(aRadioGroupId, aItemId) {
>+    var radioGroup = document.getElementById(aRadioGroupId);
>+    var items = radioGroup.getElementsByTagName("radio");
>+    var index;
>+    for (var i in items) {
>+        if (items[i].getAttribute("id") == aItemId) {
>+            index = i;
>+            break;
>+        }
>+    }
>+    ASSERT(index && index != 0, "Can't find radioGroup item to select.", true);
>+    radioGroup.selectedIndex = index;
>+}
>+
>+/**
>+ * Determines whether or not the aObject is a calIEvent
>+ *
>+ * @param aObject  the object to test
>+ * @returns        true if the object is a calIEvent, false otherwise
>+ */
>+function isEvent(aObject) {
>+    return aObject instanceof Ci.calIEvent;
>+}
>+
>+/**
>+ * Determines whether or not the aObject is a calITodo
>+ *
>+ * @param aObject  the object to test
>+ * @returns        true if the object is a calITodo, false otherwise
>+ */
>+function isToDo(aObject) {
>+    return aObject instanceof Ci.calITodo;
>+}
>+
>+
Remove one of these blank lines.
>+/**
>  * Normal get*Pref calls will throw if the pref is undefined.  This function



>Index: calendar/resources/content/mouseoverPreviews.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/resources/content/mouseoverPreviews.js,v
>retrieving revision 1.14.2.5
>diff -p -U8 -r1.14.2.5 mouseoverPreviews.js
>--- calendar/resources/content/mouseoverPreviews.js	23 Aug 2006 16:53:23 -0000	1.14.2.5
>+++ calendar/resources/content/mouseoverPreviews.js	12 Jan 2007 03:27:59 -0000
>@@ -412,8 +412,35 @@ function createTooltipHeaderLabel(text)
> 
> function createTooltipHeaderDescription(text)
> {
>   var label = document.createElement("description");
>   label.setAttribute("class", "tooltipHeaderDescription");
>   label.appendChild(document.createTextNode(text));
>   return label;
> }
>+
>+/** If now is during an occurrence, return the ocurrence.
>+    Else if now is before an ocurrence, return the next ocurrence.
>+    Otherwise return the previous ocurrence. **/
>+function getCurrentNextOrPreviousRecurrence(calendarEvent)
Change calendarEvent to aEvent

>+{
>+    if (!calendarEvent.recurrenceInfo) {
>+        return calendarEvent;
>+    }
>+
>+    var dur = calendarEvent.duration.clone();
>+    dur.isNegative = true;
>+
>+    // To find current event when now is during event, look for occurrence
>+    // starting duration ago.
>+    var probeTime = now();
>+    probeTime.addDuration(dur);
>+
>+    var occ = calendarEvent.recurrenceInfo.getNextOccurrence(probeTime);
>+
>+    if (!occ) {
>+        var occs = calendarEvent.recurrenceInfo.getOccurrences(calendarEvent.startDate, probeTime, 0, {});
>+        occ = occs[occs.length -1];
>+    }
>+    return occ;
>+}
>+
Remove the trailing blank line. We want a newline at the EOF, but not extra ones.



All of these are style nits.  r=lilmatt with them

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 251246 [details] [diff] [review]
lean and mean

>+    content/calendar/calendarUtils.js            (src/calUtils.js)

That change scares me a lot. I know it makes the patch a lot smaller, but it will make reading the files referencing calendarUtils.js much more confusing, since the filename it references to does not exists in LXR. Very confusing. So I tend to prefer a long patch here and properly name the file.

Comment 4

[Joey Minta]

Attachment: unify v2

This patch replaces calendarUtils.js with calUtils.js when it's included in <script>s.  It also incorporates most of the nits from the first review.  The following nits were not incorporated:
-Moving braces to the own lines for multiple line if() blocks.  Braces are on the ends of lines everywhere else, and this breaks that guideline.
-Renaming TZ to Tz throughout the timezone guessing function.  TZ is an abbreviation, not a word, so it should remain in caps (like ICS, etc).
-Renaming the windowtypes in openDialog functions.  This belongs in a new bug where we can actually test the changes.
-Renaming calendarEvent to aEvent in mouseover-previews.js.  The rest of the file does not use the a-prefix for arguments, and in-file consistency seems more important to me.

Comment 5

[Matthew (lilmatt) Willis]

(In reply to comment #4)
> The following nits were not incorporated:
> -Moving braces to the own lines for multiple line if() blocks.  Braces are on
> the ends of lines everywhere else, and this breaks that guideline.
We should make a decision on this.  We both say that our way "is better for readibility".

> -Renaming TZ to Tz throughout the timezone guessing function.  TZ is an
> abbreviation, not a word, so it should remain in caps (like ICS, etc).
AFAIK, in CamelCase, abbreviations should NOT be in caps. Only the first letter should be.  Note the capitalization of calHtmlExporter, calWcapCalendar, calIItipItem and calIIcsParser.  I know we have calICSCalendar and others, but I consider those erroneously named. 

> -Renaming the windowtypes in openDialog functions.  This belongs in a new bug
> where we can actually test the changes.
Cool. Please file a follow up.

> -Renaming calendarEvent to aEvent in mouseover-previews.js.  The rest of the
> file does not use the a-prefix for arguments, and in-file consistency seems
> more important to me.
Fair enough.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 251466 [details] [diff] [review]
unify v2

r2=mvl. nice cleanup!

Comment 7

[Joey Minta]

Patch checked in.  Please file follow-ups for any regressions.  For the record, trunk changes look like:

  calendar/base/src/calItipItem.js: 1.3 -> 1.4.
  calendar/base/src/calAlarmService.js: 1.23 -> 1.24.
  calendar/base/src/calItemModule.js: 1.22 -> 1.23.
  calendar/base/src/calUtils.js: 1.11 -> 1.12.
  calendar/sunbird/base/content/calendar-scripts.inc: 1.17 -> 1.18.
  calendar/base/jar.mn: 1.2 -> 1.3.
  calendar/resources/content/printDialog.xul: 1.16 -> 1.17.
  calendar/resources/content/mouseoverPreviews.js: 1.20 -> 1.21.
  calendar/resources/content/calendarProperties.xul: 1.7 -> 1.8.
  calendar/resources/content/publishDialog.xul: 1.10 -> 1.11.
  calendar/resources/content/calendarCreation.xul: 1.13 -> 1.14.
  calendar/resources/content/calendarManagement.js: 1.33 -> 1.34.
  calendar/base/content/calendar-view-core.xml: 1.11 -> 1.12.
  calendar/base/content/calendar-event-dialog.xul: 1.35 -> 1.36.
  calendar/base/content/migration.js: 1.10 -> 1.11.
  calendar/base/content/calendar-decorated-month-view.xml: 1.11 -> 1.12.
  calendar/base/content/calendar-multiday-view.xml: 1.118 -> 1.119.
  calendar/base/content/calendar-item-editing.js: 1.28 -> 1.29.
  calendar/base/content/calendar-decorated-day-view.xml: 1.15 -> 1.16.
  calendar/base/content/calendar-publish-dialog.xul: 1.1 -> 1.2.
  calendar/base/content/calendar-month-view.xml: 1.65 -> 1.66.
  calendar/base/content/calendar-recurrence-dialog.xul: 1.20 -> 1.21.
  calendar/base/content/calendar-views.js: 1.17 -> 1.18.
  calendar/base/content/calendar-decorated-week-view.xml: 1.19 -> 1.20.
  calendar/base/content/calendar-alarm-dialog.xul: 1.7 -> 1.8.
  calendar/base/content/calendar-decorated-multiweek-view.xml: 1.21 -> 1.22.
  calendar/resources/jar.mn: 1.147 -> 1.148.
  calendar/lightning/content/lightning-migration.xul: 1.1 -> 1.2.
  calendar/lightning/content/messenger-overlay-sidebar.xul: 1.49 -> 1.50.
  calendar/prototypes/wcap/sun-calendar-event-dialog-attendees.xul: 1.3 -> 1.4.
  calendar/prototypes/wcap/sun-calendar-event-dialog.xul: 1.7 -> 1.8.
  calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence.xul: 1.3 -> 1.4.
  calendar/prototypes/wcap/calendar-invitations-dialog.xul: 1.2 -> 1.3.
  calendar/prototypes/wcap/sun-calendar-event-dialog-timezone.xul: 1.3 -> 1.4.
  calendar/prototypes/wcap/sun-calendar-event-dialog-reminder.xul: 1.4 -> 1.5.
  calendar/lightning/jar.mn: 1.64 -> 1.65.
  calendar/base/content/preferences/timezones.xul: 1.3 -> 1.4.
  calendar/base/content/preferences/general.xul: 1.5 -> 1.6.

Comment 8

[Matthew (lilmatt) Willis]

We missed /m/c/base/content/migrator.xul.  r=jminta via IRC to s/calendarUtils/calUtils

checked in on MOZILLA_1_8_BRANCH and trunk.

Comment 9

[Stefan Sitter [:ssitter]]

Regression with Sunbird 0.4a1 (2007021308): Can't create new tasks:

Error: createToDo is not defined 
Source File: chrome://calendar/content/calendar-item-editing.js Line: 110