Remove duplicated code for image-strip computation in InlineFlowBoxPainter

Source/core/paint/InlineFlowBoxPainter.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/paint/InlineFlowBoxPainter.h"
 
 #include "core/paint/BoxPainter.h"
 #include "core/paint/DrawingRecorder.h"
 #include "core/rendering/InlineFlowBox.h"
@@ skipping 88 matching lines @@
     StyleImage* img = fillLayer.image();
     bool hasFillImage = img && img->canRender(m_inlineFlowBox.renderer(), m_inlineFlowBox.renderer().style()->effectiveZoom());
     if ((!hasFillImage && !m_inlineFlowBox.renderer().style()->hasBorderRadius()) || (!m_inlineFlowBox.prevLineBox() && !m_inlineFlowBox.nextLineBox()) || !m_inlineFlowBox.parent()) {
         BoxPainter::paintFillLayerExtended(*m_inlineFlowBox.boxModelObject(), paintInfo, c, fillLayer, rect, BackgroundBleedNone, &m_inlineFlowBox, rect.size(), op);
     } else if (m_inlineFlowBox.renderer().style()->boxDecorationBreak() == DCLONE) {
         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, 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);
     }
 }
 
 void InlineFlowBoxPainter::paintBoxShadow(const PaintInfo& info, RenderStyle* s, ShadowStyle shadowStyle, const LayoutRect& paintRect)
 {
     if ((!m_inlineFlowBox.prevLineBox() && !m_inlineFlowBox.nextLineBox()) || !m_inlineFlowBox.parent()) {
         BoxPainter::paintBoxShadow(info, paintRect, s, shadowStyle);
     } else {
         // FIXME: We can do better here in the multi-line case. We want to push a clip so that the shadow doesn't
         // protrude incorrectly at the edges, and we want to possibly include shadows cast from the previous/following lines
@@ skipping 22 matching lines @@
         if (box->includeLogicalLeftEdge()) {
             clipRect.setY(paintRect.y() - outsets.top());
             clipRect.setHeight(paintRect.height() + outsets.top());
         }
         if (box->includeLogicalRightEdge())
             clipRect.setHeight(clipRect.height() + outsets.bottom());
     }
     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);
     if (!paintInfo.shouldPaintWithinRoot(&m_inlineFlowBox.renderer()) || m_inlineFlowBox.renderer().style()->visibility() != VISIBLE)
         return;
 
     // You can use p::first-line to specify a background. If so, the root line boxes for
     // a line may actually have to paint a background.
     RenderStyle* styleToUse = m_inlineFlowBox.renderer().style(m_inlineFlowBox.isFirstLineStyle());
     bool shouldPaintBoxDecorationBackground;
@@ skipping 32 matching lines @@
         bool hasBorderImage = borderImageSource && borderImageSource->canRender(m_inlineFlowBox.renderer(), styleToUse->effectiveZoom());
         if (hasBorderImage && !borderImageSource->isLoaded())
             return; // Don't paint anything while we wait for the image to load.
 
         // The simple case is where we either have no border image or we are the only box for this object.
         // In those cases only a single call to draw is required.
         if (!hasBorderImage || (!m_inlineFlowBox.prevLineBox() && !m_inlineFlowBox.nextLineBox())) {
             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()));
         }
     }
 }
 
 void InlineFlowBoxPainter::paintMask(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     if (!paintInfo.shouldPaintWithinRoot(&m_inlineFlowBox.renderer()) || m_inlineFlowBox.renderer().style()->visibility() != VISIBLE || paintInfo.phase != PaintPhaseMask)
         return;
 
     LayoutRect frameRect = roundedFrameRectClampedToLineTopAndBottomIfNeeded();
@@ skipping 29 matching lines @@
     bool hasBoxImage = maskBoxImage && maskBoxImage->canRender(m_inlineFlowBox.renderer(), m_inlineFlowBox.renderer().style()->effectiveZoom());
     if (!hasBoxImage || !maskBoxImage->isLoaded()) {
         if (pushTransparencyLayer)
             paintInfo.context->endLayer();
         return; // Don't paint anything while we wait for the image to load.
     }
 
     // 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)
         paintInfo.context->endLayer();
 }
 
 LayoutRect InlineFlowBoxPainter::roundedFrameRectClampedToLineTopAndBottomIfNeeded() const
 {
     // Pixel snap rect painting.
     LayoutRect rect = m_inlineFlowBox.roundedFrameRect();
@@ skipping 11 matching lines @@
             rect.setHeight(logicalHeight);
         } else {
             rect.setX(logicalTop);
             rect.setWidth(logicalHeight);
         }
     }
     return rect;
 }
 
 } // namespace blink