Bug 194135 – Use deferrable timer to restart the Responsiveness Timer on each wheel event

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

[Benjamin Poulain]

Use deferrable timer to restart the Responsiveness on each wheel event

[Benjamin Poulain]

Created attachment 360824 [details]
Patch

[Benjamin Poulain]

@Simon: that was a really good idea. I could not use Deferrable timers directly but a simple variant makes everything work.

I'll test tomorrow that everything still works as expected. The perf looks great though.

[Benjamin Poulain]

Created attachment 360922 [details]
Patch

[Benjamin Poulain]

Ok, this time I verified responsiveness still works as expected.

I found more cases to improve during testing. I'll do a follow up patch since it's a bit unrelated with this optimization.

[Simon Fraser (smfr)]

Comment on attachment 360922 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use deferrable timer to restart the Responsiveness on each wheel event

"the Responsiveness"?

> Source/WebCore/ChangeLog:17
> +        created a real DeferrableOneShotTimer that support dealines.

deadlines

> Source/WebCore/platform/Timer.cpp:554
> +void DeferrableOneShotTimer::stop()
> +{
> +    TimerBase::stop();
> +}

Can you just remove this?

> Source/WebCore/platform/Timer.h:204
> +class WEBCORE_EXPORT DeferrableOneShotTimer : private TimerBase {

I think you need some comments explaining the difference between DeferrableOneShotTimer and ResettableOneShotTimer. How are devs supposed to know which one to use?

[EWS Watchlist]

Comment on attachment 360922 [details]
Patch

Attachment 360922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11001806

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

[EWS Watchlist]

Created attachment 360944 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Benjamin Poulain]

Created attachment 360946 [details]
Patch

[Benjamin Poulain]

I added some documentation.

Tell me what you think.

> > Source/WebCore/platform/Timer.cpp:554
> > +void DeferrableOneShotTimer::stop()
> > +{
> > +    TimerBase::stop();
> > +}
> 
> Can you just remove this?

Unfortunately no. I can't inherit TimerBase publicly since I can't expose the other methods.

[Simon Fraser (smfr)]

Comment on attachment 360946 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        The original DeferrableOneShotTimer was not really deferrable.
> +        What it allows is to restart the count down from scratch after
> +        firing.

That actually surprised me. I wonder if we actually need both types.

DeferrableOneShotTimer came in via https://trac.webkit.org/changeset/118942/webkit.

[Benjamin Poulain]

(In reply to Simon Fraser (smfr) from comment #10)
> That actually surprised me. I wonder if we actually need both types.

Me too. It seems like a very odd way to use a timer...

I can try a follow up where I get rid of ResettableOneShotTimer but that seems a bit risky given the variety of uses :(

[Darin Adler]

If we feel that DeferrableOneShotTimer is not a good name for the class that currently has that name, I think we should rename for clarity, even if we later decide to get rid of it for economy.

[Benjamin Poulain]

Comment on attachment 360946 [details]
Patch

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 360946 [details]
Patch

Clearing flags on attachment: 360946

Committed r240944: <https://trac.webkit.org/changeset/240944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.