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

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin.poulain at nokia.com> has asked  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 47844: Repaint only the invalidated area after scrolling
https://bugs.webkit.org/attachment.cgi?id=47844&action=review

------- Additional Comments from Benjamin Poulain <benjamin.poulain at nokia.com>
Updated the patch thanks to Dave Hyatt's comments:

(In reply to comment #62)
> (1) You don't really need a hash of RenderObjects on FrameView.  A count is
> sufficient.  Then you can just walk the m_positionedObjects list of the
> RenderView and if the RenderBox is fixed positioned, it's one you care about.

> (This also bypasses the need to do any transform checking, since you'll only
be
> in the RenderView list if your containing block was the RenderView).

I have changed the code to use the list contained in RenderBlock. 
I like that lot better than what I had done previously.
To access it from RenderView, I had to change: 
-    void insertPositionedObject(RenderBox*);
-    void removePositionedObject(RenderBox*);
+    virtual void insertPositionedObject(RenderBox*);
+    virtual void removePositionedObject(RenderBox*);
     void removePositionedObjects(RenderBlock*);
+    ListHashSet<RenderBox*>* positionedObjects() const { return
m_positionedObjects; }


> (2) Fixed positioned objects are always RenderBoxes, so you need to tighten
up
> the code here. You can rename your methods to refer to use FixedPositionedBox

> rather than FixedPositionedObject.
> (3) You've duplicated code in RenderObject::destroy() and
> RenderWidget::destroy().  If you add a helper function to RenderBox, e.g.,
> unregisterFixedPositionedBox, then RenderWidget could call it in its destroy
> method.  The code in RenderObject should be moved to RenderBox.

I have moved the (un)registration to 
RenderView::insertPositionedObject()
RenderView::removePositionedObject()

So the problem of the transformations disappears.


> (3) All the code in RenderObject::styleWillChange is misplaced, since only
> RenderBoxes can be fixed positioned.	You should move the code into
RenderBox.
> (Yes, some of the code in there was already misplaced, but by adding a big
new
> pile of code, you've made the problem worse. :)

Right. Fixed :)

> (4) I dislike "scrollContentsFastPath" when we know what we mean is blitting.

> scrollContentsWithBlit would be my suggestion, although I don't like my own
> choice of name much better.

I have not changed the name yet. I don't know about putting "blit" in the name
because we don't blit if we reach the threshold.
What about scrollContentsWithBlitIfPossible(), tryScrollContentsWithBlit()? ;)