Bug 32876

Created attachment 45395 [details] Proposed patch

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

Attachment 45395 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/138451

Created attachment 45396 [details] Proposed patch (rev.2) - Fix style errors.

style-queue ran check-webkit-style on attachment 45396 [details] without any errors.

Attachment 45396 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/139189

@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 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

(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.

(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.

The use of milliseconds is for compatibility with JS Date, btw.

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'.

(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.

(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.

(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!