[webkit-reviews] review denied: [Bug 55257] Support creating compositing layers for scrollable frames and iframes : [Attachment 85016] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Daniel Sievers
<sievers at google.com>'s request for review:
Bug 55257: Support creating compositing layers for scrollable frames and
iframes
https://bugs.webkit.org/show_bug.cgi?id=55257

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

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1259
> +bool
RenderLayerCompositor::requiresCompositingForScrollableFrame(RenderObject*
renderer) const
> +{
> +    if (!(m_compositingTriggers & ChromeClient::ScrollableFrameTrigger))
> +	   return false;
> +
> +    if (!renderer->isRenderView())

Given the phrasing works, I'd expect this to be called on the renderer in the
parent document (the RenderIFrame or whatever), and affect compositing in the
parent document. However, that's not what you're doing.

So I think it's wrong to add a requiresCompositingForScrollableFrame(), and
wasteful to call requiresCompositingForScrollableFrame() for every renderer in
the document when it would only ever apply to the root.

I think you should just do the root compositing check in one place.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1289
> +    return (view->scrollWidth() != view->clientWidth())
> +	   || (view->scrollHeight() != view->clientHeight());

What will cause compositing to be re-evaluated when the contents of the iframe
grow such that it becomes scrollable?