Remove duplicated code for image-strip computation in InlineFlowBoxPainter

Source/core/paint/InlineFlowBoxPainter.cpp

Index: Source/core/paint/InlineFlowBoxPainter.cpp
diff --git a/Source/core/paint/InlineFlowBoxPainter.cpp b/Source/core/paint/InlineFlowBoxPainter.cpp
index 1b32e53636489cc2e8a852bda666ac4c70204d08..70733e4cc18f15720f70e6144aa106b93207cc82 100644
--- a/Source/core/paint/InlineFlowBoxPainter.cpp
+++ b/Source/core/paint/InlineFlowBoxPainter.cpp
@@ -106,34 +106,12 @@ void InlineFlowBoxPainter::paintFillLayer(const PaintInfo& paintInfo, const Colo
         BoxPainter::paintFillLayerExtended(*m_inlineFlowBox.boxModelObject(), paintInfo, c, fillLayer, rect, BackgroundBleedNone, &m_inlineFlowBox, rect.size(), op);
     } else {
         // We have a fill image that spans multiple lines.
-        // We need to adjust tx and ty by the width of all previous lines.
-        // Think of background painting on inlines as though you had one long line, a single continuous
-        // strip. Even though that strip has been broken up across multiple lines, you still paint it
-        // as though you had one single line. This means each line has to pick up the background where
-        // the previous line left off.
-        LayoutUnit logicalOffsetOnLine = 0;
-        LayoutUnit totalLogicalWidth;
-        if (m_inlineFlowBox.renderer().style()->direction() == LTR) {
-            for (InlineFlowBox* curr = m_inlineFlowBox.prevLineBox(); curr; curr = curr->prevLineBox())
-                logicalOffsetOnLine += curr->logicalWidth();
-            totalLogicalWidth = logicalOffsetOnLine;
-            for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->nextLineBox())
-                totalLogicalWidth += curr->logicalWidth();
-        } else {
-            for (InlineFlowBox* curr = m_inlineFlowBox.nextLineBox(); curr; curr = curr->nextLineBox())
-                logicalOffsetOnLine += curr->logicalWidth();
-            totalLogicalWidth = logicalOffsetOnLine;
-            for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->prevLineBox())
-                totalLogicalWidth += curr->logicalWidth();
-        }
-        LayoutUnit stripX = rect.x() - (m_inlineFlowBox.isHorizontal() ? logicalOffsetOnLine : LayoutUnit());
-        LayoutUnit stripY = rect.y() - (m_inlineFlowBox.isHorizontal() ? LayoutUnit() : logicalOffsetOnLine);
-        LayoutUnit stripWidth = m_inlineFlowBox.isHorizontal() ? totalLogicalWidth : static_cast<LayoutUnit>(m_inlineFlowBox.width());
-        LayoutUnit stripHeight = m_inlineFlowBox.isHorizontal() ? static_cast<LayoutUnit>(m_inlineFlowBox.height()) : totalLogicalWidth;
-
+        // FIXME: frameSize ought to be the same as rect.size().
+        LayoutSize frameSize(static_cast<LayoutUnit>(m_inlineFlowBox.width()), static_cast<LayoutUnit>(m_inlineFlowBox.height()));
+        LayoutRect imageStripPaintRect = paintRectForImageStrip(rect.location(), frameSize, m_inlineFlowBox.renderer().style()->isLeftToRightDirection());
         GraphicsContextStateSaver stateSaver(*paintInfo.context);
         paintInfo.context->clip(LayoutRect(rect.x(), rect.y(), m_inlineFlowBox.width(), m_inlineFlowBox.height()));
-        BoxPainter::paintFillLayerExtended(*m_inlineFlowBox.boxModelObject(), paintInfo, c, fillLayer, LayoutRect(stripX, stripY, stripWidth, stripHeight), BackgroundBleedNone, &m_inlineFlowBox, rect.size(), op);
+        BoxPainter::paintFillLayerExtended(*m_inlineFlowBox.boxModelObject(), paintInfo, c, fillLayer, imageStripPaintRect, BackgroundBleedNone, &m_inlineFlowBox, rect.size(), op);
     }
 }
 
@@ -176,6 +154,36 @@ static LayoutRect clipRectForNinePieceImageStrip(InlineFlowBox* box, const NineP
     return clipRect;
 }
 
