254893 - Daylight savings applied incorrectly

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Fernando Schapachnik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616

Loading a remote calendar, all events prior to Nov, 1st are 1 hour late. Seems
that Calendar is applying daylight savings, although they are not used here
(Argentina).

Other products (KOrganizer or Apple) handle this Ok.

Reproducible: Always
Steps to Reproduce:
1. Load the calendar given in the URL.
2. Look how it displays.
3. Look at the .ics (prior and post Nov, 1st).

Actual Results:  
Hours prior to Nov, 1st are wrong.

Expected Results:  
All hours should be OK.

I think the description as well as the scenario that triggers it are pretty
straight forward, but if you need anything else, please let me know.

Thanks!

Comment 1

[Joey Minta]

Reporter,
    Timezone support is dramatically improved in nightly builds of Sunbird and Lightning (and Lightning 0.1).  Can you please re-test in one of those builds and report whether or not the situation has changed?

Comment 2

[Daniel Boelzle [:dbo]]

taking the bug.

The problem is in calICSService.cpp: All datetime attributes gathered from an ical component are converted from their actual timezone to UTC. The recurring series is thus calculated based on an _UTC_ startDate (maps to item's property "DTSTART" maps to ical component's startTime attribute maps to libical ICAL_DTSTART_PROPERTY). The individual occurring date times are then converted to the present timezone of the application.

The series calculation has to be performed using the original timezone information and minding its specifics concerning daylight saving. Solving this issue, we have to deal with the unloved ical timezone handling, and calendar local tz identifiers.
The current calDateTime stores just a timezone string assuming that the timezone info can be gathered from the ICS service (currently only the Olson ones generated from libical, prefixed with /mozilla.org/...). In principle, calendars can come up with own timezones, unknown to mankind.
So the correct attempt would be to extend calDateTime having access to its full timezone definition.

Assuming that a lot of timezone definitions can be shared, the ICS service can cache all timezones in play (as it already does for the tzdata.c Olson ones). This approach assumes that no two VTIMEZONE definitions with different semantics share the same TZID, which would be weird. So, I have extended calIICSService, adding/implementing AddTimezone(). As ical files come up without longitude/latitude information, this information most often gets lost.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: timezone fixes

calICSService.cpp:
- AddTimezone()
- Clone(), so one can easily plug together a timezone VCALENDAR
  (which must contain only one timezone)
- demacrofying datetime attribute methods
- fixing datetime attribute setter:
  missing ClearAllProperties() before adding property
- fixing datetime attribute getter:
  publishing upcoming timezones to ics service

calDatetime.cpp:
- SubtractDate(): icaltime_subtract() does not mind timezones
- FromIcalTime(): cleanup

lightning-preferences.js:
- minor fix for pref page using ics service now
  (instead of creating a new instance)

Two things come up my mind:
- The cached timezone calendars are not unloaded until process end. To solve this, calDateTime needs to (hard) reference its timezone and the ICS service's cache needs to hold them weakly.
- All added timezones affect the timezoneIds enumerator, i.e. the added ones appear in the preferences "MyTimeZone" list box. If this is not wanted, e.g. we just want the static ones (prefixed /mozilla.org/..), we have to filter them out.

@mvl: It would be great if you review this.

Comment 4

[Daniel Boelzle [:dbo]]

Attachment: setting timezone properly for time calculation

The multi-day-view time calculation needs to take care using the same timezone as the view (newStart, newEnd may come with a different one).

Comment 5

[Joey Minta]

Comment on attachment 217299 [details] [diff] [review]
setting timezone properly for time calculation

I'm addressing this issue in bug 332772 in a somewhat different manner.  Particularly, to avoid the dataloss issues associated with bug 304486, the solution here probably isn't the ideal path to take.  I'd be interested in feedback in bug 332772 though.

Comment 6

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

Comment on attachment 217285 [details] [diff] [review]
timezone fixes

>@@ -222,10 +228,24 @@ interface calIICSService : nsISupports
>+    calIIcalComponent addTimezone(in calIIcalComponent tzCal,
>+                                  in AUTF8String tzLatitude,
>+                                  in AUTF8String tzLongitude);

imo, there's no need for the 'tz' prefix, the method name already implies that we are dealing with timezones.

