Delete RenderLayer::updateLayerPositionsAfterDocumentScroll

Delete RenderLayer::updateLayerPositionsAfterDocumentScroll

This CL deletes RenderLayer::updateLayerPositionsAfterDocumentScroll and
unwinds all the complexity it created. Now that we're always passing
IsOverflowScroll, we can remove all the other flags for deciding when to clear
the clip cache. Also, now we always start the walk at RenderLayers that would
trigger HasSeenAncestorWithOverflowClip, so we can just assume that flag is
set.  That means there are no flags left!

Also, finally delete RenderLayerClipper::clearClipRects. Now everyone clears
all the descendant clip rects too.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177144

[eseidel, 2014-06-27 20:36:25]

Lgtm

On Friday, June 27, 2014, <abarth@chromium.org> wrote:

> Reviewers: ,
>
> Description:
> Delete RenderLayer::updateLayerPositionsAfterDocumentScroll
>
> This CL deletes RenderLayer::updateLayerPositionsAfterDocumentScroll and
> unwinds all the complexity it created. Now that we're always passing
> IsOverflowScroll, we can remove all the other flags for deciding when to
> clear
> the clip cache. Also, now we always start the walk at RenderLayers that
> would
> trigger HasSeenAncestorWithOverflowClip, so we can just assume that flag
> is
> set.  That means there are no flags left!
>
> Also, finally delete RenderLayerClipper::clearClipRects. Now everyone
> clears
> all the descendant clip rects too.
>
> Please review this at https://codereview.chromium.org/331653004/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+11, -61 lines):
>   M Source/core/frame/FrameView.cpp
>   M Source/core/rendering/RenderLayer.h
>   M Source/core/rendering/RenderLayer.cpp
>   M Source/core/rendering/RenderLayerClipper.h
>   M Source/core/rendering/RenderLayerClipper.cpp
>
>
> Index: Source/core/frame/FrameView.cpp
> diff --git a/Source/core/frame/FrameView.cpp
> b/Source/core/frame/FrameView.cpp
> index 790532e1c83eeda15c0d20373b9c3ff6063eddcf..
> 3410bfce246a70beae4c21c393caadb42315efa9 100644
> --- a/Source/core/frame/FrameView.cpp
> +++ b/Source/core/frame/FrameView.cpp
> @@ -1675,10 +1675,8 @@ void FrameView::updateLayersAndCompositingAfte
> rScrollIfNeeded()
>      // layout.
>      if (!m_nestedLayoutCount) {
>          updateWidgetPositions();
> -        if (RenderView* renderView = this->renderView()) {
> -            renderView->layer()->updateLayerPositionsAfterDocum
> entScroll();
> +        if (RenderView* renderView = this->renderView())
>              renderView->layer()->setNeedsCompositingInputsUpdate();
> -        }
>      }
>  }
>
> Index: Source/core/rendering/RenderLayer.cpp
> diff --git a/Source/core/rendering/RenderLayer.cpp
> b/Source/core/rendering/RenderLayer.cpp
> index 6e1ffea8d4011717cdb3065935949011c8022b08..
> 387eeb6fc91fcea33edde843a5978416095567f2 100644
> --- a/Source/core/rendering/RenderLayer.cpp
> +++ b/Source/core/rendering/RenderLayer.cpp
> @@ -251,6 +251,8 @@ void RenderLayer::updateLayerPositionsAfterLayout(const
> RenderLayer* rootLayer,
>  {
>      TRACE_EVENT0("blink_rendering", "RenderLayer::
> updateLayerPositionsAfterLayout");
>
> +    m_clipper.clearClipRectsIncludingDescendants();
> +
>      // FIXME: Remove incremental compositing updates after fixing the
> chicken/egg issues
>      // https://code.google.com/p/chromium/issues/detail?id=343756
>      DisableCompositingQueryAsserts disabler;
> @@ -265,9 +267,6 @@ void
RenderLayer::updateLayerPositionRecursive(UpdateLayerPositionsFlags
> flags)
>      if (flags & ForceMayNeedPaintInvalidation)
>          m_renderer->setMayNeedPaintInvalidation(true);
>
> -    // Clear our cached clip rect information.
> -    m_clipper.clearClipRects();
> -
>      if (hasOverflowControls()) {
>          // FIXME: We should figure out the right time to position the
> overflow controls.
>          // This call appears to be necessary to pass some layout test
> that use EventSender,
> @@ -345,54 +344,22 @@ bool RenderLayer::scrollsWithRespectTo(const
> RenderLayer* other) const
>      return compositingInputs().ancestorScrollingLayer !=
> other->compositingInputs().ancestorScrollingLayer;
>  }
>
> -void RenderLayer::updateLayerPositionsAfterDocumentScroll()
> -{
> -    ASSERT(this == renderer()->view()->layer());
> -    updateLayerPositionsAfterScroll();
> -}
> -
>  void RenderLayer::updateLayerPositionsAfterOverflowScroll()
>  {
> -    // FIXME: why is it OK to not check the ancestors of this layer in
> order to
> -    // initialize the HasSeenViewportConstrainedAncestor and
> HasSeenAncestorWithOverflowClip flags?
> -    updateLayerPositionsAfterScroll(IsOverflowScroll);
> +    m_clipper.clearClipRectsIncludingDescendants();
> +    updateLayerPositionsAfterScrollRecursive();
>  }
>
> -void RenderLayer::updateLayerPositionsAfterScroll(
> UpdateLayerPositionsAfterScrollFlags flags)
> +void RenderLayer::updateLayerPositionsAfterScrollRecursive()
>  {
> -    // FIXME: This shouldn't be needed, but there are some corner cases
> where
> -    // these flags are still dirty. Update so that the check below is
> valid.
> -    updateDescendantDependentFlags();
> -
> -    // If we have no visible content and no visible descendants, there is
> no point recomputing
> -    // our rectangles as they will be empty. If our visibility changes,
> we are expected to
> -    // recompute all our positions anyway.
> -    if (subtreeIsInvisible())
> -        return;
> -
> -    if (updateLayerPosition())
> -        flags |= HasChangedAncestor;
> -
> -    if ((flags & HasChangedAncestor) || (flags &
> HasSeenViewportConstrainedAncestor) || (flags & IsOverflowScroll))
> -        m_clipper.clearClipRects();
> -
> -    if (renderer()->style()->hasViewportConstrainedPosition())
> -        flags |= HasSeenViewportConstrainedAncestor;
> +    updateLayerPosition();
>
> -    if (renderer()->hasOverflowClip())
> -        flags |= HasSeenAncestorWithOverflowClip;
> -
> -    if ((flags & IsOverflowScroll) && (flags &
> HasSeenAncestorWithOverflowClip) && !renderer()->isTableCell()) {
> -        // FIXME: We could track the repaint container as we walk down
> the tree.
> +    // FIXME: We could track the repaint container as we walk down the
> tree.
> +    if (!renderer()->isTableCell())
>          m_renderer->setPreviousPaintInvalidationRect(m_renderer->
> boundsRectForPaintInvalidation(m_renderer->containerForPaintInvalidation(
> )));
> -    }
>
>      for (RenderLayer* child = firstChild(); child; child =
> child->nextSibling())
> -        child->updateLayerPositionsAfterScroll(flags);
> -
> -    // We don't update our reflection as scrolling is a translation which
> does not change the size()
> -    // of an object, thus RenderReplica will still repaint itself
> properly as the layer position was
> -    // updated above.
> +        child->updateLayerPositionsAfterScrollRecursive();
>  }
>
>  void RenderLayer::updateTransformationMatrix()
> Index: Source/core/rendering/RenderLayer.h
> diff --git a/Source/core/rendering/RenderLayer.h b/Source/core/rendering/
> RenderLayer.h
> index 4b815dbb7a59c15f642d4c7493919124d89d0a48..
> df2682b76efb846970472efb45250b8703ea5952 100644
> --- a/Source/core/rendering/RenderLayer.h
> +++ b/Source/core/rendering/RenderLayer.h
> @@ -163,7 +163,6 @@ public:
>
>      void updateLayerPositionsAfterLayout(const RenderLayer* rootLayer,
> UpdateLayerPositionsFlags);
>      void updateLayerPositionsAfterOverflowScroll();
> -    void updateLayerPositionsAfterDocumentScroll();
>
>      // FIXME: Should updateLayerPositions be private?
>      void updateLayerPositionRecursive(UpdateLayerPositionsFlags =
> CheckForPaintInvalidation);
> @@ -514,15 +513,7 @@ private:
>      // Returns true if the position changed.
>      bool updateLayerPosition();
>
> -    enum UpdateLayerPositionsAfterScrollFlag {
> -        NoFlag = 0,
> -        IsOverflowScroll = 1 << 0,
> -        HasSeenViewportConstrainedAncestor = 1 << 1,
> -        HasSeenAncestorWithOverflowClip = 1 << 2,
> -        HasChangedAncestor = 1 << 3
> -    };
> -    typedef unsigned UpdateLayerPositionsAfterScrollFlags;
> -    void
updateLayerPositionsAfterScroll(UpdateLayerPositionsAfterScrollFlags
> = NoFlag);
> +    void updateLayerPositionsAfterScrollRecursive();
>
>      void setNextSibling(RenderLayer* next) { m_next = next; }
>      void setPreviousSibling(RenderLayer* prev) { m_previous = prev; }
> Index: Source/core/rendering/RenderLayerClipper.cpp
> diff --git a/Source/core/rendering/RenderLayerClipper.cpp
> b/Source/core/rendering/RenderLayerClipper.cpp
> index ba9d089451ca4d2365fc82211ee77a9a7a7646fe..
> a89a946480b93796b8479121d8da109f6cb6b2e4 100644
> --- a/Source/core/rendering/RenderLayerClipper.cpp
> +++ b/Source/core/rendering/RenderLayerClipper.cpp
> @@ -128,11 +128,6 @@ void RenderLayerClipper::
> clearClipRectsIncludingDescendants(ClipRectsCacheSlot c
>          layer->clipper().clearClipRectsIncludingDescendants(cacheSlot);
>  }
>
> -void RenderLayerClipper::clearClipRects()
> -{
> -    m_cache = nullptr;
> -}
> -
>  LayoutRect RenderLayerClipper::childrenClipRect() const
>  {
>      // FIXME: border-radius not accounted for.
> Index: Source/core/rendering/RenderLayerClipper.h
> diff --git a/Source/core/rendering/RenderLayerClipper.h
> b/Source/core/rendering/RenderLayerClipper.h
> index 17c64bafef5995ad66f434eb076a7531cc035bac..
> 649594860bba97c5a3f61b6af69de1c6d437f944 100644
> --- a/Source/core/rendering/RenderLayerClipper.h
> +++ b/Source/core/rendering/RenderLayerClipper.h
> @@ -98,7 +98,6 @@ public:
>
>      void clearClipRectsIncludingDescendants();
>      void clearClipRectsIncludingDescendants(ClipRectsCacheSlot);
> -    void clearClipRects();
>
>      LayoutRect childrenClipRect() const; // Returns the foreground clip
> rect of the layer in the document's coordinate space.
>      LayoutRect localClipRect() const; // Returns the background clip rect
> of the layer in the local coordinate space.
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2014-06-27 20:37:44]

The CQ bit was checked by abarth@chromium.org

[eseidel, 2014-06-27 20:38:19]

lgtm

[commit-bot: I haz the power, 2014-06-27 20:39:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/abarth@chromium.org/331653004/1

[commit-bot: I haz the power, 2014-06-27 21:53:36]

Change committed as 177144