[webkit-reviews] review requested: [Bug 91502] Avoid repainting in RenderLayer::scrollTo where possible. : [Attachment 152799] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 91502: Avoid repainting in RenderLayer::scrollTo where possible.
https://bugs.webkit.org/show_bug.cgi?id=91502

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #2)
> (From update of attachment 152764 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=152764&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1669
> > +	 const bool movedNonCompositedLayer =
updateLayerPositionsAfterScroll();
> 
> const bool makes no sense as a return value.
It's just a bool now.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1701
> > +	 for (RenderObject* child = renderer()->firstChild(); child; child =
child->nextSibling())
> > +	     hasNonLayerChildObject |= !child->hasLayer();
> 
> I don't understand why you're checking for non-layer children.
I hope to skip repainting if the only things that move are composited layers.
The return value from updateLayerPositionsAfterScroll() just tells us if any
of the layers we moved weren't composited. We also need to know if we could
have
moved something that isn't a layer at all. If a non-layer changes position due
to the scroll, compositing won't save us; we really do need to repaint it in
the
new location (and repaint the old location). We could potentially be clever and
only paint the affected regions, but I decided to stick with the old logic and
repaint everything if any repainting might be required.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1705
> > +	 const bool needsFullRepaint = movedNonCompositedLayer ||
hasNonLayerChildObject;
> 
> We don't tent to use const for local variables.
Removed the const.