RESOLVED FIXED 27020
msToGregorianDateTime ignores outputIsUTC param when filling in utcOffset 
https://bugs.webkit.org/show_bug.cgi?id=27020
Summary msToGregorianDateTime ignores outputIsUTC param when filling in utcOffset
Joe Mason
Reported 2009-07-06 20:03:13 PDT
msToGregorianDateTime has an outputIsUTC parameter - if false, the output should be in localtime. If the parameter is true, the result GregorianDateTime structure is filled with a time in UTC, but the utcOffset field is still set to the local timezone offset as if it were filled with a local time. I believe it should always be 0 if outputIsUTC is true. This happens to work in most cases because utcOffset is never used - it happens that everywhere msToGregorianDateTime is called with outputIsUTC true, it's in a code path which doesn't look at utcOffset. I don't think we should be depending on this, though.
Attachments
patch to DateMath.cpp (1.41 KB, patch)
2009-07-06 20:58 PDT, Joe Mason 		staikos: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joe Mason
Comment 1 2009-07-06 20:58:38 PDT
Created attachment 32358 [details] patch to DateMath.cpp
Eric Seidel (no email)
Comment 2 2009-07-07 00:06:20 PDT
So this code path is never used?
Joe Mason
Comment 3 2009-07-07 08:03:03 PDT
The most common idiom boils down to: msToGregorianDateTime(millisecs, outputIsUTC, t); if (outputIsUTC) // do something with t.month, t.year, etc. but not t.utcOffset else // do something with t.month, t.year, etc. adjusting for t.utcOffset So the code path in this patch is followed, but in the case where utcOffset is wrong it isn't used anyway. I believe all calls to msToGregorianDateTime work like this, but some of them are pretty complex so I can't be 100% sure. We have this patch in our WinCE branch because at one point Google Calendar didn't work for us without it. I thought that it worked now, but I was submitting it anyway because it seemed like the correct thing to do (and I figured if not we would find that out during the code review.) However, Yong thinks Google Calendar still doesn't work for us without this patch. I'm trying to find a test case now.
Adam Treat
Comment 4 2009-07-14 07:53:25 PDT
Joe, can you update this bug with latest info you have about the testcase?
Joe Mason
Comment 5 2009-07-14 14:32:46 PDT
I haven't been able to find any broken behaviour that this patch fixes. However, I also haven't been able to find anything that it breaks, so I think it should go in because it makes the code more resistant to future breakage.
George Staikos
Comment 6 2009-07-15 08:11:36 PDT
Comment on attachment 32358 [details] patch to DateMath.cpp absence of comments otherwise and it looks both correct and reasonable to me, so r+
Adam Treat
Comment 7 2009-07-15 09:59:00 PDT
Landed with r45918.
Note You need to log in before you can comment on or make changes to this bug.