Closed Bug 867312 Opened 7 years ago Closed 7 years ago

Expose some APIs for working with dates the way the JS engine does

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

For a start, js/public/Date.h which has a js::MakeDate or something which takes an integer year, a 0-11 month, and a 1-based day and produces a double millisecond timestamp.
Blocks: 742206
Whiteboard: [need review]
Attachment #743779 - Attachment is obsolete: true
Attachment #743779 - Flags: review?(jwalden+bmo)
I guess it's not clear whether MakeDate should call TimeClip....
Comment on attachment 743792 [details] [diff] [review]
Expose some APIs for creating millisecond timestamps corresponding to JS dates.

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

Some sort of test that would have failed with the old <input> behavior would be nice, but I can't imagine it matters much, as no browser would ever be so stupid as to construct a Date object from the time value and call methods on it, assuming Date.prototype.* has its original values.  Right?  ;-)

::: js/public/Date.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Add a #include "jstypes.h", please, so that JS_PUBLIC_API is actually defined and not bootlegged.  :-)

And, um, #include guards, please?  Copy the style from some other js/public header.

::: js/src/jsdate.cpp
@@ +48,5 @@
>  #include "jsobjinlines.h"
>  
>  using namespace js;
>  using namespace js::types;
> +using namespace JS;

Is this actually necessary?  I'm not sure it is.  And I think we're moving in the direction of having explicit usings for namespace-JS stuff inside the engine, mostly, so this should go away somehow.

@@ +153,1 @@
>  YearFromTime(double t)

Generally we don't wrap things in |namespace| blocks any more -- add a JS:: at the start of the declaration instead.

JS_PUBLIC_API adds overhead for intra-library calls, as I understand it.  I'm kind of leery of assuming that extra overhead doesn't matter, without good reason.  Could you leave these as statics and do the same thing you did for JS::MakeDate and JS::DayFromTime, just to not tempt fate?
Attachment #743792 - Flags: review?(jwalden+bmo) → review+
Hmm, TimeClip.

HTML doesn't appear to impose any requirements on the range of dates that can be accurately represented.  Given that it's exposing JS Date objects in various places, it probably should.  Probably there should be a spec change, to the effect that if the represented date/time in question exceeds the TimeClip range, it's the appropriate kind of error?  That would naturally fall out of this, I think, if all these methods TimeClip'd their return value.  So *probably* these methods should all TimeClip.  But really it somewhat depends on whether the spec moves in the direction I expect/hope it would.  So I'd say make all these methods TimeClip for now, and if something needs to change later, we can change it.

I assume any local date/time strings will first be converted to UTC, before the code being changed here will get called?  There could be fuzziness around the very-distant edges when a time value is a *local* time value.  :-\
> Right? 

No comment.  ;)

> And, um, #include guards, please?

Yikes, good catch.  Will add.

> Is this actually necessary?  I'm not sure it is.

The other obvious option is to do s/YearFromTime/JS::YearFromTime/ on the whole file, and likewise for MonthFromTime.  I can do that if you'd prefer.  Let me know?

> add a JS:: at the start of the declaration instead.

OK, can do, assuming you mean the definition, not the declaration.

> Could you leave these as statics and do the same thing you did for JS::MakeDate and
> JS::DayFromTime

OK.  Need to be a bit careful with the namespacing bits, but should be doable.  Then I won't need the JS:: bits or the using bits either, I guess.

> So I'd say make all these methods TimeClip for now

It only makes sense to TimeClip the MakeDate method, no?  Certainly TimeClip on the month or day makes no sense; not sure about the year.

> I assume any local date/time strings will first be converted to UTC, before the code
> being changed here will get called?

I think that's the idea, yes.
(In reply to Boris Zbarsky (:bz) from comment #6)
> It only makes sense to TimeClip the MakeDate method, no?  Certainly TimeClip
> on the month or day makes no sense; not sure about the year.

Er, yes.  Commenting too quickly, not being precise enough.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f705f8db7c1
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/3f705f8db7c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.