Bug 218874 - Removed DeferrableTaskTimer
Summary: Removed DeferrableTaskTimer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-12 15:33 PST by Geoffrey Garen
Modified: 2022-02-27 23:59 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.11 KB, patch)
2020-11-13 09:39 PST, Geoffrey Garen
cdumez: review+
Details | Formatted Diff | Diff
Patch for landing (8.45 KB, patch)
2020-11-13 13:30 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2020-11-12 15:33:53 PST
Removed DeferrableTaskTimer
Comment 1 Geoffrey Garen 2020-11-13 09:39:49 PST
Created attachment 414054 [details]
Patch
Comment 2 Chris Dumez 2020-11-13 10:59:29 PST
Comment on attachment 414054 [details]
Patch

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

> Source/WebCore/platform/Timer.h:112
> +    static void startOneShot(Seconds interval, WTF::Function<void()>&& function)

Maybe it's just me but I preferred the previous order: Lambda first and then the delay. BTW, seems weird to call this interval instead of delay given that it is one shot.

The reason for my preference is that it would be consistent with window.setTimeout(() => { }, 100);

Finally, I wish we could find a better name for this function than startOneShot() given that it is a static. Maybe something like Timer::doAfter() or runAfter() (similarly to dispatch_after()).

> Source/WebKit/ChangeLog:12
> +        * UIProcess/WebPageProxy.h: Use WTF::RunLoop::Timer instead of

Good catch.

> Source/WebKit/UIProcess/WebPageProxy.h:2758
> +    WTF::RunLoop::Timer<WebPageProxy> m_updateReportedMediaCaptureStateTimer;

Why WTF:: ?
Comment 3 Geoffrey Garen 2020-11-13 11:16:20 PST
> > Source/WebCore/platform/Timer.h:112
> > +    static void startOneShot(Seconds interval, WTF::Function<void()>&& function)
> 
> Maybe it's just me but I preferred the previous order: Lambda first and then
> the delay. BTW, seems weird to call this interval instead of delay given
> that it is one shot.
> 
> The reason for my preference is that it would be consistent with
> window.setTimeout(() => { }, 100);

For the order, my thinking was that Cocoa APIs have the habit of putting the lambda last, and the reasoning is that it reads better because a lambda is a disruptive amount of text that makes it hard to see what comes after.

What do you think of that?

> Finally, I wish we could find a better name for this function than
> startOneShot() given that it is a static. Maybe something like
> Timer::doAfter() or runAfter() (similarly to dispatch_after()).

How about schedule()?

> > Source/WebKit/UIProcess/WebPageProxy.h:2758
> > +    WTF::RunLoop::Timer<WebPageProxy> m_updateReportedMediaCaptureStateTimer;
> 
> Why WTF:: ?

Will fix.
Comment 4 Chris Dumez 2020-11-13 11:32:56 PST
(In reply to Geoffrey Garen from comment #3)
> > > Source/WebCore/platform/Timer.h:112
> > > +    static void startOneShot(Seconds interval, WTF::Function<void()>&& function)
> > 
> > Maybe it's just me but I preferred the previous order: Lambda first and then
> > the delay. BTW, seems weird to call this interval instead of delay given
> > that it is one shot.
> > 
> > The reason for my preference is that it would be consistent with
> > window.setTimeout(() => { }, 100);
> 
> For the order, my thinking was that Cocoa APIs have the habit of putting the
> lambda last, and the reasoning is that it reads better because a lambda is a
> disruptive amount of text that makes it hard to see what comes after.
> 
> What do you think of that?

Not a big deal either way. It is true that dispatch_after() takes delay first and lambda last.

> 
> > Finally, I wish we could find a better name for this function than
> > startOneShot() given that it is a static. Maybe something like
> > Timer::doAfter() or runAfter() (similarly to dispatch_after()).
> 
> How about schedule()?

Sure, schedule() would be better IMO.

> 
> > > Source/WebKit/UIProcess/WebPageProxy.h:2758
> > > +    WTF::RunLoop::Timer<WebPageProxy> m_updateReportedMediaCaptureStateTimer;
> > 
> > Why WTF:: ?
> 
> Will fix.
Comment 5 Geoffrey Garen 2020-11-13 13:30:33 PST
Created attachment 414087 [details]
Patch for landing
Comment 6 EWS 2020-11-13 14:39:17 PST
Committed r269802: <https://trac.webkit.org/changeset/269802>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414087 [details].
Comment 7 Radar WebKit Bug Importer 2020-11-13 14:40:22 PST
<rdar://problem/71386106>