Refactor RenderBox::addVisualEffectOverflow()

Source/core/rendering/RenderBox.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 4049 matching lines @@
 {
     ASSERT(!needsLayout());
     // If fragmentation height has changed, we need to lay out. No need to enter the renderer if it
     // is childless, though.
     if (view()->layoutState()->pageLogicalHeightChanged() && slowFirstChild())
         layoutScope.setChildNeedsLayout(this);
 }
 
 void RenderBox::addVisualEffectOverflow()
 {
-    if (!style()->boxShadow() && !style()->hasBorderImageOutsets() && !style()->hasOutline())
+    if (!hasVisualEffectOverflow())
         return;
 
-    LayoutRect borderBox = borderBoxRect();
-    LayoutUnit overflowMinX = borderBox.x();
-    LayoutUnit overflowMaxX = borderBox.maxX();
-    LayoutUnit overflowMinY = borderBox.y();
-    LayoutUnit overflowMaxY = borderBox.maxY();
+    // Add in the final overflow with shadows, outsets and outline combined.
+    LayoutRect visualEffectOverflow = borderBoxRect();

[Julien - ping for review, 2014/07/01 23:00:16]

We have a helper function for "no overflow" that should be used here:
noOverflowRect()

[Xianzhu, 2014/07/02 00:02:03]

noOverflowRect seems incorrect to me:
- the current code always uses borderBoxRect as the default value and starting
point of visualOverflowRect;
- noOverflowRect is in content coordinates, while borderBoxRect's location is
always (0,0);
- noOverflowRect flips according to content writing-mode, while borderBoxRect
doesn't.

[Xianzhu, 2014/07/02 00:05:23]

Correction: noOverflowRect is context-box in border-box coordinates, so it is in
the same coordinates as borderBoxRect, but the sizes of them are different.

+    visualEffectOverflow.expand(computeVisualEffectOverflowExtent());
+    addVisualOverflow(visualEffectOverflow);
+}
 
-    // Compute box-shadow overflow first.
-    if (style()->boxShadow()) {
-        LayoutUnit shadowLeft;
-        LayoutUnit shadowRight;
-        LayoutUnit shadowTop;
-        LayoutUnit shadowBottom;
-        style()->getBoxShadowExtent(shadowTop, shadowRight, shadowBottom, shadowLeft);
+LayoutBoxExtent RenderBox::computeVisualEffectOverflowExtent() const
+{
+    if (!hasVisualEffectOverflow())

[Julien - ping for review, 2014/07/01 23:00:15]

Nobody calls this without a visual effect overflow so we should just have an
ASSERT.

[Xianzhu, 2014/07/02 00:02:03]

Done.

+        return LayoutBoxExtent();
 
-        // Note that box-shadow extent's left and top are negative when extends to left and top, respectively.
-        overflowMinX = borderBox.x() + shadowLeft;
-        overflowMaxX = borderBox.maxX() + shadowRight;
-        overflowMinY = borderBox.y() + shadowTop;
-        overflowMaxY = borderBox.maxY() + shadowBottom;
-    }
+    LayoutUnit top;
+    LayoutUnit right;
+    LayoutUnit bottom;
+    LayoutUnit left;
 
-    // Now compute border-image-outset overflow.
+    style()->getBoxShadowExtent(top, right, bottom, left);

[Julien - ping for review, 2014/07/01 23:00:16]

While it's a no-op to call getBoxShadowExtent without box-shadow, I would rather
have it gated by style->boxShadow() for consistency.

[Xianzhu, 2014/07/02 00:02:03]

Done.

+    // Box shadow extent's top and left are negative when extend to left and top direction, respectively.
+    // Negate to make them positive.
+    top = -top;
+    left = -left;

[Julien - ping for review, 2014/07/01 23:00:15]

I wonder if we couldn't make the code more consistent here (LayoutBoxExtent is
an extent for which negative values would mean intruding IMO).

[Xianzhu, 2014/07/02 00:02:03]

I tried that several days ago but haven't finished yet because it needs much
care not to bring bug into existing code, especially in areas that I'm not
familiar with.

May continue when have time :)

+
     if (style()->hasBorderImageOutsets()) {
         LayoutBoxExtent borderOutsets = style()->borderImageOutsets();
-
-        overflowMinX = std::min(overflowMinX, borderBox.x() - borderOutsets.left());
-        overflowMaxX = std::max(overflowMaxX, borderBox.maxX() + borderOutsets.right());
-        overflowMinY = std::min(overflowMinY, borderBox.y() - borderOutsets.top());
-        overflowMaxY = std::max(overflowMaxY, borderBox.maxY() + borderOutsets.bottom());
+        top = std::max(top, borderOutsets.top());
+        right = std::max(right, borderOutsets.right());
+        bottom = std::max(bottom, borderOutsets.bottom());
+        left = std::max(left, borderOutsets.left());
     }
 
     if (style()->hasOutline()) {
         LayoutUnit outlineSize = style()->outlineSize();
-
-        overflowMinX = std::min(overflowMinX, borderBox.x() - outlineSize);
-        overflowMaxX = std::max(overflowMaxX, borderBox.maxX() + outlineSize);
-        overflowMinY = std::min(overflowMinY, borderBox.y() - outlineSize);
-        overflowMaxY = std::max(overflowMaxY, borderBox.maxY() + outlineSize);
+        top = std::max(top, outlineSize);
+        right = std::max(right, outlineSize);
+        bottom = std::max(bottom, outlineSize);
+        left = std::max(left, outlineSize);
     }
 
-    // Add in the final overflow with shadows, outsets and outline combined.
-    LayoutRect visualEffectOverflow(overflowMinX, overflowMinY, overflowMaxX - overflowMinX, overflowMaxY - overflowMinY);
-    addVisualOverflow(visualEffectOverflow);
+    return LayoutBoxExtent(top, right, bottom, left);
 }
 
 void RenderBox::addOverflowFromChild(RenderBox* child, const LayoutSize& delta)
 {
     // Never allow flow threads to propagate overflow up to a parent.
     if (child->isRenderFlowThread())
         return;
 
     // Only propagate layout overflow from the child if the child isn't clipping its overflow.  If it is, then
     // its overflow is internal to it, and we don't care about it.  layoutOverflowRectForPropagation takes care of this
@@ skipping 479 matching lines @@
         return 0;
 
     if (!layoutState && !flowThreadContainingBlock())
         return 0;
 
     RenderBlock* containerBlock = containingBlock();
     return containerBlock->offsetFromLogicalTopOfFirstPage() + logicalTop();
 }
 
 } // namespace WebCore