Bug 164992 – Migrate some layout timer-related code from std::chrono to Seconds and MonotonicTime

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Migrate some layout timer-related code from std::chrono to Seconds and MonotonicTime

[Simon Fraser (smfr)]

Created attachment 295267 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 295269 [details]
Patch

[Darin Adler]

Comment on attachment 295269 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2779
> +        return Seconds::zero();

Can this just use 0?

> Source/WebCore/dom/Document.cpp:2785
> +    return std::max<Seconds>(Seconds::zero(), settings()->layoutInterval() - elapsed);

Can this just use 0?

> Source/WebCore/page/ChromeClient.h:236
> +    virtual Seconds eventThrottlingDelay() { return Seconds::zero(); };

Can this just use 0?

> Source/WebCore/page/FrameView.cpp:2904
> +    Seconds delay = frame().document()->minimumLayoutDelay();

I normally prefer auto for something like this.

> Source/WebCore/page/FrameView.cpp:2911
> +    m_delayedLayout = delay > Seconds::zero();

Seems strange to have to say "> 0" for this. Is there some other way?

> Source/WebCore/page/FrameView.cpp:2948
> +        Seconds delay = renderView.document().minimumLayoutDelay();

We should use auto. If we had used auto before then we wouldn’t have to touch this line of code.

> Source/WebKit/mac/WebView/WebPreferencesPrivate.h:343
>  - (void)_setLayoutInterval:(int)l;
> -- (int)_layoutInterval;
> +- (int)_layoutInterval; // Milliseonds.

Both lines need this, I think.

[Simon Fraser (smfr)]

(In reply to comment #3)
> Comment on attachment 295269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295269&action=review
> 
> > Source/WebCore/dom/Document.cpp:2779
> > +        return Seconds::zero();
> 
> Can this just use 0?

I wish; it would reduce the number of changes. Phil, can we make this possible?

> > Source/WebCore/page/FrameView.cpp:2911
> > +    m_delayedLayout = delay > Seconds::zero();
> 
> Seems strange to have to say "> 0" for this. Is there some other way?

Ditto.

[Simon Fraser (smfr)]

I guess we'd need a non-explicit constructor?

[Filip Pizlo]

(In reply to comment #5)
> I guess we'd need a non-explicit constructor?

We could add such a constructor but my thinking was that we want to be explicit about type anytime a raw double becomes Seconds, to minimize the likelihood of someone losing track of units. Do you think this is an OK trade off?

We could also add support for liberals. This could lead to shorter syntax when saying zero.

[Simon Fraser (smfr)]

(In reply to comment #6)
> (In reply to comment #5)
> > I guess we'd need a non-explicit constructor?
> 
> We could add such a constructor but my thinking was that we want to be
> explicit about type anytime a raw double becomes Seconds, to minimize the
> likelihood of someone losing track of units. Do you think this is an OK
> trade off?

I do think this is OK. A raw 'double' time is almost always seconds, e.g. from currentTime() or currentMonotonicTime().

> We could also add support for liberals. This could lead to shorter syntax
> when saying zero.

Literals? _s and _ms would be nice.

[Simon Fraser (smfr)]

Also, I have a huge patch to convert timers to use Seconds, so would like to decide on something soon :)

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > I guess we'd need a non-explicit constructor?
> > 
> > We could add such a constructor but my thinking was that we want to be
> > explicit about type anytime a raw double becomes Seconds, to minimize the
> > likelihood of someone losing track of units. Do you think this is an OK
> > trade off?
> 
> I do think this is OK. A raw 'double' time is almost always seconds, e.g.
> from currentTime() or currentMonotonicTime().
> 
> > We could also add support for liberals. This could lead to shorter syntax
> > when saying zero.
> 
> Literals? _s and _ms would be nice.

If you had literals would you still need the implicit cast?  I like to avoid the implicit constructor, but it's not a big deal to me. 

I'm fine with you making a call on this and adding what you need. Either implicit casts or literals or both would be ok with me. Happy to review such a patch.

[Simon Fraser (smfr)]

Coming right up: https://bugs.webkit.org/show_bug.cgi?id=165074

[Simon Fraser (smfr)]

https://trac.webkit.org/r208982

[Csaba Osztrogonác]

(In reply to comment #11)
> https://trac.webkit.org/r208982

It broke the iOS build, see build.webkit.org for details.

[Simon Fraser (smfr)]

(In reply to comment #12)
> (In reply to comment #11)
> > https://trac.webkit.org/r208982
> 
> It broke the iOS build, see build.webkit.org for details.

Attempted fix in https://trac.webkit.org/changeset/208986