It's already the case that `MonotonicTime`/`WallTime`/etc. use seconds as the internal representation, so we should just use `Seconds` to make it easier to convert to/from these wrapper/POD-esque objects (e.g. we would be able to drop `fromRawSeconds` and instead have a compile-time guarantee that the developer knows that the underlying value represents seconds when creating a `MonotonicTime`/`WallTime`/etc.).
Created attachment 445799 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 445874 [details] Patch
Created attachment 446263 [details] Patch
Created attachment 446265 [details] Patch
Comment on attachment 446265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446265&action=review Phil should probably be the one to review this. > Source/WTF/wtf/ApproximateTime.cpp:49 > - out.print("Approximate(", m_value, " sec)"); > + out.print("Approximate("); > + secondsSinceEpoch().dump(out); > + out.print(")"); Can't we add the necessary facilities to make this change not necessary? > Source/WTF/wtf/CurrentTime.cpp:251 > - return fromRawSeconds(currentTime()); > + return WallTime(Seconds(currentTime())); Is it really necessary to make fromRawSeconds() no longer work? > Source/WTF/wtf/CurrentTime.cpp:277 > - return static_cast<uint64_t>((m_value * 1.0e9 * info.denom) / info.numer); > + return static_cast<uint64_t>((secondsSinceEpoch().value() * 1.0e9 * info.denom) / info.numer); Is this really better?
Comment on attachment 446265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446265&action=review I think that my main worry about this change is that I'm not super sure what it improves? But, I think it's OK, if you make the changes I suggest. >> Source/WTF/wtf/ApproximateTime.cpp:49 >> + out.print(")"); > > Can't we add the necessary facilities to make this change not necessary? Why can't you just say: out.print("Approximate(", secondsSinceEpoch(), ")")? > Source/WTF/wtf/MonotonicTime.h:51 > + explicit constexpr MonotonicTime(Seconds seconds) > + : GenericTimeMixin<MonotonicTime>(seconds) > + { > + } > + Why isn't this called "fromSecondsSinceEpoch()" rather than making it a constructor? > Source/WTF/wtf/TimeWithDynamicClockType.cpp:136 > + out.print("("); > + m_seconds.dump(out); > + out.print(")"); Why isn't this just out.print("(", m_seconds, ")")? > Source/WTF/wtf/WallTime.cpp:44 > + out.print("Wall("); > + secondsSinceEpoch().dump(out); > + out.print(")"); > } Ditto. > Source/WTF/wtf/WallTime.h:52 > + explicit constexpr WallTime(Seconds seconds) > + : GenericTimeMixin<WallTime>(seconds) > + { > + } > + Ditto - I'd much rather force people to say WallTime::fromSecondsSinceEpoch(...) than WallTime(...). > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2816 > - auto dateToInsert = OperatingDate::fromWallTime(WallTime::fromRawSeconds(daysAgoInSeconds)); > + auto dateToInsert = OperatingDate::fromWallTime(WallTime(Seconds(daysAgoInSeconds))); Seems like it would be great to still be able to say fromRawSeconds. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:186 > - virtual void setLastSeen(const RegistrableDomain& primaryDomain, Seconds) = 0; > + virtual void setLastSeen(const RegistrableDomain& primaryDomain, WallTime) = 0; Are changes like this really related to this change?
<rdar://problem/86301424>