[webkit-reviews] review granted: [Bug 233795] Use `Seconds` as the underlying value for `MonotonicTime`/`WallTime`/etc. : [Attachment 446265] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 233795: Use `Seconds` as the underlying value for
`MonotonicTime`/`WallTime`/etc.
https://bugs.webkit.org/show_bug.cgi?id=233795

Attachment 446265: Patch

https://bugs.webkit.org/attachment.cgi?id=446265&action=review




--- Comment #7 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 446265
  --> https://bugs.webkit.org/attachment.cgi?id=446265
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?