>+++ mozilla/calendar/base/src/calICSService.cpp	2006-04-05 12:00:35.087520000 +0200
>-NS_IMETHODIMP
>-calIcalComponent::AddTimezoneReference(calIIcalComponent *aTimezone)
>+static nsresult unwrapVTimeZoneComponent(
>+    calIIcalComponent *tzcal, nsCOMPtr<calIIcalComponent> & rtz)

Why is this function static? That's not very common in mozilla code.
Using nsCOMPtr references is very uncommon in mozilla code. Any reason why you choose to use it? More common would be calIIcalComponent** aResult.
And can you try to use the 'aFoo' convention for method parameters? 

That's about as far as I got... :(

Comment 7

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
> >+    calIIcalComponent addTimezone(in calIIcalComponent tzCal,
> >+                                  in AUTF8String tzLatitude,
> >+                                  in AUTF8String tzLongitude);
> 
> imo, there's no need for the 'tz' prefix, the method name already implies that
> we are dealing with timezones.

ok.

> >+static nsresult unwrapVTimeZoneComponent(
> >+    calIIcalComponent *tzcal, nsCOMPtr<calIIcalComponent> & rtz)
> 
> Why is this function static? That's not very common in mozilla code.

Well, I would have preferred putting the function into an anonymous namespace, but namespaces are forbidden (as stated in http://www.mozilla.org/hacking/portable-cpp.html). The function is private to this implementation, so static seemed to be the best approach to me. You think of a better approach?

> Using nsCOMPtr references is very uncommon in mozilla code. Any reason why you
> choose to use it? More common would be calIIcalComponent** aResult.

Use of nsCOMPtr is recommended in the very same C++ coding guidelines I have mentioned. In http://www.mozilla.org/projects/xpcom/nsCOMPtr.html section "nsCOMPtrs in function signatures" it is recommended to pass nsCOMPtr by C++ reference for in/out interfaces in contrast to plain iface**, because this puts the burden on the callee function to know whether to release the passed interface pointer. It is more failsave.

> And can you try to use the 'aFoo' convention for method parameters? 

Well, most code in calICSService.cpp doesn't follow this rule, so IMO this rather uglifies the file changing the naming.

But, what about the overall approach? IMO we should postpone discussing about coding styles etc. after we have agreed whether this patch fixes the problem the right way.

Thanks for reviewing!

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: timezone fixes

adopted recent changes in calICSService.cpp

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: timezone fixes

further fix in datetime getter

Comment 10

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

Comment on attachment 219052 [details] [diff] [review]
timezone fixes

> NS_IMETHODIMP
> calICSService::GetTimezone(const nsACString& tzid,
>                            calIIcalComponent **_retval)

>-    const ical_timezone_data_struct *tzdata = get_timezone_data_struct_for_tzid(nsPromiseFlatCString(tzid).get());

Without this call, how can you get unused timezones? I think we need that functionality, for example when starting for the first time, and no events are defined yet. But you do want to be able to get times in the current timezone

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> (From update of attachment 219052 [details] [diff] [review] [edit])
> > NS_IMETHODIMP
> > calICSService::GetTimezone(const nsACString& tzid,
> >                            calIIcalComponent **_retval)
> 
> >-    const ical_timezone_data_struct *tzdata = get_timezone_data_struct_for_tzid(nsPromiseFlatCString(tzid).get());
> 
> Without this call, how can you get unused timezones? I think we need that
> functionality, for example when starting for the first time, and no events are
> defined yet. But you do want to be able to get times in the current timezone
> 

get_timezone_data_struct_for_tzid() has been merged into getTimezoneEntry(). getTimezoneEntry() first looks up the cached timezones, then in the build-in timezone array from tzdata.c (as get_timezone_data_struct_for_tzid() did before).

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #2)
> 
> Assuming that a lot of timezone definitions can be shared, the ICS service can
> cache all timezones in play (as it already does for the tzdata.c Olson ones).
> This approach assumes that no two VTIMEZONE definitions with different
> semantics share the same TZID, which would be weird. 

This sounds like a good design to me.  That said, I think we can count on seeing instances of the weirdness that you describe above in the wild, so we probably need to at least define exactly how we're going to behave when we do.

> So, I have extended calIICSService, adding/implementing AddTimezone().
> As ical files come up without longitude/latitude information, this
> information most often gets lost.

It's not clear to me why latitude and longitude implementation belong in this API.  Can you explain?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3)

> - Clone(), so one can easily plug together a timezone VCALENDAR
>   (which must contain only one timezone)

We're going to want this method for bug 314334 anyway, so it's good to have.  I don't actually see where it's being used in the most recent patch in this bug, though...

> - demacrofying datetime attribute methods

Excellent; editing these functions was always such a hassle.

> Two things come up my mind:
> - The cached timezone calendars are not unloaded until process end. To solve
> this, calDateTime needs to (hard) reference its timezone and the ICS service's
> cache needs to hold them weakly.

Sounds like a good solution to me.

> - All added timezones affect the timezoneIds enumerator, i.e. the added ones
> appear in the preferences "MyTimeZone" list box. If this is not wanted, e.g.
> we just want the static ones (prefixed /mozilla.org/..), we have to filter 
> them out.

Allowing someone to set their default to a non-native time zone seems like it's asking for trouble, especially if they disappear from the cache once all strong refs are gone.  

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #12)
> > So, I have extended calIICSService, adding/implementing AddTimezone().
> > As ical files come up without longitude/latitude information, this
> > information most often gets lost.
> 
> It's not clear to me why latitude and longitude implementation belong in this
> API.  Can you explain?

calIICSService supports longitude/latitude information for a given TZID, maybe for timezone visualization. Because a VTIMEZONE inherently doesn't have information for this, the user needs a way to pass it in. Alternatively we could specify setters.

(In reply to comment #13)
> > - The cached timezone calendars are not unloaded until process end. To solve
> > this, calDateTime needs to (hard) reference its timezone and the ICS service's
> > cache needs to hold them weakly.
> 
> Sounds like a good solution to me.

The mimic of addTimezone() would IMO somehow be strange then, because an "added" one may vanish... better: publishTimezone()?
Can you imagine any performance penalties with weak references here?
IMO we should separate timezone caching (and using timezone references in calDatetime instead of TZIDs) into another issue/patch. The present TZID-based API of calIDatetime would be fragile, IMO we need to revise it, too:
-calIDatetime::getInTimezone(tzid)
+calIDatetime::getInTimezone(tzComponent)
-calIDatetime::setTimeInTimezone(time, tzid)
-calIDatetime::setTimeInTimezone(time, tzComponent)

> > - All added timezones affect the timezoneIds enumerator, i.e. the added ones
> > appear in the preferences "MyTimeZone" list box. If this is not wanted, e.g.
> > we just want the static ones (prefixed /mozilla.org/..), we have to filter 
> > them out.
> 
> Allowing someone to set their default to a non-native time zone seems like it's
> asking for trouble, especially if they disappear from the cache once all strong
> refs are gone.  

I would vote for filtering them out in the prefs listbox, too.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #14)
> (In reply to comment #12)
> > > So, I have extended calIICSService, adding/implementing AddTimezone().
> > > As ical files come up without longitude/latitude information, this
> > > information most often gets lost.
> > 
> > It's not clear to me why latitude and longitude implementation belong in this
> > API.  Can you explain?
> 
> calIICSService supports longitude/latitude information for a given TZID, maybe
> for timezone visualization. Because a VTIMEZONE inherently doesn't have
> information for this, the user needs a way to pass it in. Alternatively we
> could specify setters.

Yeah, I don't really understand why they're in the service at all, since they're not called by any code.  Vlad, can you fill us in on the intent of having this information available?  It's not clear to me what exactly it would mean and how it could be used in a non-error-prone way.

> (In reply to comment #13)
> > > - The cached timezone calendars are not unloaded until process end. To solve
> > > this, calDateTime needs to (hard) reference its timezone and the ICS service's
> > > cache needs to hold them weakly.
> > 
> > Sounds like a good solution to me.
> 
> The mimic of addTimezone() would IMO somehow be strange then, because an
> "added" one may vanish... better: publishTimezone()?
> Can you imagine any performance penalties with weak references here?
> IMO we should separate timezone caching (and using timezone references in
> calDatetime instead of TZIDs) into another issue/patch.

Yeah, I think you're right that separating out caching from local-knowledge is probably the right thing.  After I wrote that comment, it occurred to me that unused timezones would be evicted from the cache, making it not useful for populating the timezone chooser at all.  That said, I wouldn't have a problem with leaving it more-or-less as is for now, and filing another bug on handling the memory-footprint / cache-eviction issues later.

> The present TZID-based API of calIDatetime would be fragile, IMO we need to 
> revise it, too:
> -calIDatetime::getInTimezone(tzid)
> +calIDatetime::getInTimezone(tzComponent)
> -calIDatetime::setTimeInTimezone(time, tzid)
> -calIDatetime::setTimeInTimezone(time, tzComponent)

I think a lot of this depends on how we handle tzid / component mismatches.  If I remember correctly, I think the current deal is that libical handles this for us at least some of the time by comparing the full component, and if it's different from an existing component with the same name, does a forcible tzid rename. http://www.calconnect.org/publications/icalendartimezoneproblemsandrecommendationsv1.0.pdf
has a bit of discussion about this, but not a lot.

I'm almost inclined to suggest just forcibly mapping incoming TZIDs into our own local, Olsen-based represtation, largely because the complications mentioned in 3.2.2.2 of that document sound really hard to solve in any reasonable way.

Comment 16

[Joey Minta]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > > So, I have extended calIICSService, adding/implementing AddTimezone().
> > > > As ical files come up without longitude/latitude information, this
> > > > information most often gets lost.
> > > 
> > > It's not clear to me why latitude and longitude implementation belong in this
> > > API.  Can you explain?
> > 
> > calIICSService supports longitude/latitude information for a given TZID, maybe
> > for timezone visualization. Because a VTIMEZONE inherently doesn't have
> > information for this, the user needs a way to pass it in. Alternatively we
> > could specify setters.
> 
> Yeah, I don't really understand why they're in the service at all, since
> they're not called by any code.  Vlad, can you fill us in on the intent of
> having this information available?  It's not clear to me what exactly it would
> mean and how it could be used in a non-error-prone way.
> 
I called them in the patch in Bug 302253.  As mentioned, this allows for nicer visual presentation.

Comment 17

[Stefan Sitter [:ssitter]]

The original problem reported in this bug should be solved with the check in for Bug 333717 already. What will be solved with the patch in this bug?

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> The original problem reported in this bug should be solved with the check in
> for Bug 333717 already. What will be solved with the patch in this bug?

Formerly all datetimes with TZID have been converted to UTC, which was wrong: unfolding a recurring event requires the original DTSTART timezone for daylight-saving. Both this patch and the patch from 333717 have fixed that.
Moreover, this patch fixes the timezone problem in a more general way, that foreign timezone definitions are imported at runtime. Currently only the built-in Olson ones (/mozilla.org/...) can be processed. All calendar data coming with other TZIDs will still fail, e.g. if you try to subscribe to a google calendar ics. 

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #16)
> > Yeah, I don't really understand why they're in the service at all, since
> > they're not called by any code.  Vlad, can you fill us in on the intent of
> > having this information available?  It's not clear to me what exactly it would
> > mean and how it could be used in a non-error-prone way.
> > 
> I called them in the patch in Bug 302253.  As mentioned, this allows for nicer
> visual presentation.

Ah, I get it!  They are in relation to the city that the TZ is named after, not the overall TZ.  Ideally, we'd make the documentation a little more explicit about that.  Presumably any timezones we get off the wire won't have this data at this point, so assuming this is the right thing to do, we may want to propose some sort of extension (either via ietf-calsify, calconnect, or elsewhere) to VTIMEZONEs for this.

In order to evaluate this patch, I think we need to first answer two user-level questions in light of the points brought up in 3.2.2.2 of the doc above:

What do we do about non-local timezones as far as both the UI and the data is concerned?

How do we handle incoming timezones that are identically named but have a different definition, also in terms of both UI and data?

In some ideal world we'd preserve data semantics as precisely as we can, but representing this in a sane way in the UI strikes me as pretty difficult.  In particular: how do we represent to the user that there are now two slightly different timezones that claim to be for the same area?  Is there some way to make them deal with this, other than forcibly mapping all non-local and conflicting timezones into our local database?

Comment 20

[Daniel Boelzle [:dbo]]

(In reply to comment #19)
> How do we handle incoming timezones that are identically named but have a
> different definition, also in terms of both UI and data?
> 
> In some ideal world we'd preserve data semantics as precisely as we can, but
> representing this in a sane way in the UI strikes me as pretty difficult.  In
> particular: how do we represent to the user that there are now two slightly
> different timezones that claim to be for the same area?  Is there some way to
> make them deal with this, other than forcibly mapping all non-local and
> conflicting timezones into our local database?

IMO we have to keep the round-trip in mind: Even if we could merge similar timezone definitions, we may need to use the very same TZIDs and definitions when publishing calendar data to calendar servers. Additionally, users may need to be limited to timezones their server supports (most possibly a subset of our Olson base). This leads me to the conclusion that we need to support non-local timezones at UI level. But I can imagine identifying similar timezone records and presenting aliases.

Comment 21

[cmtalbert]

At some point when considering this bug, you may need to perform a mapping between our Olsen database and other timezone databases. Obviously one of those databases is the Windows Timezone database, which is mapped in this link: http://www.unicode.org/cldr/data/docs/design/formatting/zone_log.html

Additionally, there is a CDO timezone database used when by Exchange Server and Outlook Web Interface. I found a listing of these values in MSDN. You would think there would be more values, but I cannot find a more complete list. I have attached it as cdo_zones.txt

Comment 22

[cmtalbert]

Attachment: Listing of CDO Timezones

Comment 23

[Daniel Boelzle [:dbo]]

Attachment: timezone fixes

revised patch to apply again

Comment 24

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

Comment on attachment 225580 [details] [diff] [review]
timezone fixes

>+static nsresult unwrapVTimeZoneComponent(
>+    calIIcalComponent *tzcal, nsCOMPtr<calIIcalComponent> & rtz)

That comptr usage is not right. You should not use nscomptr in parameters. It shoul dbe something like:

static nsresult unwrapVTimeZoneComponent(
         calIIcalComponent *aTimezone, calIIcalComponent **rtz)

Also, parameters should start with 'a', like aTimezone. and 'rtz' doesn't mean anything to me. Could you try to name it more meaningful?

> 
>+static nsresult unwrapVTimeZoneComponentWithId(
>+    calIIcalComponent *tzcal,
>+    nsACString & rtzid, nsCOMPtr<calIIcalComponent> & rtz)

Same about this nscomptr.

Comment 25

[Daniel Boelzle [:dbo]]

(In reply to comment #24)
> That comptr usage is not right. You should not use nscomptr in parameters. It

We already had this, I refer to comment #7 here.

> Also, parameters should start with 'a', like aTimezone. and 'rtz' doesn't mean
> anything to me. Could you try to name it more meaningful?

I commented on this in #7, too. IMO it is worse to change the naming style as most code in calICSService.cpp doesn't follow the 'aFoo' convention. rtz should read 'returned timezone'.

Comment 26

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

(In reply to comment #25)
> (In reply to comment #24)
> > That comptr usage is not right. You should not use nscomptr in parameters. It
> We already had this, I refer to comment #7 here.

Sorry, i missed that, and i didn't know that passing nscomptr actually worked. nevermind me then.
> 
> > Also, parameters should start with 'a', like aTimezone. and 'rtz' doesn't mean
> > anything to me. Could you try to name it more meaningful?
> 
> I commented on this in #7, too. IMO it is worse to change the naming style as
> most code in calICSService.cpp doesn't follow the 'aFoo' convention. rtz should
> read 'returned timezone'.

I always disliked the naming convention in that file :)

Comment 27

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

Comment on attachment 225580 [details] [diff] [review]
timezone fixes

This patch looks really good. (didn't test it yet).
I just have a few quick questions:

>+    /**
>+     * Adds and takes over ownership of a timezone record.
>+     * Record will be ignored if there is already an existing one with the
>+     * same id.
>+     * 
>+     * @param tzCal VCALENDAR with a VTIMEZONE child
>+     * @param tzLatitude timzeone latitude, empty string if unknown
>+     * @param tzLongitude timzeone longitude, empty string if unknown
>+     * @return present VCALENDAR with a VTIMEZONE child

It's not clear to me what the return parameter does. How does it differ from the inparam tzCal? Can you add a line explaining that?


>+nsresult calIcalComponent::GetDateTimeAttribute(
>+    icalproperty_kind kind, calIDateTime ** dtp)
...
>+        nsCOMPtr<calIIcalComponent> tzcal;
...
>+            // publish to ics service:
>+            nsCOMPtr<calIIcalComponent> const tzcal_(tzcal);
What's the use of this line? Why can't you just use |tzcal|?

>+TimezoneEntry const* calICSService::getTimezoneEntry(nsACString const& tzid)
> {
>-    for (int i = 0; ical_timezone_data[i].tzid != NULL; i++) {
>-        if (strcmp(tzid, ical_timezone_data[i].tzid) == 0)
>-            return &ical_timezone_data[i];
>+    TimezoneEntry * pEntry = 0;
>+    if ((!mTzHash.Get(tzid, &pEntry) || pEntry == nsnull) &&
>+        StringBeginsWith(tzid,
>+                         nsDependentCString(STRLEN_ARGS(MOZCAL_TZID_PREFIX)))) {

Please move the part that actually does something (hash.get) to a seperate line, with a dedicated check or something. It just hard to read that you check the hash when written like it is now.

Comment 28

[Daniel Boelzle [:dbo]]

(In reply to comment #27)
> >+     * @param tzCal VCALENDAR with a VTIMEZONE child
> >+     * @param tzLatitude timzeone latitude, empty string if unknown
> >+     * @param tzLongitude timzeone longitude, empty string if unknown
> >+     * @return present VCALENDAR with a VTIMEZONE child
> 
> It's not clear to me what the return parameter does. How does it differ from
> the inparam tzCal? Can you add a line explaining that?

In most cases, it does not differ from the incoming tzCal, but the (few) cases where another thread concurrently adds the same TZID, you will get that one (i.e. the actually registered one).
When caching/holding the registered instance weakly comes into play (which is currently not implemented), it may become important that all client code uses the same registered instances (although two instances with same TZID ought to have same semantics). Getting back the actually added instance is often more convenient and less to code; same reason why e.g. STL's associative container API provides an insert() returning an iterator to the actually inserted element.

> >+nsresult calIcalComponent::GetDateTimeAttribute(
> >+    icalproperty_kind kind, calIDateTime ** dtp)
> ...
> >+        nsCOMPtr<calIIcalComponent> tzcal;
> ...
> >+            // publish to ics service:
> >+            nsCOMPtr<calIIcalComponent> const tzcal_(tzcal);
> What's the use of this line? Why can't you just use |tzcal|?

getter_AddRefs(tzcal) clears tzcal so I would loose the ref, thus I need to make a const tzcal_ to assure the instance is alive for the call.

> >+TimezoneEntry const* calICSService::getTimezoneEntry(nsACString const& tzid)
> > {
> >-    for (int i = 0; ical_timezone_data[i].tzid != NULL; i++) {
> >-        if (strcmp(tzid, ical_timezone_data[i].tzid) == 0)
> >-            return &ical_timezone_data[i];
> >+    TimezoneEntry * pEntry = 0;
> >+    if ((!mTzHash.Get(tzid, &pEntry) || pEntry == nsnull) &&
> >+        StringBeginsWith(tzid,
> >+                         nsDependentCString(STRLEN_ARGS(MOZCAL_TZID_PREFIX)))) {
> 
> Please move the part that actually does something (hash.get) to a seperate
> line, with a dedicated check or something. It just hard to read that you check
> the hash when written like it is now.
> 

This would mean another variable, but I am ok with that style.

Any more objections before I rework the patch?

Comment 29

[Daniel Boelzle [:dbo]]

Attachment: show only calendar's built-in timezones in Lightning's prefs

Patch for Lightning's prefs page: showing only the built-in ones from tzdata.c.

Comment 30

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

> Any more objections before I rework the patch?

I don't have more comments on the guts of the patch. But i want to ask you to double-check the code style. There are a bunch of places where you is |if (! foo|,  you could drop the space after the !.

Comment 31

[Daniel Boelzle [:dbo]]

Attachment: timezone fixes

Thanks for reviewing!

Comment 32

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

Comment on attachment 227940 [details] [diff] [review]
timezone fixes

r=mvl

Comment 33

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

Comment on attachment 227053 [details] [diff] [review]
show only calendar's built-in timezones in Lightning's prefs

r=mvl

Comment 34

[Daniel Boelzle [:dbo]]

checked in patch 227940 and 227053

Comment 35

[Joey Minta]

Comment on attachment 227053 [details] [diff] [review]
show only calendar's built-in timezones in Lightning's prefs

Don't we need a Sunbird version of this patch for this bug to be finished?

Comment 36

[Daniel Boelzle [:dbo]]

(In reply to comment #35)
> (From update of attachment 227053 [details] [diff] [review] [edit])
> Don't we need a Sunbird version of this patch for this bug to be finished?

No, Sunbird currently uses static timezones.xul while Lightning populates the list of supported timezones using calIICSService::timezoneIds.