Bug 170656

Summary: Drop Timer::startRepeating() overload taking a double
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, darin, rniwa, sam, simon.fraser, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=170659
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Follow-up fix ysuzuki: review+

Description Chris Dumez 2017-04-09 10:12:02 PDT 
Drop Timer::startRepeating() overload taking a double as people should use Seconds type now.
Comment 1 Chris Dumez 2017-04-09 10:14:29 PDT 
Created attachment 306624 [details]
Patch
Comment 2 Chris Dumez 2017-04-09 10:29:07 PDT 
Created attachment 306625 [details]
Patch
Comment 3 Chris Dumez 2017-04-09 10:51:58 PDT 
Created attachment 306626 [details]
Patch
Comment 4 Yusuke Suzuki 2017-04-09 14:11:43 PDT 
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.
Comment 5 Simon Fraser (smfr) 2017-04-09 14:34:01 PDT 
Comment on attachment 306626 [details]
Patch

I agree with the comments.
Comment 6 Chris Dumez 2017-04-09 14:35:16 PDT 
Created attachment 306631 [details]
Patch
Comment 7 Chris Dumez 2017-04-09 14:35:56 PDT 
Yes, much nicer indeed. Thank you.
Comment 8 Chris Dumez 2017-04-09 14:51:05 PDT 
Created attachment 306632 [details]
Patch
Comment 9 Chris Dumez 2017-04-09 15:22:26 PDT 
Created attachment 306633 [details]
Patch
Comment 10 Yusuke Suzuki 2017-04-09 15:44:35 PDT 
I think annotating Seconds' members & operators with constexpr will solve the compiling issue.
Comment 11 Chris Dumez 2017-04-09 15:49:35 PDT 
Created attachment 306634 [details]
Patch
Comment 12 Chris Dumez 2017-04-09 15:58:57 PDT 
Created attachment 306635 [details]
Patch
Comment 13 Chris Dumez 2017-04-09 16:42:07 PDT 
Comment on attachment 306635 [details]
Patch

Clearing flags on attachment: 306635

Committed r215167: <http://trac.webkit.org/changeset/215167>
Comment 14 Chris Dumez 2017-04-09 16:42:09 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Adler 2017-04-10 18:35:50 PDT 
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.
Comment 16 Chris Dumez 2017-04-10 19:38:00 PDT 
Created attachment 306769 [details]
Follow-up fix
Comment 17 Chris Dumez 2017-04-10 19:38:22 PDT 
(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.
Comment 18 Chris Dumez 2017-04-10 19:46:58 PDT 
Reopen to fix bad change.
Comment 19 Yusuke Suzuki 2017-04-10 21:30:47 PDT 
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.
Comment 20 Chris Dumez 2017-04-10 22:43:13 PDT 
(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.
Comment 21 Chris Dumez 2017-04-10 22:46:37 PDT 
Committed r215218: <http://trac.webkit.org/changeset/215218>