[webkit-reviews] review denied: [Bug 100524] Allow painting outside overflow clip in accelerated scrolling layers : [Attachment 172578] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Sami Kyöstilä
<skyostil at chromium.org>'s request for review:
Bug 100524: Allow painting outside overflow clip in accelerated scrolling
layers
https://bugs.webkit.org/show_bug.cgi?id=100524

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

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


> Source/WebCore/rendering/RenderBlock.cpp:2757
> +    if (!isRoot() && !usesCompositedScrolling()) {

This seems a bit heavy-handed. Maybe it would be better to provide a variant of
visualOverflowRect() that checks for composited scrolling? This is also hot
code so beware of perf regressions.

> Source/WebCore/rendering/RenderBox.cpp:622
> +    return hasOverflowClip() && layer()->usesCompositedScrolling();

There are some cases where hasOverflowClip() does not guarantee that you have a
layer(), so you should check hasLayer() too.

> Source/WebCore/rendering/RenderBox.cpp:662
>      paintRect.move(-scrolledContentOffset()); // For
overflow:auto/scroll/hidden.
>  
> +    // Do not clip scroll layer contents to reduce the number of repaints
while scrolling.
> +    if (usesCompositedScrolling())
> +	   return;

Why bother to move the paintRect if you're going to return?