363191 - Handle Timezones more efficiently (Timezone Database)

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Code has to be written to specify a way to handle timezones generally. This must include:

* A way to guess the current System timezone (Can use code from )
* A way to guess the Timezone from a Timezone offset (Used in Google provider)
  - Get all timezones with the specified offset
  - Return the preference timezone if it has the same offset
  - Otherwise return any timezone from that list
* A way to efficiently get the full TZID from a short name like "Europe/Berlin"
* This Bug should also include bug 314339. (Handle foreign TZIDs)

An Article should be written in the Wiki about the full specifications. dmose has some ideas on this.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The missing link for the first point is

http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendarUtils.js#319

Comment 2

[cmtalbert]

We are going to address this.  Confirming.

Comment 3

[cmtalbert]

Phillip, please clarify these points:

> * A way to guess the Timezone from a Timezone offset (Used in Google provider)
>   - Get all timezones with the specified offset
>   - Return the preference timezone if it has the same offset
>   - Otherwise return any timezone from that list
Do you refer to the currently applicable (not historical), standard UTC offset for that zone?  For example, you are *not* requesting the ability to search on the daylight offset for that zone(if it exists), are you?

And preference timezone is the "timezone currently set in the preferences dialog" right?

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Good question actually, I haven't really though about that. I took a look in rfc 3339 and as far as I understand it, the offset is the current offset to UTC, taking DST into account. I can ask the Google newsgroup how they think of this.


Yes, the preference timezone is the "timezone currently set in the preferences
dialog".

Comment 5

[cmtalbert]

If possible, I would rather NOT provide functionality to look up the timezone based on its "current" UTC offset, because at different times of the year the listing of zones returned by such a search will change.  Furthermore, if we provide that functionality, it is one more piece of code that *will* break when our timezones are out of date.  I would rather provide functionality to search for a zone based on its offset as defined in its STANDARD section of its timezone definition.  That way we can generate a complete list that will be the same no matter when the search is performed.

Comment 6

[Daniel Boelzle [:dbo]]

I don't see a valid reason why this blocks 0.8, though it's of course wanted for 0.8. Philipp, could you please elaborate why this blocks gdata recurrency support?

Comment 7

[Simon Paquet [:sipaq]]

Not going to happen for 0.8

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

As I helped adapt the 2007k Olson database for 0.8, I would love to be able to help regarding timezones.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: calendar/timezones/timezones.sqlite

This is a timezones.sqlite derived out of the current tzdata which is reflected by the calITimezoneService.

Comment 11

[Daniel Boelzle [:dbo]]

Attachment: calendar/timezones/first_db.js

This is the script I've used to create the timezones.sqlite.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: WIP patch - v1

