Port DOMTimer from std::chrono::milliseconds to WTF::Seconds type.
Created attachment 303581 [details] Patch
Created attachment 303583 [details] Patch
See bug 165079
(In reply to comment #3) > See bug 165079 Hmm, I wasn't aware of this change. Sad that it has not landed since last year. How do you want to proceed? My patch should be building now and has more limited scope apparently.
Created attachment 303586 [details] Patch
Should take care of the debug build.
Comment on attachment 303583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303583&action=review > Source/WebCore/page/DOMTimer.cpp:56 > +static const Seconds maxIntervalForUserGestureForwarding { 1 }; // One second matches Gecko. > +static const Seconds minIntervalForNonUserObservableChangeTimers { 1 }; // Empirically determined to maximize battery life. 1_s > Source/WebCore/page/DOMTimer.cpp:417 > + Seconds interval = std::max(Seconds::fromMilliseconds(1), m_originalInterval); 1_ms > Source/WebCore/page/DOMTimer.cpp:441 > + auto randomizedOffsetMS = static_cast<long long>(alignmentIntervalMS * randomizedProportion); > + auto adjustedFireTimeMS = static_cast<long long>(fireTime.milliseconds()) - randomizedOffsetMS; Are you keeping these as long long explicitly for discretization? I used MonotonicTime for the adjustedFireTime. > Source/WebCore/page/DOMTimer.h:48 > + static Seconds defaultMinimumInterval() { return Seconds::fromMilliseconds(4); } 4_ms > Source/WebCore/page/DOMTimer.h:50 > + static Seconds hiddenPageAlignmentInterval() { return Seconds { 1 }; } 1_s > Source/WebCore/page/Page.cpp:1364 > + m_domTimerAlignmentInterval = Seconds { monotonicallyIncreasingTime() - m_timerThrottlingStateLastChangedTime }; Use MonotonicTime, not monotonicallyIncreasingTime() > Source/WebCore/page/Page.h:718 > + Seconds m_domTimerAlignmentIntervalIncreaseLimit { 0 }; No need to initialize a Seconds. > Source/WebCore/platform/Timer.cpp:385 > + if (auto newAlignedTime = alignedFireTime(Seconds { newTime })) Are we doing Seconds { newTime } now, instead of just Seconds(newTime)? > Source/WebCore/testing/Internals.cpp:1039 > + return !!timer->alignedFireTime(Seconds { 0 }); 0_s
Comment on attachment 303583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303583&action=review >> Source/WebCore/page/DOMTimer.cpp:56 >> +static const Seconds minIntervalForNonUserObservableChangeTimers { 1 }; // Empirically determined to maximize battery life. > > 1_s Oh, I did not know about these suffixes. I'll use them everywhere. >> Source/WebCore/page/DOMTimer.cpp:441 >> + auto adjustedFireTimeMS = static_cast<long long>(fireTime.milliseconds()) - randomizedOffsetMS; > > Are you keeping these as long long explicitly for discretization? I used MonotonicTime for the adjustedFireTime. I was trying to keep the code as close to the existing one as possible because it is a bit complicated and I worry about introducing bugs. For example, the code used to deal with milliseconds and I would need to look into it more to make sure it does the same thing with seconds. >> Source/WebCore/page/Page.cpp:1364 >> + m_domTimerAlignmentInterval = Seconds { monotonicallyIncreasingTime() - m_timerThrottlingStateLastChangedTime }; > > Use MonotonicTime, not monotonicallyIncreasingTime() I did not know about MonotonicTime, I'll use it here. >> Source/WebCore/page/Page.h:718 >> + Seconds m_domTimerAlignmentIntervalIncreaseLimit { 0 }; > > No need to initialize a Seconds. Ok. >> Source/WebCore/platform/Timer.cpp:385 >> + if (auto newAlignedTime = alignedFireTime(Seconds { newTime })) > > Are we doing Seconds { newTime } now, instead of just Seconds(newTime)? Darin coding style :)
Created attachment 303606 [details] Patch
Created attachment 303655 [details] Patch
Comment on attachment 303655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303655&action=review > Source/WebCore/page/DOMTimer.cpp:404 > + LOG(DOMTimers, "%p - Updating DOMTimer's repeat interval from %f ms to %f ms due to throttling.", this, previousInterval.milliseconds(), m_currentTimerInterval.milliseconds()); Maybe %.2f in these logs for readability. > Source/WebCore/page/DOMTimer.cpp:442 > + Seconds randomizedOffset = alignmentInterval * randomizedProportion; > + Seconds adjustedFireTime = fireTime - randomizedOffset; > return adjustedFireTime - (adjustedFireTime % alignmentInterval) + alignmentInterval + randomizedOffset; Did you do some runtime logging/debugging to make sure that this code still results in good alignment?
(In reply to comment #11) > Comment on attachment 303655 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303655&action=review > > > Source/WebCore/page/DOMTimer.cpp:404 > > + LOG(DOMTimers, "%p - Updating DOMTimer's repeat interval from %f ms to %f ms due to throttling.", this, previousInterval.milliseconds(), m_currentTimerInterval.milliseconds()); > > Maybe %.2f in these logs for readability. Ok. > > > Source/WebCore/page/DOMTimer.cpp:442 > > + Seconds randomizedOffset = alignmentInterval * randomizedProportion; > > + Seconds adjustedFireTime = fireTime - randomizedOffset; > > return adjustedFireTime - (adjustedFireTime % alignmentInterval) + alignmentInterval + randomizedOffset; > > Did you do some runtime logging/debugging to make sure that this code still > results in good alignment? Yes.
Created attachment 303658 [details] Patch
Comment on attachment 303658 [details] Patch Clearing flags on attachment: 303658 Committed r213519: <http://trac.webkit.org/changeset/213519>
All reviewed patches have been landed. Closing bug.