[webkit-reviews] review granted: [Bug 177484] requestAnimationFrame should execute before the next frame : [Attachment 363274] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 177484: requestAnimationFrame should execute before the next frame
https://bugs.webkit.org/show_bug.cgi?id=177484

Attachment 363274: Patch

https://bugs.webkit.org/attachment.cgi?id=363274&action=review




--- Comment #64 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 363274
  --> https://bugs.webkit.org/attachment.cgi?id=363274
Patch

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

This looks great, other than the renaming.

> Source/WTF/wtf/SystemTracing.h:81
> +    ScheduleUpdateRendering,
> +    TriggerUpdateRendering,
> +    UpdateRenderingStart,
> +    UpdateRenderingEnd,

These should use the term "RenderingUpdate"

> Source/WebCore/animation/DocumentTimeline.cpp:332
> +void DocumentTimeline::updateAnimationsAndSendEvents(double timestamp)

double -> DOMHighResTimeStamp ?

> Source/WebCore/animation/DocumentTimeline.cpp:338
> +	   cacheCurrentTime(Seconds(timestamp));

Would be better if cacheCurrentTime took a DOMHighResTimeStamp I think.

> Source/WebCore/dom/Document.cpp:6259
> +void Document::updateAnimationsAndSendEvents(double timestamp)

DOMHighResTimeStamp

> Source/WebCore/dom/Document.cpp:6265
> +void Document::serviceRequestAnimationFrameCallbacks(double timestamp)

DOMHighResTimeStamp

> Source/WebCore/dom/ScriptedAnimationController.cpp:191
> +void
ScriptedAnimationController::serviceRequestAnimationFrameCallbacks(double
timestamp)

DOMHighResTimeStamp

> Source/WebCore/dom/ScriptedAnimationController.cpp:216
> +	   if (callback->m_useLegacyTimeBase)

We should remove this legacy code now (in another patch)

> Source/WebCore/page/Page.cpp:1267
> +    // This function is not reentrant, e.g. a rAF callback may force
repaint.

The rAF-causing forced repaint is only an issue in our test harness, right?

> Source/WebCore/page/Page.cpp:1283
> +	   double timestamp = document->domWindow()->nowTimestamp();

DOMHighResTimeStamp

> Source/WebCore/page/Page.cpp:1292
> +	   document->updateIntersectionObservations();

Does this not take a timestamp? I think it should.

> Source/WebCore/page/Page.h:272
> +    UpdateRenderingScheduler& updateRenderingScheduler();

I think "renderingUpdateScheduler" is better.

> Source/WebCore/page/Page.h:870
> +    std::unique_ptr<UpdateRenderingScheduler> m_updateRenderingScheduler;

m_renderingUpdateScheduler

> Source/WebCore/page/Page.h:972
> +    bool m_inUpdateRendering { false };

m_inRenderingUpdate

> Source/WebCore/page/UpdateRenderingScheduler.h:36
> +class UpdateRenderingScheduler

Let's call this RenderingUpdateScheduler

> Source/WebCore/page/UpdateRenderingScheduler.h:48
> +    UpdateRenderingScheduler(Page&);

Make private.

> Source/WebCore/page/UpdateRenderingScheduler.h:49
> +    void scheduleUpdateRendering();

scheduleRenderingUpdate

> Source/WebCore/page/UpdateRenderingScheduler.h:68
> +    bool m_updateRenderingScheduled { false };

m_scheduled is probably enough.

> Tools/Tracing/SystemTracePoints.plist:209
> +		    <string>Schedule display refresh</string>

Schedule rendering update

> Tools/Tracing/SystemTracePoints.plist:219
> +		    <string>Trigger display refresh</string>

Trigger rendering update