Bug 170656 - Drop Timer::startRepeating() overload taking a double
Summary: Drop Timer::startRepeating() overload taking a double
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-09 10:12 PDT by Chris Dumez
Modified: 2017-04-10 22:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (36.73 KB, patch)
2017-04-09 10:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (36.75 KB, patch)
2017-04-09 10:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.35 KB, patch)
2017-04-09 10:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.24 KB, patch)
2017-04-09 14:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2017-04-09 14:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2017-04-09 15:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2017-04-09 15:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (37.21 KB, patch)
2017-04-09 15:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Follow-up fix (1.10 KB, patch)
2017-04-10 19:38 PDT, Chris Dumez
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>