This patch adds a calendar-timezones.xpi which comes with a
- timezones.sqlite
- calendar-timezones-ab_CD.jar (having the corresponding l10n'ed names in a fresh timezones.properties)
- enhances calITimezone to offer the displayName (l10n'ed) of a timezone
- rips out the current calTimezoneService.cpp, tzdata.c, maketzdata.pl
- removes our mozilla.org prefix while keeping these kind of names still resolvable
- adds a js implementation reading from the sqlite db
- adopts timezones preference pane to fill up dynamically
- adopts timezone chooser of event dialog to fill up dynamically
- fixes to show display names rather than raw TZID names in the event dialog
- minor drive-by code improvements (really, I promise)
- adopts provider code where necessary
- has a preliminary calendar/timezones/update.js to do updates of timezones.sqlite reading from vzic'ed zonesinfo. The script also balances out the timezones.properties to show what needs to be added/removed. This works for me, but for sure needs some more testing and verification. We may take that over to a different bug.

The patch is yet not ready for review (e.g. still has problems preinstalling calendar-timezones.xpi for sunbird), but I'd like to encourage everybody to have a look (especially at the sqlite schema) to give early feeback. Thanks in advance!

Comment 13

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

Working patch; requesting a first review. Clint, if you have time, please have a look, too.

There are some open questions/enhancements/bugs to be discussed or filed, grep for 'xxx todo' in the patch.

Comment 14

[Martin Schröder [:mschroeder]]

Comment on attachment 322744 [details] [diff] [review]
patch - v2

>Index: calendar/base/src/calTimezoneService.js
>===================================================================
>RCS file: calendar/base/src/calTimezoneService.js
>diff -N calendar/base/src/calTimezoneService.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ calendar/base/src/calTimezoneService.js	28 May 2008 08:21:16 -0000
[...]
>+    this.mTimezoneCache.UTC = this.UTC;
>+    this.mTimezoneCache.utc = this.UTC;

I think, the second line can go away.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 322744 [details] [diff] [review]
patch - v2

>+        var tzMenuPopup = tzMenuList.firstChild;
...
>         <menulist id="calendar.timezone.menulist"
>                   preference="calendar.timezone.local">
>+            <menupopup/>
>         </menulist>
I'd prefer also giving the menupopup an id, for the sake of extensibility, and then referencing that. Also it might be good to align naming of the menulist id to use dashes instead of periods.

>+[scriptable, uuid(D79161E7-0DB9-427d-A0C3-27E0DB3B030F)]
I believe our hex letters were always small, but I don't really mind.


>+    /**
>+     * The timezone provider this timezone belongs to, if any.
>+     */
>+    readonly attribute calITimezoneProvider provider;
I haven't read all of the patch, but what about circular references? I assume that the timezone provider holds the timezones, while the timezones reference the timezone provider?

It seems most of this interface is readonly, but I am missing a comment as to why this is the case. From a novice perspective, I'd assume its possible to change certain aspects of the timezone programatically. It would be great if you could include some comments how it is possible.

>Index: calendar/base/src/calAlarmService.js
>Index: calendar/base/src/calCalendarManager.js
>Index: calendar/base/src/calCalendarSearchService.js
>Index: calendar/base/src/calFreeBusyService.js

This hunk is already checked in :-)

>Index: calendar/base/src/calDateTime.cpp
>+            NS_ASSERTION(SameCOMIdentity(mTimezone, cal::UTC()), "UTC mismatch!");
From what I remember in irc backlog, this was in some util header. Was it already included?

> NS_IMETHODIMP
> calDuration::SetIcalString(const nsACString& aIcalString)
> {
>-    mDuration = icaldurationtype_from_string(nsPromiseFlatCString(aIcalString).get());
>+    mDuration = icaldurationtype_from_string(PromiseFlatCString(aIcalString).get());
>     return NS_OK;
> }
> calPeriod::SetIcalString(const nsACString& aIcalString)
> {
>     if (mImmutable)
>         return NS_ERROR_OBJECT_IS_IMMUTABLE;
>     struct icalperiodtype ip;
>-    ip = icalperiodtype_from_string(nsPromiseFlatCString(aIcalString).get());
>+    ip = icalperiodtype_from_string(PromiseFlatCString(aIcalString).get());

Reading http://developer.mozilla.org/en/docs/Migrating_from_Internal_Linkage_to_Frozen_Linkage It says PromiseFlatString classes are not around anymore. Is this also the case with PromiseFlatCString? Do we need to get rid of this?

>Index: calendar/base/src/calTimezone.cpp
This class looks fairly straightforward. Any reason you didn't implement it in js?

>Index: calendar/base/src/calTimezoneService.js
>+var g_stringBundle = null;
Do we really need to use a string bundle global? What about calGetString?

>+function calStringEnumerator(stringArray) {
>+    this.mIndex = 0;
>+    this.mStringArray = stringArray;
>+}
>+calStringEnumerator.prototype = {
>+    // nsIUTF8StringEnumerator:
>+    hasMore: function calStringEnumerator_hasMore() {
>+        return (this.mIndex < this.mStringArray.length);
>+    },
>+    getNext: function calStringEnumerator_getNext() {
>+        if (!this.hasMore()) {
>+            throw Components.results.NS_ERROR_UNEXPECTED;
>+        }
>+        return this.mStringArray[this.mIndex++];
>+    }
>+};
You could possibly make use of js 1.7 iterators here instead. See bug 418490 for utils. Otherwise isn't this something we should rather put in calUtils or similar?


>+    ensureInited: function calTimezoneService_init() {
To be pedantic, ensureInitialized sounds better :-)



>+            try {
>+                sqlTzFile.append("timezones.sqlite");
>+                var dbService = Components.classes["@mozilla.org/storage/service;1"]
>+                                          .getService(Components.interfaces.mozIStorageService);
>+                this.mDb = dbService.openDatabase(sqlTzFile);
>+
>+                var statement = this.mDb.createStatement("SELECT * FROM tz_data WHERE tzid == :tzid");
>+                this.mSelectByTzid = Components.classes["@mozilla.org/storage/statement-wrapper;1"]
>+                                               .createInstance(Components.interfaces.mozIStorageStatementWrapper);
>+                this.mSelectByTzid.initialize(statement);
>+
>+                g_stringBundle = calGetStringBundle(bundleURL);
So much in a try block? the append and var dbService can probably be safely done outside.

>+                LOG(msg);
>+                Components.utils.reportError(msg);
>+                showError(msg);
You really want this error to show, eh? ;-)

> // some static timezone helpers, we leak those, but this is ok since the
> // underlying service leaks those anyway until process termination
>+# xxx todo discuss: should we better move this file either into
>+#     	   	    - calendar/locales/en-US/chrome/calendar/
I prefer ^^

>+#                or - calendar/locales/en-US/chrome/calendar-timezones/   ?

>+extensions.{d0f7a1d4-2a2f-4b7b-a6a9-aa8059fb4b7e}.name=Timezone Definitions for Mozilla Calendar adlkjadlkj
For Mozilla Calendar what!?

>+# I've derived this list out timezones.properties
>+# - replaced '_' with ' ' on value side
>+# - xxx todo: what about 'St xyz'? Shouldnt we call those 'St. xyz'?
St. sounds correct to me for displaying.


r=philipp

Comment 16

[Stefan Sitter [:ssitter]]

Some drive by thoughts on the extension:

+    <em:id>{d0f7a1d4-2a2f-4b7b-a6a9-aa8059fb4b7e}</em:id>
+    <em:version>0.1</em:version>

As we create a new extension we can move from uuid to the recommended and more readable [e-mail] format.

As we are based on the Olson database I'd like to see the corresponding version somewhere, e.g. as part of the extension version (maybe like 0.1.2008b?) This would help to tell what version the extension is based on and if an update is required.

I think we also need to specify the required Lightning (also Sunbird via extension stub) version to ensure that the timezone extension works with it.

+        <!-- Thunderbird -->
+        <em:id>{3550f703-e582-4d05-9a08-453d09bdfdc6}</em:id>
+        <em:minVersion>2.0a1</em:minVersion>
+        <em:maxVersion>999999</em:maxVersion>

At least when hosting on a.m.o this needs to be valid values, e.g. 2.0.0.0 to 2.0.0.* that match the supported Lightning version. For example I don't want users to install that extension in Thunderbird 3 just to recognize that it won't work on Trunk and Lightning 0.9pre is required.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

With this patch the timezone service goes into js land, so we can move calendarDefaultTimezone() into the service, to fix bug 413847. Should obviously be done in that bug though.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #14)
> >+    this.mTimezoneCache.UTC = this.UTC;
> >+    this.mTimezoneCache.utc = this.UTC;
> 
> I think, the second line can go away.
We can't know if any data still relies on that, so IMO we need to keep it resolvable. I doubt that there is some data, or has any former Sunbird/Lightning version written lower-case "utc" e.g. as tzid into storage.sdb?

Comment 19

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> (From update of attachment 322744 [details] [diff] [review])
> >+        var tzMenuPopup = tzMenuList.firstChild;
> ...
> >         <menulist id="calendar.timezone.menulist"
> >                   preference="calendar.timezone.local">
> >+            <menupopup/>
> >         </menulist>
> I'd prefer also giving the menupopup an id, for the sake of extensibility, and
> then referencing that. Also it might be good to align naming of the menulist id
> to use dashes instead of periods.
done that

> >+    /**
> >+     * The timezone provider this timezone belongs to, if any.
> >+     */
> >+    readonly attribute calITimezoneProvider provider;
> I haven't read all of the patch, but what about circular references? I assume
> that the timezone provider holds the timezones, while the timezones reference
> the timezone provider?
No, calIntrinsicTimezone doesn't hold its provider (i.e. the timezone service). It uses/returns calUtils' getTimezone() on the provider getter.

> It seems most of this interface is readonly, but I am missing a comment as to
> why this is the case. From a novice perspective, I'd assume its possible to
> change certain aspects of the timezone programatically. It would be great if
> you could include some comments how it is possible.
The interface is only meant to reflect a timezone, without offering to change it. Timezone information are to some kind static. If a provider needs to expose foreign timezones, it implements a provider.

> >Index: calendar/base/src/calDateTime.cpp
> >+            NS_ASSERTION(SameCOMIdentity(mTimezone, cal::UTC()), "UTC mismatch!");
> From what I remember in irc backlog, this was in some util header. Was it
> already included?
It's in nsCOMPtr.h

icalperiodtype_from_string(nsPromiseFlatCString(aIcalString).get());
> >+    ip = icalperiodtype_from_string(PromiseFlatCString(aIcalString).get());
> 
> Reading
> http://developer.mozilla.org/en/docs/Migrating_from_Internal_Linkage_to_Frozen_Linkage
> It says PromiseFlatString classes are not around anymore. Is this also the case
> with PromiseFlatCString? Do we need to get rid of this?
Those are obsololete on trunk (typedef'ed to nsString/nsCString) and we should remove them when we finally cut branch development (because it costs us one further copy-construction). But for now, we still need those for branch.

> >Index: calendar/base/src/calTimezone.cpp
> This class looks fairly straightforward. Any reason you didn't implement it in
> js?
It's used by calICSService.cpp.

> >Index: calendar/base/src/calTimezoneService.js
> >+var g_stringBundle = null;
> Do we really need to use a string bundle global? What about calGetString?
I wanted to prevent constructing the same string bundle over and over again. W.r.t. caching, what about my suggestion (see one of my xxx todos) to cache string bundles in calUtils' calGetStringBundle?

> >+function calStringEnumerator(stringArray) {
> >+    this.mIndex = 0;
> >+    this.mStringArray = stringArray;
> >+}
> >+calStringEnumerator.prototype = {
> >+    // nsIUTF8StringEnumerator:
> >+    hasMore: function calStringEnumerator_hasMore() {
> >+        return (this.mIndex < this.mStringArray.length);
> >+    },
> >+    getNext: function calStringEnumerator_getNext() {
> >+        if (!this.hasMore()) {
> >+            throw Components.results.NS_ERROR_UNEXPECTED;
> >+        }
> >+        return this.mStringArray[this.mIndex++];
> >+    }
> >+};
> You could possibly make use of js 1.7 iterators here instead. See bug 418490
> for utils. Otherwise isn't this something we should rather put in calUtils or
> similar?
Unless there are more users for string iterators, I wouldn't want to bloat calUtils further.

> >+    ensureInited: function calTimezoneService_init() {
> To be pedantic, ensureInitialized sounds better :-)
Just for you Philipp, just for you. :-)

> >+            try {
> >+                sqlTzFile.append("timezones.sqlite");
> >+                var dbService = Components.classes["@mozilla.org/storage/service;1"]
> >+                                          .getService(Components.interfaces.mozIStorageService);
> >+                this.mDb = dbService.openDatabase(sqlTzFile);
> >+
> >+                var statement = this.mDb.createStatement("SELECT * FROM tz_data WHERE tzid == :tzid");
> >+                this.mSelectByTzid = Components.classes["@mozilla.org/storage/statement-wrapper;1"]
> >+                                               .createInstance(Components.interfaces.mozIStorageStatementWrapper);
> >+                this.mSelectByTzid.initialize(statement);
> >+
> >+                g_stringBundle = calGetStringBundle(bundleURL);
> So much in a try block? the append and var dbService can probably be safely
> done outside.
In case no calendar-timezones.xpi is installed, the sqlTzFile is undefined. I think we want maximum logging there, so I put everything into the block.

> 
> >+                LOG(msg);
> >+                Components.utils.reportError(msg);
> >+                showError(msg);
> You really want this error to show, eh? ;-)
Yes, I want.

> > // some static timezone helpers, we leak those, but this is ok since the
> > // underlying service leaks those anyway until process termination
> >+# xxx todo discuss: should we better move this file either into
> >+#     	   	    - calendar/locales/en-US/chrome/calendar/
> I prefer ^^
and sipaq, too. done that.

> >+extensions.{d0f7a1d4-2a2f-4b7b-a6a9-aa8059fb4b7e}.name=Timezone Definitions for Mozilla Calendar adlkjadlkj
> For Mozilla Calendar what!?
Oops, testing leftover.

> >+# I've derived this list out timezones.properties
> >+# - replaced '_' with ' ' on value side
> >+# - xxx todo: what about 'St xyz'? Shouldnt we call those 'St. xyz'?
> St. sounds correct to me for displaying.
Yes, think so, too.
Did s/St /St\. /g

Comment 20

[Daniel Boelzle [:dbo]]

(In reply to comment #16)
> Some drive by thoughts on the extension:
> 
> +    <em:id>{d0f7a1d4-2a2f-4b7b-a6a9-aa8059fb4b7e}</em:id>
> +    <em:version>0.1</em:version>
> 
> As we create a new extension we can move from uuid to the recommended and more
> readable [e-mail] format.
Yes, that's one of my "xxx todo" comments. Initially I though I stay in line with lightning.xpi and gdata-provider.xpi, but you're right it's time to use that for new stuff: Changed to [e-mail].

> As we are based on the Olson database I'd like to see the corresponding version
> somewhere, e.g. as part of the extension version (maybe like 0.1.2008b?) This
> would help to tell what version the extension is based on and if an update is
> required.
Yes, makes sense. What's the current number? 2007b?
BTW: The update script stamps the current (update) date into timezones.sqlite.

> I think we also need to specify the required Lightning (also Sunbird via
> extension stub) version to ensure that the timezone extension works with it.
I think it needs to be the other way round: lightning.xpi needs to define a requirement against calendar-timezones.xpi.
There's no reason that the calendar-timezones.xpi (which is only data) needs a requirement against a specific lightning version.

> +        <!-- Thunderbird -->
> +        <em:id>{3550f703-e582-4d05-9a08-453d09bdfdc6}</em:id>
> +        <em:minVersion>2.0a1</em:minVersion>
> +        <em:maxVersion>999999</em:maxVersion>
> 
> At least when hosting on a.m.o this needs to be valid values, e.g. 2.0.0.0 to
> 2.0.0.* that match the supported Lightning version. For example I don't want
> users to install that extension in Thunderbird 3 just to recognize that it
> won't work on Trunk and Lightning 0.9pre is required.
Same as above: calendar-timezones.xpi is only data, not code. In the future, we want to support all older versions of lightning (0.9pre and later) with up-to-date timezone definitions. I currently don't see a compelling reason to distinguish between branch and trunk.
I think it's more important to not make mistakes w.r.t. to db schema, because that must not change in future versions.

Comment 21

[Daniel Boelzle [:dbo]]

Attachment: patch - v3

Next iteration of the patch, working in comments and adds sorting the timezone display names.

What becomes more urging with these changes is bug 413908 since older ics files now show the prefixed form (/mozilla.org/...); quite ugly.

Comment 22

[cmtalbert]

Daniel and Philipp,

I'm super impressed with this patch!!!!  It is very good to see this. The part I love the most is the mechanism for updating the timezone definitions. Brilliant! I'm pulling both a branch and trunk tree to do some testing of all this, and then I will grant my review once every thing checks out.  I just have some little nits from the visual review of the patch:

> +                // xxx todo: l10n
> +                var msg = "No timezones found! Please install calendar-timezones.xpi!\n" + exc;
Go on and localize that into the standard properties file

>+            // libical seems to put an empty line after the inner components of the VTIMEZONE when
>+            // serializing the vzic'ed VTIMEZONEs to some ical.
>+            // This confuses looking at the timezones.sqlite dump-diff; we should fix that xxx todo write bug.
Let's file that follow-on bug

>+    db.executeSimpleSQL("VACUUM");
>+    // xxx todo: any need to further close the db?
I'm pretty sure that the Javascript interfaces will close the Db when the database objects go out of scope and get GC'd.

> +# xxx todo write bug for multi-l10n packaging
Again, a follow-on bug is needed

Trunk is building now.  Will test in the morning.

Comment 23

[cmtalbert]

My first round of testing is completed.

= Testing Upgrade from early versions =
* From 0.5 -> 0.7 -> Latest nightly
** This test did not work.  The alias results are not working. If something should be mapped to some other timezone (for example Pacific/Yap) it gets interpreted as a foreign timezone and is not re-referenced to the alias timezone i.e. Pacific/Truk.
** Also, with the storage calendar we hit a database upgrade error (or so it appears) I can no longer create storage calendars on that profile.  The error message is: "JS frame :: file:///Users/clint/Library/Thunderbird/Profiles/wj2vqof1.old/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calCalendarManager.js :: cmgr_createCalendar :: line 533"

= Testing in place with the nightly =
* Timezone conversions - works great using the in-build defined timezones
* Importing ICS files with our old timezones - I installed these as an ICS resource.  They did NOT get translated into the current (no prefix) timezones.  Instead, they are being treated as foreign timezones, and in the case of aliases (Jan Mayen -> Oslo) they did not work.  It still reports the Jan Mayen timezone.
* Importing ICS file with old timezones into storage - same behavior as above - both timezones are reporting as foreign timezones and are not transmuted into what they should be in the modern timezone definitions.

I'll be happy to file follow on bugs for this stuff, but I want to make sure that this is not covered by other bugs first.

Is the Alias support working yet?  

Is the storage calendar issue I hit really bug 429521?

I'm going to r+ this because the timezones are working, and I think the approach is sound.  I think that it would be best to get this code in and work out these migration issues in follow on bugs.

Great work guys.

Comment 24

[Stefan Sitter [:ssitter]]

(In reply to comment #23)
> Is the storage calendar issue I hit really bug 429521?

Yes, if you were using Trunk test builds. Using MOZILLA_1_8_BRANCH builds it should work.

Comment 25

[Daniel Boelzle [:dbo]]

(In reply to comment #23)
> My first round of testing is completed.
> 
> = Testing Upgrade from early versions =
> * From 0.5 -> 0.7 -> Latest nightly
> ** This test did not work.  The alias results are not working. If something
> should be mapped to some other timezone (for example Pacific/Yap) it gets
> interpreted as a foreign timezone and is not re-referenced to the alias
> timezone i.e. Pacific/Truk.
Are you using an ics file? Then, isn't that the same as bug 413908? There's no auto update when parsing ics, all is treated as is (foreign). I think we will change that and try auto updating any timezone if possible (i.e. those prefixed with /mozilla.org/ and those that match our Olson names or aliased Olson names), leaving the rest as is (foreign). Since we can assume the user has a top-notch calendar-timezones.xpi installed, this is IMO good. Clint, do you see any problems doing so?

> ** Also, with the storage calendar we hit a database upgrade error (or so it
> appears) I can no longer create storage calendars on that profile.  The error
> message is: "JS frame ::
> file:///Users/clint/Library/Thunderbird/Profiles/wj2vqof1.old/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calCalendarManager.js
> :: cmgr_createCalendar :: line 533"
As Stefan mentioned: branch or trunk?

> = Testing in place with the nightly =
> * Timezone conversions - works great using the in-build defined timezones
> * Importing ICS files with our old timezones - I installed these as an ICS
> resource.  They did NOT get translated into the current (no prefix) timezones. 
> Instead, they are being treated as foreign timezones, and in the case of
> aliases (Jan Mayen -> Oslo) they did not work.  It still reports the Jan Mayen
> timezone.
I think it's the same issue as described above, known bug.

> * Importing ICS file with old timezones into storage - same behavior as above -
> both timezones are reporting as foreign timezones and are not transmuted into
> what they should be in the modern timezone definitions.
dto.

> I'll be happy to file follow on bugs for this stuff, but I want to make sure
> that this is not covered by other bugs first.
> 
> Is the Alias support working yet?  
It should work (I will add some test code), but yet I can only imagine one scenario where it's used: Upgrade an old storage.sdb calendar (using a now aliased timezone).

Thanks for testing!

Comment 26

[cmtalbert]

(In reply to comment #25)
> (In reply to comment #23)
> > My first round of testing is completed.
> > 
> > = Testing Upgrade from early versions =
> > * From 0.5 -> 0.7 -> Latest nightly
> > ** This test did not work.  The alias results are not working. If something
> > should be mapped to some other timezone (for example Pacific/Yap) it gets
> > interpreted as a foreign timezone and is not re-referenced to the alias
> > timezone i.e. Pacific/Truk.
> Are you using an ics file? Then, isn't that the same as bug 413908? There's no
> auto update when parsing ics, all is treated as is (foreign). I think we will
> change that and try auto updating any timezone if possible (i.e. those prefixed
> with /mozilla.org/ and those that match our Olson names or aliased Olson
> names), leaving the rest as is (foreign). Since we can assume the user has a
> top-notch calendar-timezones.xpi installed, this is IMO good. Clint, do you see
> any problems doing so?
I don't see any problems in doing so as long as you maintain a list of aliases and adhere to that.  For example we won't find a Pacific/Yap in the Olson database but that should still be moved to Pacific/Truk and have the mozilla.org prefix stripped.

> 
> > ** Also, with the storage calendar we hit a database upgrade error (or so it
> > appears) I can no longer create storage calendars on that profile.  The error
> > message is: "JS frame ::
> > file:///Users/clint/Library/Thunderbird/Profiles/wj2vqof1.old/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calCalendarManager.js
> > :: cmgr_createCalendar :: line 533"
> As Stefan mentioned: branch or trunk?

branch

> 
> > = Testing in place with the nightly =
> > * Timezone conversions - works great using the in-build defined timezones
> > * Importing ICS files with our old timezones - I installed these as an ICS
> > resource.  They did NOT get translated into the current (no prefix) timezones. 
> > Instead, they are being treated as foreign timezones, and in the case of
> > aliases (Jan Mayen -> Oslo) they did not work.  It still reports the Jan Mayen
> > timezone.
> I think it's the same issue as described above, known bug.
Yeah I agree.

> 
> > * Importing ICS file with old timezones into storage - same behavior as above -
> > both timezones are reporting as foreign timezones and are not transmuted into
> > what they should be in the modern timezone definitions.
> dto.
> 
> > I'll be happy to file follow on bugs for this stuff, but I want to make sure
> > that this is not covered by other bugs first.
> > 
> > Is the Alias support working yet?  
> It should work (I will add some test code), but yet I can only imagine one
> scenario where it's used: Upgrade an old storage.sdb calendar (using a now
> aliased timezone).
I tried to test that and I hit the above database error, so it isn't yet clear to me if it works.

Oh, I had one other comment about the review: do we want/need to modify the --enable-extensions line in the mozconfig files to include the timezone extension?

Comment 27

[Daniel Boelzle [:dbo]]

- I'll investigate some more into your findings before checking anything in.
- Since Sunbird and Lightning (in the future) depend on calendar-timezones.xpi, I think there's no need to add it to enable-extensions.

Comment 28

[Daniel Boelzle [:dbo]]

(In reply to comment #23)
> = Testing Upgrade from early versions =
> * From 0.5 -> 0.7 -> Latest nightly
> ** This test did not work.  The alias results are not working. If something
> should be mapped to some other timezone (for example Pacific/Yap) it gets
> interpreted as a foreign timezone and is not re-referenced to the alias
> timezone i.e. Pacific/Truk.
I've tried to reproduce that, upgrading from 0.7 (and BTW added a unit test for Yap->Truk) and it works. If possible, please file a follow-up for that.

> ** Also, with the storage calendar we hit a database upgrade error (or so it
> appears) I can no longer create storage calendars on that profile.  The error
> message is: "JS frame ::
> file:///Users/clint/Library/Thunderbird/Profiles/wj2vqof1.old/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calCalendarManager.js
> :: cmgr_createCalendar :: line 533"
A trivial bug in the patch, fixed that.

> Instead, they are being treated as foreign timezones, and in the case of
> aliases (Jan Mayen -> Oslo) they did not work.  It still reports the Jan Mayen
> timezone.
W.r.t. ics this will be fixed in bug 413908, but w.r.t. storage it works, tested that.

(In reply to comment #26)
> > auto update when parsing ics, all is treated as is (foreign). I think we will
> > change that and try auto updating any timezone if possible (i.e. those prefixed
> > with /mozilla.org/ and those that match our Olson names or aliased Olson
> > names), leaving the rest as is (foreign). Since we can assume the user has a
> > top-notch calendar-timezones.xpi installed, this is IMO good. Clint, do you see
> > any problems doing so?
> I don't see any problems in doing so as long as you maintain a list of aliases
> and adhere to that.  For example we won't find a Pacific/Yap in the Olson
> database but that should still be moved to Pacific/Truk and have the
> mozilla.org prefix stripped.
Yes, we stay with aliases if timezones are purged out of the Olson set. We will fix bug 413908 replacing with current (unaliased) definitions.

(In reply to comment #22)
> > +                // xxx todo: l10n
> > +                var msg = "No timezones found! Please install calendar-timezones.xpi!\n" + exc;
> Go on and localize that into the standard properties file
done.

> >+            // libical seems to put an empty line after the inner components of the VTIMEZONE when
> >+            // serializing the vzic'ed VTIMEZONEs to some ical.
> >+            // This confuses looking at the timezones.sqlite dump-diff; we should fix that xxx todo write bug.
> Let's file that follow-on bug
filed bug 437418

> > +# xxx todo write bug for multi-l10n packaging
> Again, a follow-on bug is needed
filed bug 437441


Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 29

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008060510 Calendar/0.9pre

Sunbird is broken after startup, views and menus don't work. Many errors are visible, e.g.:

Error: No timezones found! Please install calendar-timezones.xpi.
Source file: file:///.../js/calTimezoneService.js Line: 166

Error: window is not defined
Source file: file:///.../js/calUtils.js Line: 1439

Comment 30

[Daniel Boelzle [:dbo]]

Attachment: fixing packages-static

This ought to be the proper fix for Sunbird Windows. I don't have a full Windows build at hand (probably tomorrow morning soonest), so I'd be thankful if you, Stefan, or somebody else could verify the patch on his Windows build.

Comment 31

[Stefan Sitter [:ssitter]]

Comment on attachment 323930 [details] [diff] [review]
fixing packages-static

I'll try to test this later once I'm back to my development system.

From a first glance I think the lines must be moved upwards from the [talkback] section to the [calendar] section.

Comment 32

[Daniel Boelzle [:dbo]]

done full Windows static installer build => fixes the problem

We could even consider to create an own section for calendar-timezones...

Comment 33

[Stefan Sitter [:ssitter]]

Comment on attachment 323930 [details] [diff] [review]
fixing packages-static

r=ssitter if you move it to [calendar]. I just want to be sure it's installed if users deselect Talkback during installation.

This fixes the startup error (tested with Trunk) but the extension is disabled due to the following lines in install.rdf:

 <em:minVersion>0.9pre</em:minVersion>
 <em:maxVersion>0.6a1</em:maxVersion>

Comment 34

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

(In reply to comment #33)
> This fixes the startup error (tested with Trunk) but the extension is disabled
> due to the following lines in install.rdf:
> 
>  <em:minVersion>0.9pre</em:minVersion>
>  <em:maxVersion>0.6a1</em:maxVersion>
I think this needs further thinking, because the updates will be performed on trunk.