+LayoutRect InlineFlowBoxPainter::paintRectForImageStrip(const LayoutPoint& paintOffset, const LayoutSize& frameSize, bool isLeftToRightDirection) const
+{
+    // We have a fill/border/mask image that spans multiple lines.
+    // We need to adjust the offset by the width of all previous lines.
+    // Think of background painting on inlines as though you had one long line, a single continuous
+    // strip. Even though that strip has been broken up across multiple lines, you still paint it
+    // as though you had one single line. This means each line has to pick up the background where
+    // the previous line left off.
+    LayoutUnit logicalOffsetOnLine = 0;
+    LayoutUnit totalLogicalWidth;
+    if (isLeftToRightDirection) {
+        for (InlineFlowBox* curr = m_inlineFlowBox.prevLineBox(); curr; curr = curr->prevLineBox())
+            logicalOffsetOnLine += curr->logicalWidth();
+        totalLogicalWidth = logicalOffsetOnLine;
+        for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->nextLineBox())
+            totalLogicalWidth += curr->logicalWidth();
+    } else {
+        for (InlineFlowBox* curr = m_inlineFlowBox.nextLineBox(); curr; curr = curr->nextLineBox())
+            logicalOffsetOnLine += curr->logicalWidth();
+        totalLogicalWidth = logicalOffsetOnLine;
+        for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->prevLineBox())
+            totalLogicalWidth += curr->logicalWidth();
+    }
+    LayoutUnit stripX = paintOffset.x() - (m_inlineFlowBox.isHorizontal() ? logicalOffsetOnLine : LayoutUnit());
+    LayoutUnit stripY = paintOffset.y() - (m_inlineFlowBox.isHorizontal() ? LayoutUnit() : logicalOffsetOnLine);
+    LayoutUnit stripWidth = m_inlineFlowBox.isHorizontal() ? totalLogicalWidth : frameSize.width();
+    LayoutUnit stripHeight = m_inlineFlowBox.isHorizontal() ? frameSize.height() : totalLogicalWidth;
+    return LayoutRect(stripX, stripY, stripWidth, stripHeight);
+}
+
 void InlineFlowBoxPainter::paintBoxDecorationBackground(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     ASSERT(paintInfo.phase == PaintPhaseForeground);
@@ -228,28 +236,11 @@ void InlineFlowBoxPainter::paintBoxDecorationBackground(const PaintInfo& paintIn
             BoxPainter::paintBorder(*m_inlineFlowBox.boxModelObject(), paintInfo, paintRect, m_inlineFlowBox.renderer().style(m_inlineFlowBox.isFirstLineStyle()), BackgroundBleedNone, m_inlineFlowBox.includeLogicalLeftEdge(), m_inlineFlowBox.includeLogicalRightEdge());
         } else {
             // We have a border image that spans multiple lines.
-            // We need to adjust tx and ty by the width of all previous lines.
-            // Think of border image painting on inlines as though you had one long line, a single continuous
-            // strip. Even though that strip has been broken up across multiple lines, you still paint it
-            // as though you had one single line. This means each line has to pick up the image where
-            // the previous line left off.
-            // FIXME: What the heck do we do with RTL here? The math we're using is obviously not right,

[chrishtr, 2014/12/04 17:25:03]

Are you sure it's correct to address RTL directly in this code?

[fs, 2014/12/04 17:42:02]

I've not done anything to address this FIXME (so maybe it should still be
there... oops, I'll add it back.) - ie. isLeftToRightDirection is passed as
'true' here.

[chrishtr, 2014/12/04 17:47:57]

Ah I missed that subtlety.

[fs, 2014/12/04 18:15:45]

Restored FIXME (and one like it to the mask case.)

-            // but it isn't even clear how this should work at all.
-            LayoutUnit logicalOffsetOnLine = 0;
-            for (InlineFlowBox* curr = m_inlineFlowBox.prevLineBox(); curr; curr = curr->prevLineBox())
-                logicalOffsetOnLine += curr->logicalWidth();
-            LayoutUnit totalLogicalWidth = logicalOffsetOnLine;
-            for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->nextLineBox())
-                totalLogicalWidth += curr->logicalWidth();
-            LayoutUnit stripX = adjustedPaintOffset.x() - (m_inlineFlowBox.isHorizontal() ? logicalOffsetOnLine : LayoutUnit());
-            LayoutUnit stripY = adjustedPaintOffset.y() - (m_inlineFlowBox.isHorizontal() ? LayoutUnit() : logicalOffsetOnLine);
-            LayoutUnit stripWidth = m_inlineFlowBox.isHorizontal() ? totalLogicalWidth : frameRect.width();
-            LayoutUnit stripHeight = m_inlineFlowBox.isHorizontal() ? frameRect.height() : totalLogicalWidth;
-
+            LayoutRect imageStripPaintRect = paintRectForImageStrip(adjustedPaintOffset, frameRect.size());
             LayoutRect clipRect = clipRectForNinePieceImageStrip(&m_inlineFlowBox, borderImage, paintRect);
             GraphicsContextStateSaver stateSaver(*paintInfo.context);
             paintInfo.context->clip(clipRect);
-            BoxPainter::paintBorder(*m_inlineFlowBox.boxModelObject(), paintInfo, LayoutRect(stripX, stripY, stripWidth, stripHeight), m_inlineFlowBox.renderer().style(m_inlineFlowBox.isFirstLineStyle()));
+            BoxPainter::paintBorder(*m_inlineFlowBox.boxModelObject(), paintInfo, imageStripPaintRect, m_inlineFlowBox.renderer().style(m_inlineFlowBox.isFirstLineStyle()));
         }
     }
 }
