368121 - add code to update old tzids

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

When the calendar engine encounters /mozilla.org/ tzids with datestamps earlier than the one in the current build, it should automagically update the tzids to the most current timestamp.  This includes both in events and in preferences.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: work in progress, v1

Comment 2

[cmtalbert]

Attachment: Work in progress - change to AUTF8String from AString

Comment 3

[Matthew (lilmatt) Willis]

Here's the list of "goofy changed timezones":

old: "/mozilla.org/20050126_1/Africa/Asmera",
new: "/mozilla.org/20070129_1/Africa/Asmara"

old: "/mozilla.org/20050126_1/Atlantic/Faeroe",
new: "/mozilla.org/20070129_1/Atlantic/Faroe"

old: "/mozilla.org/20050126_1/Africa/Timbuktu",
new: "/mozilla.org/20070129_1/Africa/Bamako"

old: "/mozilla.org/20050126_1/America/Argentina/ComodRivadavia",
new: "/mozilla.org/20070129_1/America/Argentica/Catamarca"

old: "/mozilla.org/20050126_1/America/Louisville",
new: "/mozilla.org/20070129_1/America/Kentucky/Louisville"

old: "/mozilla.org/20050126_1/Europe/Belfast",
new: "/mozilla.org/20070129_1/Europe/London"

old: "/mozilla.org/20050126_1/Pacific/Yap",
new: "/mozilla.org/20070129_1/Pacific/Truk"

Comment 4

[cmtalbert]

Attachment: Works to update zones - is ugly and needs love

Comment 5

[cmtalbert]

Attachment: Updates timezone IDs

This patch should work to update timezone IDs. Asking Matt to take a look for a first review. Note that the patch from 321653 must be applied AFTER this patch. Otherwise, this patch won't update anything (gTzidPrefix will still be a 2005 style ID's).

Comment 6

[Matthew (lilmatt) Willis]

Attachment: All changes for this bug plus new tz db

This patch combines the fixes for upgrading to new tzids and the new tz db itself.

Comment 7

[cmtalbert]

Comment on attachment 253300 [details] [diff] [review]
All changes for this bug plus new tz db

Nice work!

Comment 8

[cmtalbert]

