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

[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 53701: The patch with the images
https://bugs.webkit.org/attachment.cgi?id=53701&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> +	 <p style="position: absolute; top: 210px">You should see a yellow rect
on the left, and a red one on the right. No green pixels.</p>

You should change the colors so that red pixels indicate failure. Green is
good, red is bad.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +2010-04-19  Benjamin Poulain  <ikipou at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Update of fixed elements is not made correctly when the page has
been scrolled
> +	   https://bugs.webkit.org/show_bug.cgi?id=36783

This changelog entry needs to describe what the problem was, and how these
changes tbat you are making fix the bug.

> +	   * page/FrameView.cpp:
> +	   (WebCore::FrameView::scrollPositionChanged):
> +	   * rendering/RenderLayer.cpp:
> +	   (WebCore::RenderLayer::updateLayerPositions):
> +	   * rendering/RenderLayer.h:
> +	   (WebCore::RenderLayer::):
> +	   * rendering/RenderLayerCompositor.cpp:
> +	   (WebCore::RenderLayerCompositor::updateCompositingLayers):
> +	   * rendering/RenderLayerCompositor.h:
> +	   (WebCore::):

A good changelog entry will also annotate each of the file changes.

> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
> index 4b6cc36..60d0ce9 100644
> --- a/WebCore/page/FrameView.cpp
> +++ b/WebCore/page/FrameView.cpp
> @@ -1011,10 +1011,7 @@ void FrameView::scrollPositionChanged()
>      if (!m_nestedLayoutCount && hasFixedObjects()) {
>	   if (RenderView* root = m_frame->contentRenderer()) {
>	       root->updateWidgetPositions();
> -#if USE(ACCELERATED_COMPOSITING)
> -	       if (root->usesCompositing())
> -		  
root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);
> -#endif
> +	      
root->layer()->updateLayerPositions(RenderLayer::UpdateCompositingLayers |
RenderLayer::UpdateFixedSubtreeOnScroll);
>	   }

The updateCompositingLayers() is still required.We may need to re-analyze the
compositing hierarchy on scrolling, and updateLayerPositions() doesn't do this.


> diff --git a/WebCore/rendering/RenderLayer.cpp
b/WebCore/rendering/RenderLayer.cpp
> index 2d868bb..aff19df 100644
> --- a/WebCore/rendering/RenderLayer.cpp
> +++ b/WebCore/rendering/RenderLayer.cpp
> @@ -247,6 +247,20 @@ bool RenderLayer::hasAcceleratedCompositing() const
>  
>  void RenderLayer::updateLayerPositions(UpdateLayerPositionsFlags flags,
IntPoint* cachedOffset)
>  {
> +    if (flags & UpdateFixedSubtreeOnScroll) {
> +	   if (renderer()->style()->position() == FixedPosition)
> +	       flags &= ~UpdateFixedSubtreeOnScroll;
> +	   else if (hasTransform()) {
> +	       // transformation is a referencial for fixed child layers
> +	       // no need to update them
> +	       return;
> +	   } else {
> +	       for (RenderLayer* child = firstChild(); child; child =
child->nextSibling())
> +		   child->updateLayerPositions(flags, cachedOffset);
> +	       return;
> +	   }
> +    }

I don't really like how UpdateFixedSubtreeOnScroll causes early returns here.
It should work for this flag to be used in combination with the other flags,
but this code doesn't work that way.

I'm leaning towards having a new method just to update fixed layers. Sorry to
not be more specific earlier.


> diff --git a/WebCore/rendering/RenderLayerCompositor.cpp
b/WebCore/rendering/RenderLayerCompositor.cpp
> index 9430613..a8cf221 100644
> --- a/WebCore/rendering/RenderLayerCompositor.cpp
> +++ b/WebCore/rendering/RenderLayerCompositor.cpp
> @@ -165,12 +165,6 @@ void
RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
>      case CompositingUpdateOnPaitingOrHitTest:
>	   checkForHierarchyUpdate = true;
>	   break;
> -    case CompositingUpdateOnScroll:
> -	   if (m_compositingConsultsOverlap)
> -	       checkForHierarchyUpdate = true; // Overlap can change with
scrolling, so need to check for hierarchy updates.
> -
> -	   needGeometryUpdate = true;
> -	   break;
>      }
>  
>      if (!checkForHierarchyUpdate && !needGeometryUpdate)
> diff --git a/WebCore/rendering/RenderLayerCompositor.h
b/WebCore/rendering/RenderLayerCompositor.h
> index 4dd8712..173062c 100644
> --- a/WebCore/rendering/RenderLayerCompositor.h
> +++ b/WebCore/rendering/RenderLayerCompositor.h
> @@ -41,7 +41,6 @@ class RenderVideo;
>  enum CompositingUpdateType {
>      CompositingUpdateAfterLayoutOrStyleChange,
>      CompositingUpdateOnPaitingOrHitTest,
> -    CompositingUpdateOnScroll
>  };
>  
>  // RenderLayerCompositor manages the hierarchy of

You should revert these changes.