[webkit-reviews] review granted: [Bug 204713] Throttling requestAnimationFrame should be controlled by RenderingUpdateScheduler : [Attachment 384597] 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 204713: Throttling requestAnimationFrame should be controlled by
RenderingUpdateScheduler
https://bugs.webkit.org/show_bug.cgi?id=204713

Attachment 384597: Patch

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




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

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

> Source/WebCore/dom/ScriptedAnimationController.h:54
> +    int registerCallback(Ref<RequestAnimationFrameCallback>&&);

Why did the typedef go away?

> Source/WebCore/dom/ScriptedAnimationController.h:63
> -    enum class ThrottlingReason {
> -	   VisuallyIdle 		   = 1 << 0,
> -	   OutsideViewport		   = 1 << 1,
> -	   LowPowerMode 		   = 1 << 2,
> -	   NonInteractedCrossOriginFrame   = 1 << 3,
> -    };
> -    void addThrottlingReason(ThrottlingReason);
> -    void removeThrottlingReason(ThrottlingReason);
> -
> -    WEBCORE_EXPORT bool isThrottled() const;
> -    WEBCORE_EXPORT Seconds interval() const;
> +    
> +    void setOutsideViewport(bool outsideViewport) { m_outsideViewport =
outsideViewport; }
> +    void setNonInteractedCrossOriginFrame(bool
nonInteractedCrossOriginFrame) { m_nonInteractedCrossOriginFrame =
nonInteractedCrossOriginFrame; }
> +    bool isThrottled() const { return interval() >
fullSpeedAnimationInterval; }

I prefer the old version, which should use an OptionSet<> of reasons.

> Source/WebCore/page/Page.h:714
> +    Seconds preferredRenderingUpdateInterval() const;
> +    unsigned preferredFramesPerSecond() const;

It's really confusing to have both of these.

> Source/WebCore/page/RenderingUpdateScheduler.cpp:73
> +    if (interval > 1_s ||
!DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this,
m_page.preferredFramesPerSecond()))

Why the 1s cutoff here?

> Source/WebCore/page/RenderingUpdateScheduler.h:50
> +    void updateRenderingUpdateRate();

Let's call this adjustRenderingUpdateFrequency()

> Source/WebCore/page/RenderingUpdateScheduler.h:58
> +    void updatePreferredFramesPerSecond();

Confusing to have this and updateRenderingUpdateRate()

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h:48
> +    void setPreferredFramesPerSecond(DisplayRefreshMonitorClient&,
unsigned);
> +    bool scheduleAnimation(DisplayRefreshMonitorClient&, unsigned =
MaxFramesPerSecond);

That unsigned arg needs to have a name. And it's odd to have both
setPreferredFramesPerSecond and this argument to scheduleAnimation().