(In reply to comment #7)
> (From update of attachment 253300 [details] [diff] [review])
> Nice work!
> 
Little too click happy...tested with Africa/Timbuktu, Atlantic/Faeore and America/Cancun timezones on my Mac OS X with Sunbird trunk build. Patch does indeed seem to work and looks good.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 253300 [details] [diff] [review]
All changes for this bug plus new tz db


Looks good; nice work!  My only suggestions are some added commentary for future maintainability.

r=dmose with the requested comment changes

>+void calTzId::Assign(char* c)
>+{
>+    nsCString::Assign(c);
>+}
>+
>+void calTzId::Assign(const nsACString_internal& aStr)
>+{

Please add a comment explaining why it's necessary to use nsACString_internal here.   Also add a comment explaining why the two signatures of ::Assign behave differently.  

>Index: calendar/base/src/calDateTime.h
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calDateTime.h,v
>retrieving revision 1.10.2.1
>diff -u -8 -p -r1.10.2.1 calDateTime.h
>--- calendar/base/src/calDateTime.h	19 Sep 2006 20:39:30 -0000	1.10.2.1
>+++ calendar/base/src/calDateTime.h	30 Jan 2007 07:07:54 -0000
>@@ -44,16 +44,23 @@
> #include "jsapi.h"
> #include "nsIXPCScriptable.h"
> 
> #include "calIDateTime.h"
> 
> struct icaltimetype;
> struct _icaltimezone;
> 
>+class calTzId : public nsCString
>+{
>+public:
>+    void Assign(char* c);
>+    void Assign(const nsACString_internal& aStr);
>+};

Please add comments explaining what this class and this methods are for.

>Index: calendar/base/src/calICSService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calICSService.cpp,v
>retrieving revision 1.20.2.9
>diff -u -8 -p -r1.20.2.9 calICSService.cpp
>--- calendar/base/src/calICSService.cpp	6 Nov 2006 20:21:46 -0000	1.20.2.9
>+++ calendar/base/src/calICSService.cpp	30 Jan 2007 07:07:54 -0000
>@@ -1272,17 +1272,17 @@ calICSService::CreateIcalProperty(const 
> 
> /**
>  ** Timezone bits
>  **/
> 
> // include tzdata, to get ical_timezone_data_struct
> #include "tzdata.c"
> 
>-#define MOZCAL_TZID_PREFIX "/mozilla.org/20050126_1/"
>+#define MOZCAL_TZID_PREFIX "/mozilla.org/20070129_1/"

Add an XXX comment pointing out that we should really be using the global that we defined in tzdata.c here, if you would.


>@@ -1391,8 +1391,90 @@ NS_IMETHODIMP
> calICSService::GetTimezoneLongitude(const nsACString& tzid, nsACString& _retval)
> {
>     TimezoneEntry const* pEntry = getTimezoneEntry(tzid);
>     if (pEntry == nsnull)
>         return NS_ERROR_FAILURE;
>     _retval = pEntry->mLongitude;
>     return NS_OK;
> }
>+
>+NS_IMETHODIMP
>+calICSService::GetTzIdPrefix(nsACString& _retval)
>+{
>+    _retval.Assign(gTzIdPrefix);
>+
>+    return NS_OK;
>+}
>+
>+/**
>+ * LatestTzId
>+ * Gets updated timezone ID for this zone.
>+ * if this isn't a mozilla.org zone
>+ *     return untouched
>+ *
>+ * if the zone locale was changed or the zone was deleted since our last update
>+ *     construct new tzId for this zone
>+ *
>+ * if the dstamp of the zone is different from our current dtstamp
>+ *     construct new tzId for this zone with new dtstamp
>+ *
>+ * otherwise, we do not change the tzId and return nothing.

It doesn't look to me like the above comment really describes what the method does.  Specifically, the tzid itself never gets changed at all, we simply decide whether or not to construct a new, updated version and return it.  As part of this, the first and last cases described above do exactly the same thing: nothing.

>+ */
>+NS_IMETHODIMP
>+calICSService::LatestTzId(const nsACString& tzid, nsACString& _retval) {
>+    // If it doesn't start with "/mozilla.org/" then it isn't ours.
>+    if (!StringBeginsWith(
>+            tzid, nsDependentCString(STRLEN_ARGS("/mozilla.org/")))) {
>+        return NS_OK;
>+    }
>+
>+    // We know that the zone ID is "/mozilla.org/<some date>/continent/..."
>+    // So ending of the mozilla prefix is the index of that slash before the
>+    // continent. Therefore, we start looking for the prefix-ending slash
>+    // after position 13.
>+    PRInt32 prefixEnd = tzid.FindChar('/', 13);
>+    PRInt32 continentEnd = tzid.FindChar('/', prefixEnd + 1);
>+
>+    // Go through our list of deletions and changes in Olsen, and update
>+    // these to entirely new zones.

Probably good to add an XXX comment here that at some point in the future, we'll want to make this table-driven.

>Index: calendar/resources/content/calendarUtils.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/resources/content/calendarUtils.js,v
>retrieving revision 1.12.2.14
>diff -u -8 -p -r1.12.2.14 calendarUtils.js
>--- calendar/resources/content/calendarUtils.js	30 Oct 2006 21:10:47 -0000	1.12.2.14
>+++ calendar/resources/content/calendarUtils.js	30 Jan 2007 07:07:57 -0000
>@@ -394,17 +402,17 @@ function guessSystemTimezone()
>     }
>     try {
>         var stringBundleTZ = gCalendarBundle.getString("likelyTimezone");
> 
>         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;
>+            stringBundleTZ = "/mozilla.org/20070129_1/" + stringBundleTZ;

Add an XXX comment here that we should really be using calIICSService.tzIdPrefix to figure this out, too.

Comment 10

[Matthew (lilmatt) Willis]

Nits addressed including the following one that dmose forgot to put in bugzilla:
> since "continent" is being created on the stack, using nsCAutoString instead of
> nsCString

Patch checked in on SUNBIRD_0_3_BRANCH and LIGHTNING_0_3_BRANCH.

Once we smoketest 0.3.1pre builds with this patch, we can consider landing on MOZILLA_1_8_BRANCH and trunk.

-> [fixed0.3.1]

Comment 11

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

Comment on attachment 253300 [details] [diff] [review]
All changes for this bug plus new tz db

>Index: calendar/base/src/calDateTime.h
> class calDateTime : public calIDateTime,
>                     public nsIXPCScriptable
>-    nsCString mTimezone;
>+    calTzId mTimezone;

Why-oh-why is this a new class. What was wrong with the normal string?

>Index: calendar/base/src/calICSService.cpp
>+NS_IMETHODIMP
>+calICSService::LatestTzId(const nsACString& tzid, nsACString& _retval) {
>+    // If it doesn't start with "/mozilla.org/" then it isn't ours.
>+    if (!StringBeginsWith(
>+            tzid, nsDependentCString(STRLEN_ARGS("/mozilla.org/")))) {
>+        return NS_OK;
You must assign something to _retval here. Otherwise, it will be undefined. That might be ok in case of an error, but you return a success code. That means the return value must have some meaning.


>+    if (continent == NS_LITERAL_CSTRING("Africa")) {

I wonder if |continent.EqualsLiteral("Africa")| would be any faster. At least it would be more readable, due to less uppercase shouting.
Same goes for all the following string comparisons.

>+    if ((_retval.Length() == 0) &&
You don't know what _retval was, so you can't assume it's empty. You never initialized it.

Comment 12

[Matthew (lilmatt) Willis]

14:25 <@mvl> lilmatt: ehm, your new timezone definitions are wrong
14:25 <@lilmatt> How so
14:25 <@mvl> you changed history
14:25 <@lilmatt> Right 
14:25 <@mvl> for the changed timezones, you should split the definition in two parts
14:25 <@mvl> before and after the change
14:26 <@mvl> the tz compiler from olson does support that
14:26 <@lilmatt> Ya we should, but that breaks Outlook interop as Outlook can only deal with 2 RRULES in a VTIMEZONE... one for DAYLIGHT and one for STANDARD
14:26 <@lilmatt> We'd need 4
14:26 <@mvl> we never used it, because we don't really care about 1930. But we do care about 2006
14:26 <@lilmatt> dmose asserted that interop with outlook will be felt my more users than users who look at events in the past, in the one month window where DST is wrong
14:26 <@mvl> does outlook still not support that?
14:27 <@mvl> was is tested?
14:27 <@lilmatt> Outlook 2K doesn't, and Simdeks's research shows that 2K still has significant penetration in the SMB and "I didn't pay for this" market


Rationale for not creating technically correct timezone updates (maintaining the pre-2007 DST timezones)

Comment 13

[Matthew (lilmatt) Willis]

Attachment: Patch as checked in on MOZILLA_1_8_BRANCH and trunk

Comment 14

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 16

[cmtalbert]

(In reply to comment #12)
> 14:25 <@mvl> lilmatt: ehm, your new timezone definitions are wrong
> 14:25 <@lilmatt> How so
> 14:25 <@mvl> you changed history
...snip...
> Rationale for not creating technically correct timezone updates (maintaining
> the pre-2007 DST timezones)
> 

Tested on Outlook 2000 and Outlook 2003 --> CANNOT import an ICS file with multiple RRULES defined in the VTIMEZONE block.

Outlook 2007 CAN import an ICS file with multiple RRULES defined in the VTIMEZONE block.