[webkit-reviews] review denied: [Bug 36783] Update of fixed elements is not made correctly when the page has been scrolled : [Attachment 53217] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Benjamin Poulain
<benjamin.poulain at nokia.com>'s request for review:
Bug 36783: Update of fixed elements is not made correctly when the page has
been scrolled
https://bugs.webkit.org/show_bug.cgi?id=36783

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
> index 4b6cc36..badca3b 100644
> --- a/WebCore/page/FrameView.cpp
> +++ b/WebCore/page/FrameView.cpp
> @@ -1011,6 +1011,7 @@ void FrameView::scrollPositionChanged()
>      if (!m_nestedLayoutCount && hasFixedObjects()) {
>	   if (RenderView* root = m_frame->contentRenderer()) {
>	       root->updateWidgetPositions();
> +	       root->updateFixedElementPositions();
>  #if USE(ACCELERATED_COMPOSITING)
>	       if (root->usesCompositing())
>		  
root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);

Calling updateFixedElementPositions() here may in fact obsolete the need to
call updateCompositingLayers().

> diff --git a/WebCore/rendering/RenderView.cpp
b/WebCore/rendering/RenderView.cpp

> +void RenderView::updateFixedElementPositions()

The method name doesn't accurately reflect what the method does. It's fixing up
cached repaint rects, not updating element positions.

Maybe updateLayersAfterScroll().

> +{
> +    ListHashSet<RenderBox*>::const_iterator end =
positionedObjects()->end();
> +    for (ListHashSet<RenderBox*>::const_iterator it =
positionedObjects()->begin(); it != end; ++it) {
> +	   RenderBox* renderBox = *it;
> +	   if (renderBox->style()->position() != FixedPosition)
> +	       continue;
> +	   renderBox->layer()->updateLayerPositions();

You should look at the UpdateLayerPositionsFlags argument to
updateLayerPositions(), and use the appropriate flags. For example, I don't
think you need the 'full repaint' flag.

You may also do redundant work here if there are nested fixed position
elements.

I'd like to see a final version of this patch.