Bug 27020

Summary: msToGregorianDateTime ignores outputIsUTC param when filling in utcOffset
Product: WebKit Reporter: Joe Mason <joenotcharles>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, manyoso, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 23154    
Attachments:
Description Flags
patch to DateMath.cpp staikos: review+

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.