Bug 27020 - msToGregorianDateTime ignores outputIsUTC param when filling in utcOffset
Summary: msToGregorianDateTime ignores outputIsUTC param when filling in utcOffset
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 23154
  Show dependency treegraph
 
Reported: 2009-07-06 20:03 PDT by Joe Mason
Modified: 2009-07-15 09:59 PDT (History)
3 users (show)

See Also:


Attachments
patch to DateMath.cpp (1.41 KB, patch)
2009-07-06 20:58 PDT, Joe Mason
staikos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 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.
Comment 1 Joe Mason 2009-07-06 20:58:38 PDT
Created attachment 32358 [details]
patch to DateMath.cpp
Comment 2 Eric Seidel (no email) 2009-07-07 00:06:20 PDT
So this code path is never used?
Comment 3 Joe Mason 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.
Comment 4 Adam Treat 2009-07-14 07:53:25 PDT
Joe, can you update this bug with latest info you have about the testcase?
Comment 5 Joe Mason 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.
Comment 6 George Staikos 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+
Comment 7 Adam Treat 2009-07-15 09:59:00 PDT
Landed with r45918.