[webkit-reviews] review granted: [Bug 222305] Timeout calculations are error-prone for compound IPC operations : [Attachment 421336] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 222305: Timeout calculations are error-prone for compound IPC operations
https://bugs.webkit.org/show_bug.cgi?id=222305

Attachment 421336: Patch

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




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 421336
  --> https://bugs.webkit.org/attachment.cgi?id=421336
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421336&action=review

r=me

> Source/WebKit/Platform/IPC/Timeout.h:47
> +    explicit constexpr Timeout(MonotonicTime absoluteTimeout)
> +	   : m_absoluteTimeout(absoluteTimeout)
> +    {
> +    }

Is this constructor ever used? I looked but didn't see a use. It feels a little
weird to construct a timeout from an absolute time, since a timeout is a time
point relative to a starting point; maybe we should not encourage it.

> Source/WebKit/Platform/IPC/Timeout.h:48
> +    static constexpr Timeout infinity() { return { }; }

I think we should either make the default constructor an infinite timeout, or
remove the default constructor and use infinity() everywhere. Having two ways
to make an infinite timeout will produce confusing code in the long run. It
looks like you preferred infinity() in this patch, so why don't we remove the
default constructor.

> Source/WebKit/Platform/IPC/Timeout.h:50
> +    operator Seconds() const { return std::max((m_absoluteTimeout -
MonotonicTime::now()), 0_s ); }

Let's remove this operator, or give it an explicit name like
"remainingSeconds()" or "secondsUntilDeadline()".

When an automatic type conversion also changes the meaning of the data, it
reduces type safety.

In this case, the change in meaning is also ambiguous because a timeout could
tell you (a) how many seconds it was originally scheduled for (b) how many
seconds it has left or (c) a relative to the epoch or (d) b relative to the
epoch.

> Source/WebKit/Platform/IPC/Timeout.h:52
> +    constexpr operator MonotonicTime() const { return m_absoluteTimeout; }
> +    operator TimeWithDynamicClockType() const { return m_absoluteTimeout; }

Can we replace these type conversions with a "MonotonicTime deadline() const {
return m_abosluteTimeout; }"? Again, better to be explicit about what time
value we're retrieving.

> Source/WebKit/Platform/IPC/Timeout.h:53
> +    bool hasPassed() const { return MonotonicTime::now() >=
m_absoluteTimeout; }

I would call this didTimeOut().

> Source/WebKit/Platform/IPC/Timeout.h:55
> +    MonotonicTime m_absoluteTimeout;

I would call this m_deadline.