Clean up BoxClipper

Clean up BoxClipper

This patch refactors BoxClipper to be a little cleaner. There should be
no change in behavior.

Changes:
1) BoxClipper::pushedClip has been removed as it was deadcode.

2) LayoutBox& is now const.

3) LayoutPoint m_accumulatedOffset has been removed since it was only
needed in the constructor.

4) bool m_pushedClip has been removed in favor of checking whether the
clip type is not uninitialized.

5) roundedRects heap allocation has been moved inside the
displayItemConstructionIsDisabled check.

This has been pulled out of https://codereview.chromium.org/1193433004.

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

[pdr., 2015-06-29 18:17:40]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, jbroman@chromium.org

[chrishtr, 2015-06-29 18:22:57]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-29 18:22:58]

lgtm

[commit-bot: I haz the power, 2015-06-29 18:23:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212293003/1

[danakj, 2015-06-29 18:48:29]

Thanks for doing this separately.

On Mon, Jun 29, 2015 at 11:17 AM, <pdr@chromium.org> wrote:

> Reviewers: chrishtr, jbroman,
>
> Description:
> Clean up BoxClipper
>
> This patch refactors BoxClipper to be a little cleaner. There should be
> no change in behavior.
>
> Changes:
> 1) BoxClipper::pushedClip has been removed as it was deadcode.
>
> 2) LayoutBox& is now const.
>
> 3) LayoutPoint m_accumulatedOffset has been removed since it was only
> needed in the constructor.
>
> 4) bool m_pushedClip has been removed in favor of checking whether the
> clip type is not uninitialized.
>
> 5) roundedRects heap allocation has been moved inside the
> displayItemConstructionIsDisabled check.
>
> This has been pulled out of https://codereview.chromium.org/1193433004.
>
> BUG=484943
>
> Please review this at https://codereview.chromium.org/1212293003/
>
> Base URL: https://chromium.googlesource.com/chromium/blink.git@master
>
> Affected files (+36, -34 lines):
>   M Source/core/layout/LayoutBox.h
>   M Source/core/layout/LayoutBox.cpp
>   M Source/core/layout/LayoutTable.h
>   M Source/core/layout/LayoutTable.cpp
>   M Source/core/paint/BoxClipper.h
>   M Source/core/paint/BoxClipper.cpp
>   M Source/platform/graphics/paint/DisplayItem.h
>   M Source/platform/graphics/paint/DisplayItem.cpp
>
>
> Index: Source/core/layout/LayoutBox.cpp
> diff --git a/Source/core/layout/LayoutBox.cpp
> b/Source/core/layout/LayoutBox.cpp
> index
>
ec29ce1a630c24b8a3e22cc8311564fe2bf7432d..20aca77f26ceaab7599f6bade59be3bdd749f70e
> 100644
> --- a/Source/core/layout/LayoutBox.cpp
> +++ b/Source/core/layout/LayoutBox.cpp
> @@ -1396,7 +1396,7 @@ bool LayoutBox::paintInvalidationStateIsDirty() const
>  }
>  #endif
>
> -LayoutRect LayoutBox::overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy relevancy)
> +LayoutRect LayoutBox::overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy relevancy) const
>  {
>      // FIXME: When overflow-clip (CSS3) is implemented, we'll obtain the
> property
>      // here.
> Index: Source/core/layout/LayoutBox.h
> diff --git a/Source/core/layout/LayoutBox.h
> b/Source/core/layout/LayoutBox.h
> index
>
f4bb2a5fde3fb063cb41d0150ea8bc0deb450536..ba526889900b194ee5b5843ac8808826e8e70299
> 100644
> --- a/Source/core/layout/LayoutBox.h
> +++ b/Source/core/layout/LayoutBox.h
> @@ -580,7 +580,7 @@ public:
>
>      virtual LayoutRect localCaretRect(InlineBox*, int caretOffset,
> LayoutUnit* extraWidthToEndOfLine = nullptr) override;
>
> -    virtual LayoutRect overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy = IgnoreOverlayScrollbarSize);
> +    virtual LayoutRect overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy = IgnoreOverlayScrollbarSize) const;
>      LayoutRect clipRect(const LayoutPoint& location);
>      virtual bool hasControlClip() const { return false; }
>      virtual LayoutRect controlClipRect(const LayoutPoint&) const { return
> LayoutRect(); }
> Index: Source/core/layout/LayoutTable.cpp
> diff --git a/Source/core/layout/LayoutTable.cpp
> b/Source/core/layout/LayoutTable.cpp
> index
>
f001559160a6d8a7285c6fd39bdc19e481d86a3f..e7c7e2b8d55e760f362c752153aaee277d19c636
> 100644
> --- a/Source/core/layout/LayoutTable.cpp
> +++ b/Source/core/layout/LayoutTable.cpp
> @@ -1310,7 +1310,7 @@ int LayoutTable::firstLineBoxBaseline() const
>      return -1;
>  }
>
> -LayoutRect LayoutTable::overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy relevancy)
> +LayoutRect LayoutTable::overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy relevancy) const
>  {
>      LayoutRect rect = LayoutBlock::overflowClipRect(location, relevancy);
>
> Index: Source/core/layout/LayoutTable.h
> diff --git a/Source/core/layout/LayoutTable.h
> b/Source/core/layout/LayoutTable.h
> index
>
2d335f457ae8b8cdbb86b99dd8e5332c02466b6f..0532969fc51bc6a0cf43c47a2fce7371f5e8b23d
> 100644
> --- a/Source/core/layout/LayoutTable.h
> +++ b/Source/core/layout/LayoutTable.h
> @@ -310,7 +310,7 @@ private:
>      LayoutUnit convertStyleLogicalWidthToComputedWidth(const Length&
> styleLogicalWidth, LayoutUnit availableWidth);
>      LayoutUnit convertStyleLogicalHeightToComputedHeight(const Length&
> styleLogicalHeight);
>
> -    virtual LayoutRect overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy = IgnoreOverlayScrollbarSize) override;
> +    virtual LayoutRect overflowClipRect(const LayoutPoint& location,
> OverlayScrollbarSizeRelevancy = IgnoreOverlayScrollbarSize) const override;
>
>      virtual void addOverflowFromChildren() override;
>
> Index: Source/core/paint/BoxClipper.cpp
> diff --git a/Source/core/paint/BoxClipper.cpp
> b/Source/core/paint/BoxClipper.cpp
> index
>
e9ad1b3894cdccf3d4cbe5c71541fd723396a040..32f4f9ccf14647fa7cdda1d964baca0e9d393e3d
> 100644
> --- a/Source/core/paint/BoxClipper.cpp
> +++ b/Source/core/paint/BoxClipper.cpp
> @@ -15,12 +15,10 @@
>
>  namespace blink {
>
> -BoxClipper::BoxClipper(LayoutBox& box, const PaintInfo& paintInfo, const
> LayoutPoint& accumulatedOffset, ContentsClipBehavior contentsClipBehavior)
> -    : m_pushedClip(false)
> -    , m_accumulatedOffset(accumulatedOffset)
> +BoxClipper::BoxClipper(const LayoutBox& box, const PaintInfo& paintInfo,
> const LayoutPoint& accumulatedOffset, ContentsClipBehavior
> contentsClipBehavior)
> +    : m_box(box)
>      , m_paintInfo(paintInfo)
> -    , m_box(box)
> -    , m_clipType(DisplayItem::ClipBoxPaintPhaseFirst)
> +    , m_clipType(DisplayItem::UninitializedType)
>  {
>      if (m_paintInfo.phase == PaintPhaseBlockBackground ||
> m_paintInfo.phase == PaintPhaseSelfOutline || m_paintInfo.phase ==
> PaintPhaseMask)
>          return;
> @@ -31,11 +29,11 @@ BoxClipper::BoxClipper(LayoutBox& box, const
> PaintInfo& paintInfo, const LayoutP
>      if (!isControlClip && !isOverflowClip)
>          return;
>
> -    LayoutRect clipRect = isControlClip ?
> m_box.controlClipRect(m_accumulatedOffset) :
> m_box.overflowClipRect(m_accumulatedOffset);
> +    LayoutRect clipRect = isControlClip ?
> m_box.controlClipRect(accumulatedOffset) :
> m_box.overflowClipRect(accumulatedOffset);
>      FloatRoundedRect clipRoundedRect(0, 0, 0, 0);
>      bool hasBorderRadius = m_box.style()->hasBorderRadius();
>      if (hasBorderRadius)
> -        clipRoundedRect =
> m_box.style()->getRoundedInnerBorderFor(LayoutRect(m_accumulatedOffset,
> m_box.size()));
> +        clipRoundedRect =
> m_box.style()->getRoundedInnerBorderFor(LayoutRect(accumulatedOffset,
> m_box.size()));
>
>      if (contentsClipBehavior == SkipContentsClipIfPossible) {
>          LayoutRect contentsVisualOverflow =
> m_box.contentsVisualOverflowRect();
> @@ -45,50 +43,54 @@ BoxClipper::BoxClipper(LayoutBox& box, const
> PaintInfo& paintInfo, const LayoutP
>          LayoutRect conservativeClipRect = clipRect;
>          if (hasBorderRadius)
>
> 
conservativeClipRect.intersect(LayoutRect(clipRoundedRect.radiusCenterRect()));
> -        conservativeClipRect.moveBy(-m_accumulatedOffset);
> +        conservativeClipRect.moveBy(-accumulatedOffset);
>          if (m_box.hasLayer())
>              conservativeClipRect.move(m_box.scrolledContentOffset());
>          if (conservativeClipRect.contains(contentsVisualOverflow))
>              return;
>      }
>
> -    OwnPtr<Vector<FloatRoundedRect>> roundedRects;
> -    if (hasBorderRadius) {
> -        roundedRects = adoptPtr(new Vector<FloatRoundedRect>());
> -        roundedRects->append(clipRoundedRect);
> -    }
> -
> -    OwnPtr<ClipDisplayItem> clipDisplayItem =
> ClipDisplayItem::create(m_box, m_clipType, pixelSnappedIntRect(clipRect),
> roundedRects.release());
>      if (RuntimeEnabledFeatures::slimmingPaintEnabled()) {
> -        m_clipType = m_paintInfo.displayItemTypeForClipping();
>          ASSERT(m_paintInfo.context->displayItemList());
> -        if
>
(!m_paintInfo.context->displayItemList()->displayItemConstructionIsDisabled())
> +        if
>
(!m_paintInfo.context->displayItemList()->displayItemConstructionIsDisabled())
> {
> +            m_clipType = m_paintInfo.displayItemTypeForClipping();
> +            OwnPtr<Vector<FloatRoundedRect>> roundedRects;
> +            if (hasBorderRadius) {
> +                roundedRects = adoptPtr(new Vector<FloatRoundedRect>());
> +                roundedRects->append(clipRoundedRect);
> +            }
> +            OwnPtr<ClipDisplayItem> clipDisplayItem =
> ClipDisplayItem::create(m_box, m_clipType, pixelSnappedIntRect(clipRect),
> roundedRects.release());
>
>  m_paintInfo.context->displayItemList()->add(clipDisplayItem.release());
> +        }
>      } else {
> +        m_clipType = m_paintInfo.displayItemTypeForClipping();
> +        OwnPtr<Vector<FloatRoundedRect>> roundedRects;
> +        if (hasBorderRadius) {
> +            roundedRects = adoptPtr(new Vector<FloatRoundedRect>());
> +            roundedRects->append(clipRoundedRect);
> +        }
> +        OwnPtr<ClipDisplayItem> clipDisplayItem =
> ClipDisplayItem::create(m_box, m_clipType, pixelSnappedIntRect(clipRect),
> roundedRects.release());
>          clipDisplayItem->replay(*paintInfo.context);
>      }
> -
> -    m_pushedClip = true;
>  }
>
>  BoxClipper::~BoxClipper()
>  {
> -    if (!m_pushedClip)
> +    if (m_clipType == DisplayItem::UninitializedType)
>          return;
>
>      ASSERT(m_box.hasControlClip() || (m_box.hasOverflowClip() &&
> !m_box.layer()->isSelfPaintingLayer()));
>
> -    DisplayItem::Type endType =
> DisplayItem::clipTypeToEndClipType(m_clipType);
>      if (RuntimeEnabledFeatures::slimmingPaintEnabled()) {
>          ASSERT(m_paintInfo.context->displayItemList());
>          if
>
(!m_paintInfo.context->displayItemList()->displayItemConstructionIsDisabled())
> {
>              if
> (m_paintInfo.context->displayItemList()->lastDisplayItemIsNoopBegin())
>
>  m_paintInfo.context->displayItemList()->removeLastDisplayItem();
>              else
> -
> m_paintInfo.context->displayItemList()->add(EndClipDisplayItem::create(m_box,
> endType));
> +
> m_paintInfo.context->displayItemList()->add(EndClipDisplayItem::create(m_box,
> DisplayItem::clipTypeToEndClipType(m_clipType)));
>          }
>      } else {
> -        EndClipDisplayItem endClipDisplayItem(m_box, endType);
> +        EndClipDisplayItem endClipDisplayItem(m_box,
> DisplayItem::clipTypeToEndClipType(m_clipType));
>          endClipDisplayItem.replay(*m_paintInfo.context);
>      }
>  }
> Index: Source/core/paint/BoxClipper.h
> diff --git a/Source/core/paint/BoxClipper.h
> b/Source/core/paint/BoxClipper.h
> index
>
106bfec7ee8fb5332cb2bb91f579d73ad7906e08..59b6a6dd93c9b707cb91cba87286d2f21e596952
> 100644
> --- a/Source/core/paint/BoxClipper.h
> +++ b/Source/core/paint/BoxClipper.h
> @@ -17,15 +17,11 @@ enum ContentsClipBehavior { ForceContentsClip,
> SkipContentsClipIfPossible };
>
>  class BoxClipper {
>  public:
> -    BoxClipper(LayoutBox&, const PaintInfo&, const LayoutPoint&
> accumulatedOffset, ContentsClipBehavior);
> +    BoxClipper(const LayoutBox&, const PaintInfo&, const LayoutPoint&
> accumulatedOffset, ContentsClipBehavior);
>      ~BoxClipper();
> -
> -    bool pushedClip() { return m_pushedClip; }
>  private:
> -    bool m_pushedClip;
> -    LayoutPoint m_accumulatedOffset;
> +    const LayoutBox& m_box;
>      const PaintInfo& m_paintInfo;
> -    LayoutBox& m_box;
>      DisplayItem::Type m_clipType;
>  };
>
> Index: Source/platform/graphics/paint/DisplayItem.cpp
> diff --git a/Source/platform/graphics/paint/DisplayItem.cpp
> b/Source/platform/graphics/paint/DisplayItem.cpp
> index
>
58b42d478555af9c107f94ffbf8970ca5c0fd752..c5ea8215da9c523be93db06cc0d55cc498b57665
> 100644
> --- a/Source/platform/graphics/paint/DisplayItem.cpp
> +++ b/Source/platform/graphics/paint/DisplayItem.cpp
> @@ -148,6 +148,9 @@ WTF::String DisplayItem::typeAsDebugString(Type type)
>      if (isEndClipType(type))
>          return "End" + clipTypeAsDebugString(endClipTypeToClipType(type));
>
> +    if (type == UninitializedType)
> +        return "UninitializedType";
> +
>      PAINT_PHASE_BASED_DEBUG_STRINGS(FloatClip);
>      if (isEndFloatClipType(type))
>          return "End" +
> typeAsDebugString(endFloatClipTypeToFloatClipType(type));
> Index: Source/platform/graphics/paint/DisplayItem.h
> diff --git a/Source/platform/graphics/paint/DisplayItem.h
> b/Source/platform/graphics/paint/DisplayItem.h
> index
>
ed8985384fd6aee80bffa12ab56447a5271950e0..e8c65223544cb87e83e2dcad1400997dc500a883
> 100644
> --- a/Source/platform/graphics/paint/DisplayItem.h
> +++ b/Source/platform/graphics/paint/DisplayItem.h
> @@ -173,7 +173,8 @@ public:
>          EndSubtreePaintPhaseLast = EndSubtreePaintPhaseFirst +
> PaintPhaseMax,
>          EndSubtreeLast = EndSubtreePaintPhaseLast,
>
> -        TypeLast = EndSubtreeLast
> +        UninitializedType,
> +        TypeLast = UninitializedType
>      };
>
>      struct Id {
>
>
>

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

[commit-bot: I haz the power, 2015-06-29 19:45:43]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198013