[webkit-reviews] review granted: [Bug 132794] REGRESSION (iOS WebKit2): requestAnimationFrame fires more than once between layer tree commits : [Attachment 231999] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 132794: REGRESSION (iOS WebKit2): requestAnimationFrame fires more than
once between layer tree commits
https://bugs.webkit.org/show_bug.cgi?id=132794

Attachment 231999: patch
https://bugs.webkit.org/attachment.cgi?id=231999&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231999&action=review


> Source/WebCore/dom/ScriptedAnimationController.h:95
> +    virtual DisplayRefreshMonitorFactory* displayRefreshMonitorFactory()
const override;

Do we really need a factory class, or can we just ask it to create a
DisplayRefreshMonitor?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:42
> +    if (factory)
> +	   return factory->createDisplayRefreshMonitor(displayID);

It's a bit weird for the create() method to just turn around and use the
factory passed in.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2014?

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:63
> +    if (!client->m_displayIDIsSet)

Weird to access internal state.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:71
> +    if (!client->m_displayIDIsSet)

Ditto.

> Source/WebCore/platform/graphics/GraphicsLayerUpdater.cpp:86
> +DisplayRefreshMonitorFactory*
GraphicsLayerUpdater::displayRefreshMonitorFactory() const
> +{
> +    return m_client ? m_client->displayRefreshMonitorFactory() : nullptr;
> +}
> +#endif

Maybe push the 82DisplayRefreshMonitor onto the GraphicsLayerUpdater?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3937
> +DisplayRefreshMonitorFactory*
RenderLayerCompositor::displayRefreshMonitorFactory() const
> +{
> +    Frame& frame = m_renderView.frameView().frame();
> +    Page* page = frame.page();
> +    if (!page)
> +	   return nullptr;
> +
> +    return page->chrome().client().displayRefreshMonitorFactory();
> +}
> +

Can this go away?

> Source/WebCore/rendering/RenderLayerCompositor.h:392
> +    DisplayRefreshMonitorFactory* displayRefreshMonitorFactory() const;

Why does RLC have to be a factory?