Migrate some layout timer-related code from std::chrono to Seconds and MonotonicTime
Created attachment 295267 [details] Patch
Created attachment 295269 [details] Patch
Comment on attachment 295269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295269&action=review > Source/WebCore/dom/Document.cpp:2779 > + return Seconds::zero(); Can this just use 0? > Source/WebCore/dom/Document.cpp:2785 > + return std::max<Seconds>(Seconds::zero(), settings()->layoutInterval() - elapsed); Can this just use 0? > Source/WebCore/page/ChromeClient.h:236 > + virtual Seconds eventThrottlingDelay() { return Seconds::zero(); }; Can this just use 0? > Source/WebCore/page/FrameView.cpp:2904 > + Seconds delay = frame().document()->minimumLayoutDelay(); I normally prefer auto for something like this. > Source/WebCore/page/FrameView.cpp:2911 > + m_delayedLayout = delay > Seconds::zero(); Seems strange to have to say "> 0" for this. Is there some other way? > Source/WebCore/page/FrameView.cpp:2948 > + Seconds delay = renderView.document().minimumLayoutDelay(); We should use auto. If we had used auto before then we wouldn’t have to touch this line of code. > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:343 > - (void)_setLayoutInterval:(int)l; > -- (int)_layoutInterval; > +- (int)_layoutInterval; // Milliseonds. Both lines need this, I think.
(In reply to comment #3) > Comment on attachment 295269 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295269&action=review > > > Source/WebCore/dom/Document.cpp:2779 > > + return Seconds::zero(); > > Can this just use 0? I wish; it would reduce the number of changes. Phil, can we make this possible? > > Source/WebCore/page/FrameView.cpp:2911 > > + m_delayedLayout = delay > Seconds::zero(); > > Seems strange to have to say "> 0" for this. Is there some other way? Ditto.
I guess we'd need a non-explicit constructor?
(In reply to comment #5) > I guess we'd need a non-explicit constructor? We could add such a constructor but my thinking was that we want to be explicit about type anytime a raw double becomes Seconds, to minimize the likelihood of someone losing track of units. Do you think this is an OK trade off? We could also add support for liberals. This could lead to shorter syntax when saying zero.
(In reply to comment #6) > (In reply to comment #5) > > I guess we'd need a non-explicit constructor? > > We could add such a constructor but my thinking was that we want to be > explicit about type anytime a raw double becomes Seconds, to minimize the > likelihood of someone losing track of units. Do you think this is an OK > trade off? I do think this is OK. A raw 'double' time is almost always seconds, e.g. from currentTime() or currentMonotonicTime(). > We could also add support for liberals. This could lead to shorter syntax > when saying zero. Literals? _s and _ms would be nice.
Also, I have a huge patch to convert timers to use Seconds, so would like to decide on something soon :)
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > I guess we'd need a non-explicit constructor? > > > > We could add such a constructor but my thinking was that we want to be > > explicit about type anytime a raw double becomes Seconds, to minimize the > > likelihood of someone losing track of units. Do you think this is an OK > > trade off? > > I do think this is OK. A raw 'double' time is almost always seconds, e.g. > from currentTime() or currentMonotonicTime(). > > > We could also add support for liberals. This could lead to shorter syntax > > when saying zero. > > Literals? _s and _ms would be nice. If you had literals would you still need the implicit cast? I like to avoid the implicit constructor, but it's not a big deal to me. I'm fine with you making a call on this and adding what you need. Either implicit casts or literals or both would be ok with me. Happy to review such a patch.
Coming right up: https://bugs.webkit.org/show_bug.cgi?id=165074
https://trac.webkit.org/r208982
(In reply to comment #11) > https://trac.webkit.org/r208982 It broke the iOS build, see build.webkit.org for details.
(In reply to comment #12) > (In reply to comment #11) > > https://trac.webkit.org/r208982 > > It broke the iOS build, see build.webkit.org for details. Attempted fix in https://trac.webkit.org/changeset/208986