Bug 32876 - HTMLInputElement::valueAsDate getter support
Summary: HTMLInputElement::valueAsDate getter support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 32697
  Show dependency treegraph
 
Reported: 2009-12-22 12:18 PST by Kent Tamura
Modified: 2009-12-23 11:11 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (29.68 KB, patch)
2009-12-22 12:32 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (29.64 KB, patch)
2009-12-22 12:35 PST, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-12-22 12:18:51 PST
The getter part of Bug#32697.
Comment 1 Kent Tamura 2009-12-22 12:32:44 PST
Created attachment 45395 [details]
Proposed patch
Comment 2 WebKit Review Bot 2009-12-22 12:34:54 PST
Attachment 45395 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLInputElement.cpp:1389:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/html/HTMLInputElement.cpp:2147:  Missing space before {  [whitespace/braces] [5]
WebCore/html/HTMLInputElement.cpp:2149:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3
Comment 3 WebKit Review Bot 2009-12-22 12:35:03 PST
Attachment 45395 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/138451
Comment 4 Kent Tamura 2009-12-22 12:35:45 PST
Created attachment 45396 [details]
Proposed patch (rev.2)

- Fix style errors.
Comment 5 WebKit Review Bot 2009-12-22 12:40:18 PST
style-queue ran check-webkit-style on attachment 45396 [details] without any errors.
Comment 6 WebKit Review Bot 2009-12-22 12:41:09 PST
Attachment 45396 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/139189
Comment 7 Adam Barth 2009-12-22 13:07:06 PST
@tkent: What's the deal with the compile warning?  Is that a real problem with the patch?

 94     static inline double invalidTotalMillisecond() { return std::numeric_limits<double>::quiet_NaN(); }
Comment 8 Darin Adler 2009-12-22 13:08:18 PST
Comment on attachment 45396 [details]
Proposed patch (rev.2)

In general, for times WTF and WebCore use doubles that are in seconds since the Unix epoch. I probably would suggest that ISODateTime do the same thing, and then have the valueAsDate function multiply by 1000, because it's HTML5 that does things in milliseconds. I like how the other code uses seconds, because that works well with world standards. Things like milliseconds and microseconds are derived from the base unit, "second". And I like having only one type of time.

WebCore::Timer and WebCore::SharedTimer are good examples of this.

I'm not so happy with the term "total millisecond". I see how it came from the fact that the millisecond part of the ISODateTime was in a field named "millisecond". But if I wanted to use the word total I would probably call it "total in milliseconds". I'm not sure I'd use "total" to mean the entire date. Note that the comment above about using seconds instead. I think a better term for the function would probably be secondsSinceEpoch(). Or if you really want to have the 1000 multiplier then I'd call it millisecondsSinceEpoch().

The enum in ISODateTime should just be called Type since it is a member of ISODateTime. If it was at namespace scope, then DateTimeType would be a good name for it.

Patch seems fine. r=me as is
Comment 9 Darin Adler 2009-12-22 13:14:04 PST
(In reply to comment #7)
> @tkent: What's the deal with the compile warning?  Is that a real problem with
> the patch?

To me it seems like #include <limits> needs to be added to that header.
Comment 10 Kent Tamura 2009-12-22 13:17:38 PST
(In reply to comment #7)
> @tkent: What's the deal with the compile warning?  Is that a real problem with
> the patch?
> 
>  94     static inline double invalidTotalMillisecond() { return
> std::numeric_limits<double>::quiet_NaN(); }

I think Chromium needs `#include <limits>' explicitly for std::numeri_limits and other ports implicitly include <limits>.
I'll check it.
Comment 11 Ian 'Hixie' Hickson 2009-12-22 14:17:19 PST
The use of milliseconds is for compatibility with JS Date, btw.
Comment 12 Kent Tamura 2009-12-23 03:30:17 PST
I committed the patch as r52524 with some modifications.

Thank you for reviewing.

(In reply to comment #8)
> In general, for times WTF and WebCore use doubles that are in seconds since the
> Unix epoch. I probably would suggest that ISODateTime do the same thing, and
> then have the valueAsDate function multiply by 1000, because it's HTML5 that
> does things in milliseconds. I like how the other code uses seconds, because
> that works well with world standards. Things like milliseconds and microseconds
> are derived from the base unit, "second". And I like having only one type of
> time.

The comment is understandable, but I didn't change so.
I feel the merit of the only-one-time-type for WebKit developers is not more important than the millisecond precision for world-wide users.
And Date in V8 supports millisecond precision.

> I'm not so happy with the term "total millisecond". I see how it came from the
> fact that the millisecond part of the ISODateTime was in a field named
> "millisecond". But if I wanted to use the word total I would probably call it
> "total in milliseconds". I'm not sure I'd use "total" to mean the entire date.
> Note that the comment above about using seconds instead. I think a better term
> for the function would probably be secondsSinceEpoch(). Or if you really want
> to have the 1000 multiplier then I'd call it millisecondsSinceEpoch().

Ok, I renamed it to millisecondsSinceEpoch().

> The enum in ISODateTime should just be called Type since it is a member of
> ISODateTime. If it was at namespace scope, then DateTimeType would be a good
> name for it.

Renamed to just `Type'.
Comment 13 Kent Tamura 2009-12-23 04:04:55 PST
(In reply to comment #12)
> > In general, for times WTF and WebCore use doubles that are in seconds since the
> > Unix epoch. I probably would suggest that ISODateTime do the same thing, and
> > then have the valueAsDate function multiply by 1000, because it's HTML5 that
> > does things in milliseconds. I like how the other code uses seconds, because
> > that works well with world standards. Things like milliseconds and microseconds
> > are derived from the base unit, "second". And I like having only one type of
> > time.
> 
> The comment is understandable, but I didn't change so.
> I feel the merit of the only-one-time-type for WebKit developers is not more
> important than the millisecond precision for world-wide users.

Ah, Darin meant representing a millisecond part as a fractional number, not removing a millisecond part.
Comment 14 Kent Tamura 2009-12-23 08:58:37 PST
(In reply to comment #13)
> Ah, Darin meant representing a millisecond part as a fractional number, not
> removing a millisecond part.

On my Mac, floor((2002.0 / 1000.0) * 1000.0) is 2001.0, not 2002.0.
We would need extra rounding code to avoid such problems if we represented milliseconds as fractional numbers.  I don't think it's reasonable.
Comment 15 Darin Adler 2009-12-23 11:11:13 PST
(In reply to comment #14)
> On my Mac, floor((2002.0 / 1000.0) * 1000.0) is 2001.0, not 2002.0.
> We would need extra rounding code to avoid such problems if we represented
> milliseconds as fractional numbers.  I don't think it's reasonable.

Yes, it's a problem to use floor rather than round. But I don't agree that the basic approach is not reasonable. You should do it!