423730 - [1.1] Implement XPath function adjust-dateTime-to-timezone

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Merle Sterling]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008031713 Minefield/3.0b4pre

We need to implement the XForms 1.1 XPath function adjust-dateTime-to-timezone().


Reproducible: Always

Comment 1

[Merle Sterling]

Attachment: testcase

Comment 2

[Merle Sterling]

Attachment: testcase2

Additional testcase. This one expects results for Central Standard Time and also adds a test for the case where the the dateTime argument includes time zone information.

Comment 3

[Merle Sterling]

Attachment: patch

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 310483 [details] [diff] [review]
patch

>+  char ctime[60];
You could say why 60

>+  char zone_location[40];
And here why 40

Would be great to have some more tests. At least some cases
where the original time zone is +XX:00

Comment 5

[Merle Sterling]

(In reply to comment #4)
> (From update of attachment 310483 [details] [diff] [review])
> >+  char ctime[60];
> You could say why 60
> >+  char zone_location[40];
> And here why 40
One and only one reason: Trying to be consistent with other functions that manipulate time. In all cases the size of the buffer could probably be shorter.

Comment 6

[aaronr]

Comment on attachment 310483 [details] [diff] [review]
patch

>Index: nsXFormsXPathFunctions.cpp
>===================================================================
 
>+NS_IMETHODIMP
>+nsXFormsXPathFunctions::AdjustDateTimeToTimezone(const nsAString &aDateTime,
>+                                                 nsAString &aResult)

...

>+
>+  // The dateTime is valid, so get the timeZone information. If there is time
>+  // time zone information we are dealing with case 3 and have a bit more work
>+  // to do to convert aDateTime to the local time zone.

nit: you have 'there is time time zone information'.  One too many times :)

>+  nsAutoString timeString, timeZoneString;
>+  PRInt32 timeSeparator = aDateTime.FindChar(PRUnichar('T'));
>+  timeString.Append(Substring(aDateTime,
>+                              timeSeparator + 1,
>+                              aDateTime.Length() - timeSeparator));
>+  nsXFormsUtils::GetTimeZone(timeString, timeZoneString);
>+
>+  PRExplodedTime time;
>+  char ctime[60];
>+
>+  if (!timeZoneString.IsEmpty()) {
>+    nsAutoString hoursString, minutesString;
>+    hoursString.Append(Substring(timeZoneString, 1, 2));
>+    minutesString.Append(Substring(timeZoneString, 4, 2));

nit: comment here.  Just giving an example of what the timeZoneString would look like at this point would probably be enough.

>+
>+    nsresult rv;
>+    PRInt32 hours = hoursString.ToInteger(&rv);
>+    PRInt32 minutes = minutesString.ToInteger(&rv);
>+    PRInt32 tzSecs = (hours * 3600) + (minutes * 60);
>+    if (timeZoneString.CharAt(0) == '+') {

looks like you are assuming that the time zone string will contain the '+' or '-'.  But in nsXFormsUtils::GetTimeZone it looks like you are skipping over the +/- when building your string

...

>+    // the time to the local time zone.
>+    PR_NormalizeTime(&time, PR_LocalTimeParameters);
>+    PR_FormatTime(ctime, sizeof(ctime), "%Y-%m-%dT%H:%M:%S\0", &time);
>+
>+  } else {
>+    // No time zone information or a GMT time.

nit: make comment a little clearer.  Perhaps, 'This is either a GMT time or there is no time zone information available'.

>+    PR_ExplodeTime(t_dateTime, PR_LocalTimeParameters, &time);
>+    PR_FormatTime(ctime, sizeof(ctime), "%Y-%m-%dT%H:%M:%S\0", &time);
>+  }
>+

Please verify that your code will work with '+' and '-' testcases (the substring problem I noted before), fix nits and I'll review again.

Comment 7

[Merle Sterling]

Attachment: testcase3

Additional testcase with dateTimes that have time zone information. The last two cases are -00:00 and +00:00 which are both GMT times.

Comment 8

[Merle Sterling]

(In reply to comment #6)
> >+    nsresult rv;
> >+    PRInt32 hours = hoursString.ToInteger(&rv);
> >+    PRInt32 minutes = minutesString.ToInteger(&rv);
> >+    PRInt32 tzSecs = (hours * 3600) + (minutes * 60);
> >+    if (timeZoneString.CharAt(0) == '+') {
> looks like you are assuming that the time zone string will contain the '+' or
> '-'.  But in nsXFormsUtils::GetTimeZone it looks like you are skipping over > the +/- when building your string

Good catch. The testcases were working because they only had negative time zone offsets and I wasn't explicitly testing for '-' (so the time zone offset would be added as it should, but the '+' case would not have worked). I added a testcase with positive time zone offsets.

Comment 9

[Merle Sterling]

Attachment: patch

Fixes Aaron's nits and the problem in nsXFormsUtils::GetTimeZone where the time zone string being built skipped the leading the + or -.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, is this right after all. Here in Finland I get datetime ending with +01:00
although it should be +03:00 (now during summer time)
For example in the test 3, the last test,
adjust-dateTime-to-timezone('2007-10-07T02:22:00+00:00') (GMT) becomes
2007-10-07T05:22:00+01:00

Merle, any comments?

Comment 11

[Merle Sterling]

Attachment: patch

Fix calculation of time zone string.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 323620 [details] [diff] [review]
patch

Works fine here

Comment 13

[Merle Sterling]

Attachment: Patch for 1.8 branch

Comment 14

[aaronr]

checked into 1.8 branch for msterlin