[webkit-reviews] review granted: [Bug 57202] Allow setting composited backing stores for scrollbars and scroll corners : [Attachment 89331] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted James Robinson
<jamesr at chromium.org>'s request for review:
Bug 57202: Allow setting composited backing stores for scrollbars and scroll
corners
https://bugs.webkit.org/show_bug.cgi?id=57202

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

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

r=me but please fix the clipLayer geometry issue. You could add a testcase or
two where composited iframe contents potentially overlap the iframe scrollbars.


> Source/WebCore/rendering/RenderLayer.cpp:3631
> +}
> +GraphicsLayer* RenderLayer::layerForScrollCorner() const

Missing blank line.

> Source/WebCore/rendering/RenderLayerBacking.h:159
> +    bool requiresHorizontalScrollbarLayer();
> +    bool requiresVerticalScrollbarLayer();
> +    bool requiresScrollCornerLayer();

These should be const.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1144
> -	   m_clipLayer->setSize(FloatSize(frameView->layoutWidth(),
frameView->layoutHeight()));
> +	   m_clipLayer->setSize(frameView->visibleContentRect(true /* include
scrollbars */).size());

This is wrong for non-overlay scrollbars. The clipping layer has to exclude the
scrollbar, otherwise composited content inside the iframe can overlap those
scrollbars.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1469
> +    if (view->platformWidget())
> +	   return false;
> +#if PLATFORM(MAC)
> +    if (!view->hasOverlayScrollbars())
> +	   return false;
> +#endif

Please wrap this up in a helper function, rather than repeating it 3 times.