Bug 170656 – Drop Timer::startRepeating() overload taking a double

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Drop Timer::startRepeating() overload taking a double as people should use Seconds type now.

[Chris Dumez]

Created attachment 306624 [details]
Patch

[Chris Dumez]

Created attachment 306625 [details]
Patch

[Chris Dumez]

Created attachment 306626 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 306626 [details]
Patch

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

r=me with nits

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:91
> +        m_requestFrameTimer.startRepeating(Seconds { 1. / m_frameRequestRate.value() });

I like `1_s / m_frameRequestRate.value()` rather.

> Source/WebCore/page/Frame.cpp:124
> +const Seconds scrollFrequency { 1000 / 60. };

Ditto. I think `1000_s / 60` is more descriptive.

> Source/WebCore/page/WheelEventTestTrigger.cpp:63
> +        m_testTriggerTimer.startRepeating(Seconds { 1. / 60 });

Ditto.

> Source/WebCore/platform/cocoa/ScrollController.mm:389
> +    m_snapRubberbandTimer.startRepeating(Seconds { 1. / 60 });

Ditto.

> Source/WebCore/platform/cocoa/ScrollController.mm:565
> +    m_scrollSnapTimer.startRepeating(Seconds { 1. / 60 });

Ditto.

> Source/WebCore/rendering/RenderMarquee.cpp:171
> +    m_timer.startRepeating(Seconds { speed() * 0.001 });

I think `1_ms * speed()` is better.

> Source/WebCore/rendering/RenderMarquee.cpp:233
> +            m_timer.startRepeating(Seconds { speed() * 0.001 });

Ditto.

[Simon Fraser (smfr)]

Comment on attachment 306626 [details]
Patch

I agree with the comments.

[Chris Dumez]

Created attachment 306631 [details]
Patch

[Chris Dumez]

Yes, much nicer indeed. Thank you.

[Chris Dumez]

Created attachment 306632 [details]
Patch

[Chris Dumez]

Created attachment 306633 [details]
Patch

[Yusuke Suzuki]

I think annotating Seconds' members & operators with constexpr will solve the compiling issue.

[Chris Dumez]

Created attachment 306634 [details]
Patch

[Chris Dumez]

Created attachment 306635 [details]
Patch

[Chris Dumez]

Comment on attachment 306635 [details]
Patch

Clearing flags on attachment: 306635

Committed r215167: <http://trac.webkit.org/changeset/215167>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 306635 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeGtk.cpp:1817
> -    return time / 2000.;
> +    return 2_ms * time;

That’s not right. It should be be 1/2 ms, not 2 ms.

[Chris Dumez]

Created attachment 306769 [details]
Follow-up fix

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Comment on attachment 306635 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306635&action=review
> 
> > Source/WebCore/rendering/RenderThemeGtk.cpp:1817
> > -    return time / 2000.;
> > +    return 2_ms * time;
> 
> That’s not right. It should be be 1/2 ms, not 2 ms.

Thank you, will fix.

[Chris Dumez]

Reopen to fix bad change.

[Yusuke Suzuki]

Comment on attachment 306769 [details]
Follow-up fix

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

r=me

> Source/WebCore/rendering/RenderThemeGtk.cpp:1817
> +    return 1_ms * time / 2.;

`500_ns * time` could be simpler, but the both are ok to me.

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #19)
> Comment on attachment 306769 [details]
> Follow-up fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306769&action=review
> 
> r=me
> 
> > Source/WebCore/rendering/RenderThemeGtk.cpp:1817
> > +    return 1_ms * time / 2.;
> 
> `500_ns * time` could be simpler, but the both are ok to me.

Assuming you mean 500_us * time.

[Chris Dumez]

Committed r215218: <http://trac.webkit.org/changeset/215218>