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

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied 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 47622: Repaint only the invalidated area after scrolling
https://bugs.webkit.org/attachment.cgi?id=47622&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
Some comments:

(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).

(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.

(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. :)

(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.