@@ -299,25 +290,14 @@ void InlineFlowBoxPainter::paintMask(const PaintInfo& paintInfo, const LayoutPoi
     // The simple case is where we are the only box for this object. In those
     // cases only a single call to draw is required.
     if (!m_inlineFlowBox.prevLineBox() && !m_inlineFlowBox.nextLineBox()) {
-        BoxPainter::paintNinePieceImage(*m_inlineFlowBox.boxModelObject(), paintInfo.context, LayoutRect(adjustedPaintOffset, frameRect.size()), m_inlineFlowBox.renderer().style(), maskNinePieceImage, compositeOp);
+        BoxPainter::paintNinePieceImage(*m_inlineFlowBox.boxModelObject(), paintInfo.context, paintRect, m_inlineFlowBox.renderer().style(), maskNinePieceImage, compositeOp);
     } else {
         // We have a mask image that spans multiple lines.
-        // We need to adjust _tx and _ty by the width of all previous lines.

[chrishtr, 2014/12/04 17:25:03]

Same Q here about RTL. Are we missing RTL test coverage?

[fs, 2014/12/04 17:42:02]

See above. I'm not sure about the test-coverage for RTL, but it's not unlikely
that it's missing. I've not intended to change behavior with this change though
(isLeftToRightDirection == true here and above.)

-        LayoutUnit logicalOffsetOnLine = 0;
-        for (InlineFlowBox* curr = m_inlineFlowBox.prevLineBox(); curr; curr = curr->prevLineBox())
-            logicalOffsetOnLine += curr->logicalWidth();
-        LayoutUnit totalLogicalWidth = logicalOffsetOnLine;
-        for (InlineFlowBox* curr = &m_inlineFlowBox; curr; curr = curr->nextLineBox())
-            totalLogicalWidth += curr->logicalWidth();
-        LayoutUnit stripX = adjustedPaintOffset.x() - (m_inlineFlowBox.isHorizontal() ? logicalOffsetOnLine : LayoutUnit());
-        LayoutUnit stripY = adjustedPaintOffset.y() - (m_inlineFlowBox.isHorizontal() ? LayoutUnit() : logicalOffsetOnLine);
-        LayoutUnit stripWidth = m_inlineFlowBox.isHorizontal() ? totalLogicalWidth : frameRect.width();
-        LayoutUnit stripHeight = m_inlineFlowBox.isHorizontal() ? frameRect.height() : totalLogicalWidth;
-
+        LayoutRect imageStripPaintRect = paintRectForImageStrip(adjustedPaintOffset, frameRect.size());
         LayoutRect clipRect = clipRectForNinePieceImageStrip(&m_inlineFlowBox, maskNinePieceImage, paintRect);
         GraphicsContextStateSaver stateSaver(*paintInfo.context);
         paintInfo.context->clip(clipRect);
-        BoxPainter::paintNinePieceImage(*m_inlineFlowBox.boxModelObject(), paintInfo.context, LayoutRect(stripX, stripY, stripWidth, stripHeight), m_inlineFlowBox.renderer().style(), maskNinePieceImage, compositeOp);
+        BoxPainter::paintNinePieceImage(*m_inlineFlowBox.boxModelObject(), paintInfo.context, imageStripPaintRect, m_inlineFlowBox.renderer().style(), maskNinePieceImage, compositeOp);
     }
 
     if (pushTransparencyLayer)