Refactor RenderBox::addVisualEffectOverflow()

Source/core/rendering/RenderBox.cpp

Index: Source/core/rendering/RenderBox.cpp
diff --git a/Source/core/rendering/RenderBox.cpp b/Source/core/rendering/RenderBox.cpp
index 269aa5dd75c1e3ea9c534c88a53874b1cd77c4b5..b28d087651868f619abf4066ab46abc1f8239c66 100644
--- a/Source/core/rendering/RenderBox.cpp
+++ b/Source/core/rendering/RenderBox.cpp
@@ -4067,52 +4067,48 @@ void RenderBox::markForPaginationRelayoutIfNeeded(SubtreeLayoutScope& layoutScop
 
 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();
-
-    // Compute box-shadow overflow first.
-    if (style()->boxShadow()) {
-        LayoutUnit shadowLeft;
-        LayoutUnit shadowRight;
-        LayoutUnit shadowTop;
-        LayoutUnit shadowBottom;
-        style()->getBoxShadowExtent(shadowTop, shadowRight, shadowBottom, shadowLeft);
-
-        // 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;
-    }
+    // 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);
+}
+
+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();
+
+    LayoutUnit top;
+    LayoutUnit right;
+    LayoutUnit bottom;
+    LayoutUnit left;
+
+    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 :)

 
-    // Now compute border-image-outset overflow.
     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)