[webkit-reviews] review granted: [Bug 32876] HTMLInputElement::valueAsDate getter support : [Attachment 45396] Proposed patch (rev.2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 32876: HTMLInputElement::valueAsDate getter support
https://bugs.webkit.org/show_bug.cgi?id=32876

Attachment 45396: Proposed patch (rev.2)
https://bugs.webkit.org/attachment.cgi?id=45396&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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