[webkit-reviews] review canceled: [Bug 33150] Do not render the full frame when there is some elements with fixed positioning : [Attachment 45996] Repaint only the invalidated area after scrolling

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin.poulain at nokia.com> has canceled Benjamin Poulain
<benjamin.poulain at nokia.com>'s request for review:
Bug 33150: Do not render the full frame when there is some elements with fixed
positioning
https://bugs.webkit.org/show_bug.cgi?id=33150

Attachment 45996: Repaint only the invalidated area after scrolling
https://bugs.webkit.org/attachment.cgi?id=45996&action=review

------- Additional Comments from Benjamin Poulain <benjamin.poulain at nokia.com>
(In reply to comment #12)
> (From update of attachment 45996 [details])
> 
> > +void FrameView::fastScrollContents(const IntSize& scrollDelta, const
IntRect& rectToScroll, const IntRect& clipRect)
> 
> I dislike the method name. Shouldn't it always be fast? 

No, there are some cases when you cannot avoid the slow path. For example, if
the view is not opaque, you need full repaint.

> Does FrameView already
> have a scrollContents method?

Yep.

> > +	     Vector<RenderObject*> objectsFixedInViewport;
> 
> We say fixedObjects elsewhere, right?

Right :)
Fixed

> Instead of the updateInvalidSubRect, couldn't you clear the
> objectsFixedInViewport instead? and ask
> if (!objectsFixedInViewport.isEmpty()) below? I find that less mysterious.

This would not be correct. You can have updateInvalidSubRect == true and
objectsFixedInViewport.isEmpty() in the case where all fixed object are under a
transformed layer in the hierarchy.

> > +		     IntRect updateRect =
(objectsFixedInViewport[i])->paintingRootRect(topLevelRect);
> 
> Are the () really needed?

Oups. Fixed  :)

> > +		 // there are too many fixed objects, repaint everything might
be easier
> 
> // the number of fixed objects exceed the threshold, so we repaint
everything. 

Changed.


> >	 if (canBlitOnScroll() && !rootPreventsBlitting()) { // The main frame
can just blit the WebView window
> > -	    // FIXME: Find a way to blit subframes without blitting overlapping
content
> > -	    hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect);
> > +	     // FIXME: Find a way to blit subframes without blitting
overlapping content
> > +	     fastScrollContents(-scrollDelta, scrollViewRect, clipRect);
> 
> Wrong indentation

Is it? The coding convention says 4 spaces, not 3.