Bug 233795 – Use `Seconds` as the underlying value for `MonotonicTime`/`WallTime`/etc.

• Status: NEW
• Component: New Bugs

[Devin Rousso]

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

[Devin Rousso]

Created attachment 445799 [details]
Patch

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 445874 [details]
Patch

[Devin Rousso]

Created attachment 446263 [details]
Patch

[Devin Rousso]

Created attachment 446265 [details]
Patch

[Myles C. Maxfield]

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?

[Filip Pizlo]

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?

[Radar WebKit Bug Importer]

<rdar://problem/86301424>