Closed Bug 400950 Opened 12 years ago Closed 12 years ago

Change calDatetime to reference its timezone definition

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(6 files, 5 obsolete files)

Follow-up from bug 314339 comment #68:
calDatetime currently refers to its timezone by name which is
- leaking timezone definitions (keeping timezone definitions always resolvable in calICSService over the process lifetime).
- hindering any attempt to have concurrent timezone definitions sharing the same TZID, because there could only be one definition managed by calICSService per TZID.
The proposal is that calDatetime object should keep a reference to their definition, thus a timezone definition could be freed once the last reference on it has vanished.
Moreover a provider should be able to build up items (and datetime objects) with its own foreign timezone definitions.
Blocks: 314339
Assignee: nobody → daniel.boelzle
Flags: wanted-calendar0.8+
Status: NEW → ASSIGNED
Attached patch work in progress (obsolete) — Splinter Review
Attached patch work in progress (obsolete) — Splinter Review
Attachment #290022 - Attachment is obsolete: true
Attached patch work in progress (obsolete) — Splinter Review
Attachment #290398 - Attachment is obsolete: true
Attached patch full patchSplinter Review
Since this patch touches a lot of code, I expect regressions and would appreciate if more people could test it and report bugs.

quick walk thru:

IDL:
- new calITimezone, calITimezoneProvider, calITimezoneService
- changes calIDateTime to use calITimezone
- changes calIICSService: moves over timezone relevant stuff to calITimezoneProvider
- adds additional calITimezoneProvider parameter for ICS parsing, so timezones could be resolved even though they don't appear in the parsed VCALENDAR

code:
- revises calDatetime, calICSService, calRecurrenceRule etc. + code improvements
- improves calAttributeHelpers: + CAL_ATTR_SET_PRE/POST
- additional calUtils.h helpers: class cal::XpcomBase, sameXpcomObject, cal::UTC() etc.
- additional calUtils.js helpers: getTimezoneService(), UTC(), floating(), calendarDefaultTimezone() now returns a calITimezone
- implements calTimezone, calTimezoneService; will be fed by timezone.sdb in the future
- fixes switching the timezone via freebusy/attendee pane
- *hopefully changes all client code*
Attachment #290441 - Attachment is obsolete: true
Attachment #290734 - Flags: review?(ctalbert)
Attachment #290735 - Flags: review?(philipp)
Attachment #290737 - Flags: review?(michael.buettner)
Comment on attachment 290737 [details] [diff] [review]
views, event dialog

r=mickey.
Attachment #290737 - Flags: review?(michael.buettner) → review+
Attached file Comments on the Core Patch (obsolete) —
These are my review comments on the core patch.  Search for >> in the left margin to find my comments.  Some of it is stylistic.  There are a couple of larger concerns in the comments though.  

One is: should we have the ability to add a timezone to our timezone service/provider?  Or will we merely ensure that the providers will take charge of storing foreign timezone definitions.  If the later, then how does that timezone definition get found?  Does that mean that any provider that wants to store foreign timezone definitions also needs to implement the calITimezoneProvider interface?

Two: There is one part in the code, where we assume there is a containing component with a timezone definition. I think we need to make lots and lots of noise (error console, NSERROR_something) if we find that assumption invalidated.

Three: There is another part where it looked like we might have the opportunity to create timezone refrences on an ical Component *without* a corresponding timezone entry in the top level component.  I want to be sure that this is not done.

Both the exact spots for Two and Three are noted in the comments.

