834988 - Various date/time code naming/documentation improvements

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

There's a lot wrong with the current code in terms of naming and lack of documentation.  (I also have my doubts that some parts of it are actually correct, as currently constructed, but it's hard to be sure one way or the other without doing some cleanups first.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: LocalUTCDifferenceSeconds changes

If the |(localTimeSeconds +/- ....)| bit looks wrong, that's because of more totally wrong naming, to be fixed in the next patch.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Various DST offset fixes

There's more that should be done in this stuff, specifically around AdjustTime, but I haven't yet figured it out well enough to write a patch improving it.

Comment 3

[David Mandelin [:dmandelin]]

Comment on attachment 706700 [details] [diff] [review]
Various DST offset fixes

Review of attachment 706700 [details] [diff] [review]:
-----------------------------------------------------------------

The old parameter naming was really messed up. I would prefer something like 't' to utcSeconds, since it's really just a Unix/C time value.

Comment 4

[Jeff Walden [:Waldo]]

It took me awhile to realize that in C, |t| is always a UTC time.  It's not in ES5, unfortunately, so I think the more-verbose, more-explicit naming is worthwhile for readers (like me, before I started these patches/reviews, and possibly after as well) more familiar with ECMAScript than with C date/time functionality.

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42042277940b
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba0f5b6013c

I want to do a little bit more to the thoroughly byzantine AdjustTime, but at this point maybe I should just punt to another bug.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42042277940b
https://hg.mozilla.org/mozilla-central/rev/aba0f5b6013c