All in all, a very nice patch, great job!  I'll go back through and re-review once I get your updates and feedback on my comments.  Until then I'm holding my review.
(In reply to comment #9)
> One is: should we have the ability to add a timezone to our timezone
> service/provider?  Or will we merely ensure that the providers will take charge
> of storing foreign timezone definitions.  If the later, then how does that
> timezone definition get found?  Does that mean that any provider that wants to
> store foreign timezone definitions also needs to implement the
> calITimezoneProvider interface?
Yes, a calITimezone has a provider attribute stating to which provider the timezone belongs resp. is scoped. This makes it easy to check e.g. whether a timezone is "foreign" or is from the Olson timezone service:

if (!compareObjects(tz.provider, getTimezoneService())) { // is foreign

Beware that the following is *wrong*, because TZIDs are not unique any longer:

if (!getTimezoneService().getTimezone(tz.tzid)) { // is foreign

So I think calStorageCalendar needs to implement calItimezoneProvider, like WCAP already does (see full patch). On reading items (getItems) and building up the datetime objects, it need to cover its own (foreign) ones. More specifically, I can imagine newDateTime becomes a function of calStorageCalendar and does that job.

Two and Three covered in the upcoming answeres attachment.
> All in all, a very nice patch, great job!  I'll go back through and re-review
> once I get your updates and feedback on my comments.  Until then I'm holding my
> review.
Thanks Clint!
Attached file Comments on the Core Patch -- answeres (obsolete) —
look for lines starting with "<<".
again, with better LF
Attachment #290869 - Attachment is obsolete: true
Comment on attachment 290870 [details]
Comments on the Core Patch -- answeres

>Index: calendar/base/content/calendar-dnd-listener.js
>===================================================================
>-                parser.parseString(data);
>+                parser.parseString(data, null);
>
>>>Is this going to work if we are dragging a foriegn timezone event?  Why would
>the encapsulating VCALENDAR not include the timezone of the event...
>Unless this ICS is just a VEVENT only and is relying on the fact that we have
>"magically" parsed the timezones already.  In which case, this will break if the zone
>refers to an as-yet-unparsed foreign timezone.  QUESTION: How does this handle 
>(or how should this handle) foreign zones?  I.e. I drag an ICS file from my
>third party email into Sunbird...or is there no real way to get past this with
>valid ICS?
><<I don't see a solution, because we are lacking that VTIMEZONE data.
Yeah, I think we're going to be ok here.  I can open an ICS file with a foreign timezone and it is parsed ok, so perhaps this drag case will also be ok since the open case also doesn't use a timezoneProvider interface (I think it just grabs the timezone from the VCALENDAR definition for the timezone.
>Index: calendar/base/public/calIICSService.idl
>===================================================================
>     void addSubcomponent(in calIIcalComponent comp);
>-    void removeSubcomponent(in calIIcalComponent comp);
>+// xxx todo: discuss, still incorrect w.r.t. referenced timezones
>+//           no client code existing!
>+//     void removeSubcomponent(in calIIcalComponent comp);
>>>I don't understand why this code was commented out, and I don't understand the comment.
>Please explain.
><<There are already two flaw in the existing code:
>  - If you add a component/property that refers to timezones,
>    those timezones will get added to the parent component.
>    If you remove that component/property thereafter, the timezones won't get purged out,
>    because other added components/properties might need them.
>Since there isn't a single use case right (because we always build up fresh VCALENDAR
>components for serialization), I dropped this function until we have a use case.
OK Sounds good.
>+    /// the timezone provider this timezone belongs to
>+    readonly attribute calITimezoneProvider provider;
>>>Please use the standard /**.*.*.**/ style comments
><<ok. BTW: Is the /// comment not supported by mozilla's doc tools?
I'm not sure if //// is supported or not.  But we use the /** * **/ everywhere else, so it makes sense to keep doing that, INMHO
>+    /**
>+     * For debugging purposes.
>+     *
>+     * @return "UTC", "floating" or component's ical representation
>+     */
>+    AUTF8String toString();
>>>What does this return if you call it with a "real" timezone?  Should we put a
>#ifdef DEBUG around this?  Or should we just leave it in like this?  I tend to think
>that if this will be beneficial to future provider writers then we should leave
>it in without an #ifdef DEBUG around it.  That way they do not have to compile
>C++ in order to debug a JS based provider.  How do you envision this being used?
><<
>For real ones, it returns the ical string of the VTIMEZONE component.
>This is simply for debugging purposes in venkman and turned out to be very useful for me.
>I don't see a reason why we shouldn't keep it.
Let's keep it!

>+    calITimezone getTimezone(in AUTF8String tzid);
>
>>>What about addTimezone as an interface?  Particularly with the timezone database, we
>might want to add a timezone definition to our listing of foreign timezones?  The way
>I understand this, the provider will implement this interface to "resolve" their
>timezones.  Wouldn't a provider need to add the provider specific timezones to this
>interface's list?
><<
>No, I don't think so. This interface has two purposes:
> - resolve timezones referenced in data portions, e.g. when parsing single VEVENTs
> - resolve "global" Olson timezones (implemented by timezone service only)
>We shouldn't pollute the timezone (reference Olson) database with foreign ones.
>A problem we already discussed: we may have clashing TZIDs and we don't know when
>to get rid of a VTIMEZONE stored there.
>My vision is that any calendar source/sink (i.e. the providers) need to keep track
>of their foreign timezones, i.e. the storage provider will save foreign timezones
>and return items that have datatimes with foreign timezones. And after this change,
>a privider could actually do so, because the timezone information need no longer
>be hosted in ICS service, but could be served from anywhere.
I agree.  I always get hung up about this because it was in one of the various previous designs, and I can't get it out of my head.

>Index: calendar/base/src/calDateTime.cpp
>===================================================================
>+#include "ical.h"
>>>Nit: I don't see any need to remove this indenting
><<you see a good reason for that indentation? #include is a preprocessor command.
I think with indenting it is more readable.  But, I also concede your point. You don't normally indent preprocessor directives.  Either way.

>+#define CAL_ATTR_SET_POST Normalize()
>>>Ah, I see why you made the above changes in calAttributeHelpers now.  However,
>doesn't calICSService also use the calAttributeHelpers?  How does that work?
><<no, it doesn't
Yeah, it does something similar but different.  Cool.
>-
>-calDateTime::calDateTime(const calDateTime& cdt)
>-    : mImmutable(PR_FALSE),
>-      mIsValid(cdt.mIsValid),
>-      mNativeTime(cdt.mNativeTime),
>-      mYear(cdt.mYear),
>-      mMonth(cdt.mMonth),
>-      mDay(cdt.mDay),
>-      mHour(cdt.mHour),
>-      mMinute(cdt.mMinute),
>-      mSecond(cdt.mSecond),
>-      mWeekday(cdt.mWeekday),
>-      mYearday(cdt.mYearday),
>-      mIsDate(cdt.mIsDate),
>-      mTimezone(cdt.mTimezone)
>>>Why did you remove the copy constructor?
><<
>Native XPCOM objects have no real copy semantics in the strict sense of C++.
>You always have to reset the ref-count to 0. We should use Clone() instead which has a clear semantic.
Sounds good.  I mistrust copy constructors as a rule, myself.

>+    char const * const ics = icaltime_as_ical_string(t);
>>>Can you take a look at this ^?  It compiled, but I've never seen two const declarations
>on the same line.  Please correct it unless there is some wizardry going on here
>that I am unaware of.  Also I think it should be const char * and not char const *, the 
>former is more readable.
><<
>What I am following here with double const is const-correctness.
>"const char * ics" or "char const* ics" (which is the same) leaves "ics" modifyable.
>I (and many collegues) prefer "T const...", because you have to read C pointers and
>type qualifiers from right to left like "ics is a const pointer to const char.".
>That way it's easier to read. Consider e.g. "const char * const*". "char const* const*"
>reades easier IMHO. Moreover compilers usually dump out that same style.
Not sure how I have coded C++ for this long and never seen this.  Perhaps that says something about the code I have helped to write! 8-o But, I like what you're doing, and I agree that it's more readable this way.  Leave it as is, please.

>+    // xxx todo: discuss is_daylight
>+//     if (tz) {
>+//         icaltimezone_get_utc_offset(tz, icalt, &icalt->is_daylight);
>+//     }
>>>What needs to be discussed here?  What is the issue?  I think is_daylight is used
>by libical in odd ways.
><<
>Yes, that's my assumption, too. We've to face that it hasn't been minded yet.
>Maybe someone knows why that's correct or not?
This is a GREAT PLACE for some unit tests.  I'll see if I can get mschroeder to add it to his list.  Of course it'd have to be a C unit test written against libical directly, but maybe we have a contributor that wants the challenge.  
Let's file a follow-on bug for investigation and elimination (hopefully) of this line, if we can determine that it has no effect.

In my testing, I used daylight and non-daylight, northern and southern zones, and all times were computed correctly.  So, I tend to think that is_daylight matters less than we fear.

>-    // nsISupports interface
>     NS_DECL_ISUPPORTS
>-
>-    // calIDateTime interface
>     NS_DECL_CALIDATETIME
>-
>-    // nsIXPCScriptable interface
>     NS_DECL_NSIXPCSCRIPTABLE
>>>NIT: Why remove these comments?  I'd opt to leave them.
><<They don't buy us anything IMO.
No, they don't.  But if you're looking at our codebase to learn what is going on, they are slightly helpful in understanding these macros.  That's why I'd leave them in.  I can go either way though.

>+    nsCOMPtr<calITimezone> tz;
>+    if (tzid_) {
>+        nsDependentCString const tzid(tzid_);
>+        calIcalComponent * comp = nsnull;
>+        if (parent) {
>+            comp = parent->getParentVCalendarOrThis();
>+        }
>+        // look up parent if timezone is already referenced:
>+        if (comp) {
>+            comp->mReferencedTimezones.Get(tzid, getter_AddRefs(tz));
>+        }
>+        if (!tz) {
>+            icaltimezone const* zone = itt.zone;
>+            if (!zone && comp) {
>+                // look up parent VCALENDAR for VTIMEZONE:
>+                zone = icalcomponent_get_timezone(comp->mComponent, tzid_);
>+                NS_ASSERTION(zone, tzid_);
>+            }
>+            if (zone) {
>+                nsCOMPtr<calIIcalComponent> const tzComp(
>+                    new calIcalComponent(icaltimezone_get_component(const_cast<icaltimezone *>(zone)),
>+                                         comp));
>+                CAL_ENSURE_MEMORY(tzComp);
>+                tz = new calTimezone(tzComp, tzid);
>+                CAL_ENSURE_MEMORY(tz);
>+            }
>+            if (!tz && parent) {
>+                // look up tz provider:
>+                calITimezoneProvider * const tzProvider = parent->getTzProvider();
>+                if (tzProvider) {
>+                    tzProvider->GetTimezone(tzid, getter_AddRefs(tz));
>+                    NS_ASSERTION(tz, tzid_);
>+                }
>+            }
>+            if (!tz) {
>+                // look up tz in tz service.
>+                // this hides errors from incorrect ics files, which could state
>+                // a TZID that is not present in the ics file.
>+                // The other way round, it makes this product more error tolerant.
>+                cal::getTimezoneService()->GetTimezone(tzid, getter_AddRefs(tz));
>+                NS_ASSERTION(tz, tzid_);
>+            }
>+            if (comp && tz) {
>+                // assure timezone is known:
>+                comp->AddTimezoneReference(tz);
>+            }
>+        }
>+        if (tz) {
>+            // correct itt which would else appear floating:
>+            itt.zone = cal::getIcalTimezone(tz);
>+            itt.is_utc = 0;
>         }
>>> TODO: Follow on bug >> We need to document this order of precdence for finding
>a timezone definition so that we always do this the same way (if we're looking them
>up in some other place (which I hope we're not, but still...)
><<where should we document that?
For now, I suggest we use the Timezone Feature page on the wiki: http://wiki.mozilla.org/Calendar:Timezones, and aim to get that into any kind of developer documentation that we do for provider developers.

>+    if (!itt.is_utc && itt.zone) {
>+        if (parent) {
>+            nsCOMPtr<calITimezone> tz;
>+            nsresult rv = dt->GetTimezone(getter_AddRefs(tz));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+            rv = parent->getParentVCalendarOrThis()->AddTimezoneReference(tz);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+        } // else set TZID even though no containing component
>+          // is present to add the VTIMEZONE to
>+        icalparameter * const param = icalparameter_new_from_value_string(
>+            ICAL_TZID_PARAMETER, icaltimezone_get_tzid(const_cast<icaltimezone *>(itt.zone)));
>+        icalproperty_set_parameter(prop, param);
>>> Under what circumstances do we not have a containing component?  This seems like
>it might generate invalid ICS if we hit it and then did an export of the containing
>calendar.
><<Ok.
>There is e.g. calRecurrenceDate::GetIcalProperty().
>I consider this fragile, maybe calIcalProperty could hold it's calITimezone,
>so when added to the parent component that timezone is known.
This is worth investigating, but let's not do that in this patch (we're already doing enough). Please file a follow on bug for this.

>+    // xxx todo: weak assumption that this VTIMEZONE still has a parent since
>+    //           we need it to get an icaltimezone...
>+    NS_ASSERTION(mParent, "VTIMEZONE has no parent!");
>>> I'm fine with this assumtion as  long as we can be reasonably sure we aren't 
>writing ICS without a parent (see previous comment).  However, I'd like to make
>this stronger than an assertion.  Can we log this to the Error Console instead? That
>way we can easily tell when this code has been called when no parent exists and we 
>can easily tell whether or not this is something that we will need to resolve?
>Let's log this error to the error console.
><<
>Would be nice, but I think it's only a topic for providers and the timezone service
>and those should be able to follow this constraint, else they would easily face their
>timezone support doesn't work. As far as I can see no user action can cause this.
I'm ok with that.

>-    icalcomponent_merge_component(comp, tzcomp);
>+AddTimezoneComponentToIcal(nsACString const& tzid, calITimezone * tz, void * arg)
>+{
>+    icalcomponent * const comp = static_cast<icalcomponent *>(arg);
>+    icaltimezone * icaltz = cal::getIcalTimezone(tz);
>+    if (icaltz) {
>+        icalcomponent * const tzcomp = icalcomponent_new_clone(icaltimezone_get_component(icaltz));
>+        icalcomponent_add_component(comp, tzcomp);
>+    }
>>> Why do we add this timezone component via icalcomponent_add when it was traditionally
>performed by icalcomponent_merge?  Why this change?  Unless there is a good reason,
>then I think we should revert to using icalcomponent_merge...
><<
>Single reason is that icalcomponent_merge expects a VCALENDAR (and asserts otherwise),
>but we want to put in a VTIMEZONE. We could easily use add here since the
>mReferencedTimezones bag only has only one VTIMEZONE per TZID.
Thanks for the explanation!  Let's use add

> 
>-    icalcomponent_add_component(mComponent, ical->mComponent);
>+    if (ical->mParent) {
>+        ical->mComponent = icalcomponent_new_clone(ical->mComponent);
>+    }
>     ical->mParent = this;
>+    icalcomponent_add_component(mComponent, ical->mComponent);
>>> I don't understand this.  Why does the existence of the ical's mParent mean that
>we need to clone the ical->mComponent?  Shouldn't we always clone it since we're adding
>ical->mComponent to a new component?
><<
>That code has been in before, I just shifted it down.
>The reason is that in case the incoming component (to be added) is part of
>another parent component (i.e. ical->mParent is set), then we need to copy
>that portion and get it into the called component, else that icalcomponent
>would be referred (and freed) from two parent components later on.
Ah OK.


>-    // XXX like AddSubcomponent, this is questionable
>     NS_ENSURE_ARG_POINTER(prop);
>-
>-    calIcalProperty *ical = static_cast<calIcalProperty *>(prop);
>-    if(ical->mParent)
>+    // XXX like AddSubcomponent, this is questionable
>>> Yes, please explain.  Perhaps some comments about this would be good to,
>for future reference when people are wondering about memory usage.
><<
>We assume a calIcalProperty is passed in (else the cast wouldn't run and
>we are about to crash), so we assume that this ICS service code has created
>the property. Should I add this comment?
I think this is a good comment to add.

>+//     // XXX like AddSubcomponent, this is questionable
>+//     calIcalProperty *ical = static_cast<calIcalProperty *>(prop);
>+//     icalcomponent_remove_property(mComponent, ical->mProperty);
>+//     ical->mParent = nsnull;
>+//     return NS_OK;
>+// }
>>> Why does removeProperty get removed?  I don't see the bearing of this on
>the timezone reference changes.
><<
>I explained this above. If you add then remove a property/component, the referenced
>timezones won't get purged out. Because there's currently no caller, I don't see a
>need to support this (or repair, because it's been broken before).
This is also some good text to add to the comment above. Perhaps merging these two thoughts together.  Something like if we're going to support removing the timezone when we remove its referring component then we'll need to change....


>+    calITimezoneProvider * getTzProvider() const {
>>> It's much more readable (to me) to have "const" come first
><<
>You're mixing up different C++ concepts here. Qualifying a member function "const"
>like this ensures the function has only read permissions on the object
>(at most could write "mutable" members which indicates bad design BTW).
>It's a common way to ensure const correctness:
> You are stating that the function doesn't modify the object
> (and the compiler helps to enforce that).
Good to know, thanks

>+
>+    calIcalComponent * getParentVCalendarOrThis() {
>+        calIcalComponent * that = this;
>>> Something more descriptive than "that" for the variable, please
I think we need to describe what the role of "that" is here.  People not accustomed to this code will find this difficult.  You can leave the variable, if you want, but please add a comment about what this is doing.

>-    mEnd = new calDateTime(&(icalp->end));
>+    mEnd = new calDateTime(&(icalp->end), nsnull);
>>> What if the start and end time are in two different timezones?  Then, you need
>to pass in the timezone in order to properly set the time difference.
><<
>First look I thought that, too (same applies for RRULE UNTIL).
>But periods can only specifiy floating/UTC, there's no TZID parameter.
>So I let calDatetime deduce floating/UTC from the icaltime (no tz passed).
Good to know.  Then this code is fine as is.

>Index: calendar/base/src/calRecurrenceRule.h
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calRecurrenceRule.h,v
>retrieving revision 1.1.6.1
>diff -u -8 -p -r1.1.6.1 calRecurrenceRule.h
>--- calendar/base/src/calRecurrenceRule.h	29 Oct 2007 16:06:41 -0000	1.1.6.1
>+++ calendar/base/src/calRecurrenceRule.h	29 Nov 2007 20:13:31 -0000
>@@ -1,9 +1,8 @@
>
>  * ***** END LICENSE BLOCK ***** */
>-
>-#ifndef CALRECURRENCERULE_H_
>-#define CALRECURRENCERULE_H_
>+#if !defined(INCLUDED_CAL_RECURRENCERULE_H)
>+#define INCLUDED_CAL_RECURRENCERULE_H
> >> There is no need to change these
><<
>Maybe I should explain my overeagerness w.r.t. preprocessing macros a bit ;)
>C++ formally forbids to define macros starting either with a double underscore
>and a lower-case letter or a single underscore and a upper-case letter, because
>those are reserved for the system/runtime headers, e.g. __cplusplus or _CSTDDEF_.
>I am not sure about trailing underscores, though I've already seen those in system headers,
>but whenever I see an include guard like the above, I tend to remove those underscores,
>and most often I garnish the macro to carry its purpose, like INCLUDED_FOO.
With this explanation that sounds fine. I abandon my demands w.r.t. include headers.

>+                        NS_WARNING(ical_timezone_data[i].tzid);
>>> Include some warning text with this so we know why we're being warned about this zone
><< Don't you think the file/line number is sufficient? This would only bloat the code...
I suppose that's fine.  Since I wouldn't want to spend the effort to make it a localizable error it won't really be that intelligible to most users anyway.

>+nsCOMPtr<calITimezoneService> const& getTimezoneService() {
>+    static nsCOMPtr<calITimezoneService> sTzService;
>+    if (!sTzService) {
>+        sTzService = do_GetService(CAL_TIMEZONESERVICE_CONTRACTID);
>+        NS_ASSERTION(sTzService, "Could not init timezone service! Will crash now...");
>>> We need to figure this out and understand whether we want to ship with an old, outdated
>tzdata.c file just so that we have *something* in the event that the user deletes the 
>timezone extension. Thoughts?  This probably should be filed as a follow on bug.
><<
>Yeah, we need to resolve this. What we need to think about, too, is how to model
>the inter-xpi dependency. I think the timezone service code should be kept with
>lightning-core.xpi, so we don't have to juggle native code for the timezone-database.xpi;
>that one should only ship the timezones.sdb, if possible.
>
>IMO lightning-core.xpi should be dependent on the timezones-database.xpi.
>
>That brings us to the update story:
>- timezones-database.xpi has no update url
>  => asks AMO for latest xpi, updates..
>- lightning-core.xpi has no update url
>  => asks AMO for latest xpi, updates lightning.xpi
>     which contains {lightning-core.xpi, timezones-database.xpi [, providers]}?
>Is that feasible?
>
>We could make lightning-core.xpi run without timezones-database.xpi and fall back
>to the compliled tzdata.c stuff, but I wouldn't want that since it's almost never
>used and only bloats the download sets.

Yeah, we need to figure this out.  I also don't want to ship the tzdata.c if we can reasonably avoid it.  Let's create a follow on bug for updating the tz database so that we will have a forum to discuss the update strategies.


>+        if (tzid) {
>+            nsCOMPtr<calITimezone> tz;
>+            tzProvider->GetTimezone(nsDependentCString(tzid), getter_AddRefs(tz));
>+            if (tz) {
>+                return tz;
>+            }
>+            NS_ASSERTION(tz, "no timezone found, falling back to floating!");
>>> This is always bad.  We should probably elevate this warning into the Error Console.
><<ok. yes, I agree.
Yeah, let's do that.

> function compareObjects(aObject, aOtherObject, aIID) {
>+    // xxx todo: seems to work fine e.g. for WCAP, but I still mistrust this trickery...
>+    //           Anybody knows an official API that could be used for this purpose?
>+    //           For what reason do clients need to pass aIID since
>+    //           every XPCOM object has to implement nsISupports?
>+    //           XPCOM (like COM, like UNO, ...) defines that QueryInterface *only* needs to return
>+    //           the very same pointer for nsISupports during its lifetime.
>+    if (!aIID) {
>+        aIID = Components.interfaces.nsISupports;
>+    }
>>> I don't understand what you're asking and what the deal is here.
><<
>- How to test two references (wrapped or not) whether they refer to the same object?
>  Is there an official API?
>- I don't know why compareObjects has a third parameter since both of the passed
>  references refer to XPCOM objects which implement nsISupports.
>- In native code, you could test two references by simply querying both references
>  to nsISupports, then comparing the interface pointer. nsISupports is the only pointer
>  that needs to stay stable over an object's lifetime.
I think the reason for the aIID parameter is so that you can test if objectA and objectB both implement interface aIID. Since they all must be nsISupports as well, I don't see any harm in this code on the surface.  Am I still missing something?  I'm not aware of any "official" API for this, but my knowledge of the platform internals is not comprehensive.

Overall, this looks good.
r=ctalbert with those small changes and follow-on bugs.  Great job Daniel!
Comment on attachment 290734 [details] [diff] [review]
core part, unit tests

Granting review, as per the information in comment 13.

r=ctalbert
Attachment #290734 - Flags: review?(ctalbert) → review+
Comment on attachment 290735 [details] [diff] [review]
alarm service, providers

Code in general looks good, haven't gotten around to testing yet though. There is the one or other XXX TODO still in your code, I'm assuming you will take care of that before checkin. (The one that I noticed was in a toString() method in wcap)


Regarding Google: The Google timezone code looks a bit strange to me, since at one point the full mozilla timezone id is passed and at a different point only the Google/Olson timezone id. But this is not really a problem for this bug.

getMozillaTimezone can stay as you rewrote it, but since I fallback to UTC() everywhere that fromRFC3339 is called from, this version should also work fine (untested!). I should probably fall back to floating() instead of UTC(), but again this is not part of this bug.

function getMozillaTimezone(aICALTimezone) {
    ASSERT(aICALTimezone, "No timezone passed", true);
    if (aICALTimezone == "floating") {
       return floating();
    } else if (aICALTimezone == "UTC") {
       return UTC();
    }

    // [...] 

    return floating();
 }

>@@ -252,17 +250,17 @@ function fromRFC3339(aStr, aTimezone) {
>     var dateTime = createDateTime();
> 
>     // Killer regex to parse RFC3339 dates
>     var re = new RegExp("^([0-9]{4})-([0-9]{2})-([0-9]{2})" +
>         "([Tt]([0-9]{2}):([0-9]{2}):([0-9]{2})(\.[0-9]+)?)?" +
>         "(([Zz]|([+-])([0-9]{2}):([0-9]{2})))?");
> 
>     var matches = re.exec(aStr);
>-    var moztz = getMozillaTimezone(aTimezone) || "UTC";
>+    var moztz = getMozillaTimezone(aTimezone) || UTC();
Since the way you rewrote getMozillaTimezone always returns a timezone, we don't need || UTC() here.


r=philipp
Attachment #290735 - Flags: review?(philipp) → review+
(In reply to comment #13)
> >+    // xxx todo: discuss is_daylight
> >+//     if (tz) {
> >+//         icaltimezone_get_utc_offset(tz, icalt, &icalt->is_daylight);
> >+//     }
> >>>What needs to be discussed here?  What is the issue?  I think is_daylight is used
> >by libical in odd ways.
> ><<
> >Yes, that's my assumption, too. We've to face that it hasn't been minded yet.
> >Maybe someone knows why that's correct or not?
> This is a GREAT PLACE for some unit tests.  I'll see if I can get mschroeder to
> add it to his list.  Of course it'd have to be a C unit test written against
> libical directly, but maybe we have a contributor that wants the challenge.  
> Let's file a follow-on bug for investigation and elimination (hopefully) of
> this line, if we can determine that it has no effect.
> 
> In my testing, I used daylight and non-daylight, northern and southern zones,
> and all times were computed correctly.  So, I tend to think that is_daylight
> matters less than we fear.
filed follow-up bug 406573

> >>> TODO: Follow on bug >> We need to document this order of precdence for finding
> >a timezone definition so that we always do this the same way (if we're looking them
> >up in some other place (which I hope we're not, but still...)
> ><<where should we document that?
> For now, I suggest we use the Timezone Feature page on the wiki:
> http://wiki.mozilla.org/Calendar:Timezones, and aim to get that into any kind
> of developer documentation that we do for provider developers.
added section <http://wiki.mozilla.org/Calendar:Timezones#Lookup_precedence>

> >+    if (!itt.is_utc && itt.zone) {
> >+        if (parent) {
> >+            nsCOMPtr<calITimezone> tz;
> >+            nsresult rv = dt->GetTimezone(getter_AddRefs(tz));
> >+            NS_ENSURE_SUCCESS(rv, rv);
> >+            rv = parent->getParentVCalendarOrThis()->AddTimezoneReference(tz);
> >+            NS_ENSURE_SUCCESS(rv, rv);
> >+        } // else set TZID even though no containing component
> >+          // is present to add the VTIMEZONE to
> >+        icalparameter * const param = icalparameter_new_from_value_string(
> >+            ICAL_TZID_PARAMETER, icaltimezone_get_tzid(const_cast<icaltimezone *>(itt.zone)));
> >+        icalproperty_set_parameter(prop, param);
> >>> Under what circumstances do we not have a containing component?  This seems like
> >it might generate invalid ICS if we hit it and then did an export of the containing
> >calendar.
> ><<Ok.
> >There is e.g. calRecurrenceDate::GetIcalProperty().
> >I consider this fragile, maybe calIcalProperty could hold it's calITimezone,
> >so when added to the parent component that timezone is known.
> This is worth investigating, but let's not do that in this patch (we're already
> doing enough). Please file a follow on bug for this.
filed follow up bug 406576

> >-    // XXX like AddSubcomponent, this is questionable
> >     NS_ENSURE_ARG_POINTER(prop);
> >-
> >-    calIcalProperty *ical = static_cast<calIcalProperty *>(prop);
> >-    if(ical->mParent)
> >+    // XXX like AddSubcomponent, this is questionable
> >>> Yes, please explain.  Perhaps some comments about this would be good to,
> >for future reference when people are wondering about memory usage.
> ><<
> >We assume a calIcalProperty is passed in (else the cast wouldn't run and
> >we are about to crash), so we assume that this ICS service code has created
> >the property. Should I add this comment?
> I think this is a good comment to add.
did that

> >+//     // XXX like AddSubcomponent, this is questionable
> >+//     calIcalProperty *ical = static_cast<calIcalProperty *>(prop);
> >+//     icalcomponent_remove_property(mComponent, ical->mProperty);
> >+//     ical->mParent = nsnull;
> >+//     return NS_OK;
> >+// }
> >>> Why does removeProperty get removed?  I don't see the bearing of this on
> >the timezone reference changes.
> ><<
> >I explained this above. If you add then remove a property/component, the referenced
> >timezones won't get purged out. Because there's currently no caller, I don't see a
> >need to support this (or repair, because it's been broken before).
> This is also some good text to add to the comment above. Perhaps merging these
> two thoughts together.  Something like if we're going to support removing the
> timezone when we remove its referring component then we'll need to change....
did that

> >+    calIcalComponent * getParentVCalendarOrThis() {
> >+        calIcalComponent * that = this;
> >>> Something more descriptive than "that" for the variable, please
> I think we need to describe what the role of "that" is here.  People not
> accustomed to this code will find this difficult.  You can leave the variable,
> if you want, but please add a comment about what this is doing.
added a comment

> >>> We need to figure this out and understand whether we want to ship with an old, outdated
> >tzdata.c file just so that we have *something* in the event that the user deletes the 
> >timezone extension. Thoughts?  This probably should be filed as a follow on bug.
> ><<
> >Yeah, we need to resolve this. What we need to think about, too, is how to model
> >the inter-xpi dependency. I think the timezone service code should be kept with
> >lightning-core.xpi, so we don't have to juggle native code for the timezone-database.xpi;
> >that one should only ship the timezones.sdb, if possible.
> >
> >IMO lightning-core.xpi should be dependent on the timezones-database.xpi.
> >
> >That brings us to the update story:
> >- timezones-database.xpi has no update url
> >  => asks AMO for latest xpi, updates..
> >- lightning-core.xpi has no update url
> >  => asks AMO for latest xpi, updates lightning.xpi
> >     which contains {lightning-core.xpi, timezones-database.xpi [, providers]}?
> >Is that feasible?
> >
> >We could make lightning-core.xpi run without timezones-database.xpi and fall back
> >to the compliled tzdata.c stuff, but I wouldn't want that since it's almost never
> >used and only bloats the download sets.
> 
> Yeah, we need to figure this out.  I also don't want to ship the tzdata.c if we
> can reasonably avoid it.  Let's create a follow on bug for updating the tz
> database so that we will have a forum to discuss the update strategies.
filed follow up bug 406579

> >+        if (tzid) {
> >+            nsCOMPtr<calITimezone> tz;
> >+            tzProvider->GetTimezone(nsDependentCString(tzid), getter_AddRefs(tz));
> >+            if (tz) {
> >+                return tz;
> >+            }
> >+            NS_ASSERTION(tz, "no timezone found, falling back to floating!");
> >>> This is always bad.  We should probably elevate this warning into the Error Console.
> ><<ok. yes, I agree.
> Yeah, let's do that.
filed follow up enh bug 406581 
(In reply to comment #16)
> function getMozillaTimezone(aICALTimezone) {
>     ASSERT(aICALTimezone, "No timezone passed", true);
>     if (aICALTimezone == "floating") {
>        return floating();
>     } else if (aICALTimezone == "UTC") {
>        return UTC();
>     }
> 
>     // [...] 
> 
>     return floating();
>  }
did that, seems to work.

> >-    var moztz = getMozillaTimezone(aTimezone) || "UTC";
> >+    var moztz = getMozillaTimezone(aTimezone) || UTC();
did that.



Checked in on HEAD and MOZILLA_1_8_BRANCH.

Thank to you guys for the quick review!

Since this is quite an invasive change, please keep your eyes open for regressions ;)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Depends on: 406801
Blocks: 393414
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.