Improve shadow support in EllipsisBox.

Source/core/rendering/EllipsisBox.cpp

Index: Source/core/rendering/EllipsisBox.cpp
diff --git a/Source/core/rendering/EllipsisBox.cpp b/Source/core/rendering/EllipsisBox.cpp
index b7611c1d20d834f490d213870c7eaa4336dc4bd0..0611c70b055fe3b60d3dfd269ab98aafe61f3f2a 100644
--- a/Source/core/rendering/EllipsisBox.cpp
+++ b/Source/core/rendering/EllipsisBox.cpp
@@ -30,6 +30,8 @@
 #include "core/rendering/RenderBlock.h"
 #include "core/rendering/RootInlineBox.h"
 
+#include <algorithm>
+
 namespace WebCore {
 
 void EllipsisBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset, LayoutUnit lineTop, LayoutUnit lineBottom)
@@ -39,13 +41,8 @@ void EllipsisBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset, La
     Color textColor = style->visitedDependentColor(CSSPropertyWebkitTextFillColor);

[Stephen Chennney, 2013/05/16 17:03:14]

Rename this "defaultTextColor".

[jbroman, 2013/05/16 17:22:47]

Changed to "styleTextColor" to be clear that this arises from the style, not
some global default.

     if (textColor != context->fillColor())
         context->setFillColor(textColor, style->colorSpace());
-    bool setShadow = false;
-    if (style->textShadow()) {
-        context->setShadow(LayoutSize(style->textShadow()->x(), style->textShadow()->y()),
-                           style->textShadow()->blur(), style->textShadow()->color(), style->colorSpace());
-        setShadow = true;
-    }
 
+    Color actualTextColor = textColor;

[Stephen Chennney, 2013/05/16 17:03:14]

Then name this "textColor".

[jbroman, 2013/05/16 17:22:47]

Done.

     const Font& font = style->font();
     if (selectionState() != RenderObject::SelectionNone) {
         paintSelection(context, paintOffset, style, font);
@@ -53,25 +50,59 @@ void EllipsisBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset, La
         // Select the correct color for painting the text.
         Color foreground = paintInfo.forceBlackText() ? Color::black : renderer()->selectionForegroundColor();
         if (foreground.isValid() && foreground != textColor)
-            context->setFillColor(foreground, style->colorSpace());
+            actualTextColor = foreground;
     }
 
     // FIXME: Why is this always LTR? Fix by passing correct text run flags below.
+    const ShadowData* shadow = style->textShadow();
     FloatPoint boxOrigin(paintOffset);
     boxOrigin.move(x(), y());
+    FloatRect boxRect(boxOrigin, LayoutSize(logicalWidth(), logicalHeight()));
     FloatPoint textOrigin(boxOrigin.x(), boxOrigin.y() + style->fontMetrics().ascent());
     TextRun textRun = RenderBlock::constructTextRun(renderer(), font, m_str, style, TextRun::AllowTrailingExpansion);
     TextRunPaintInfo textRunPaintInfo(textRun);
-    textRunPaintInfo.bounds = FloatRect(boxOrigin, FloatSize(logicalWidth(), logicalHeight()));
-    context->drawText(font, textRunPaintInfo, textOrigin);
+    textRunPaintInfo.bounds = boxRect;
+
+    bool opaque = actualTextColor.alpha() == 255;
+    if (!opaque)
+        context->setFillColor(Color::black, style->colorSpace());
+    else
+        context->setFillColor(actualTextColor, style->colorSpace());
+
+    do {
+        IntSize extraOffset;
+        if (shadow) {
+            FloatSize shadowOffset(shadow->x(), shadow->y());
+            if (shadow->next() || !opaque) {

[Stephen Chennney, 2013/05/16 17:03:14]

What the abstract meaning of this? I would prefer a variable "needToDrawAgain"
or something, describing why you need to take a different path. It also avoids
duplicating the test further down, which is a recipe for bugs if someone ever
makes changes. You can even use it as the while condition.

[jbroman, 2013/05/16 17:22:47]

I've tried to mirror paintTextWithShadows from InlineTextBox.cpp as closely as
possible in order to capture the current behaviour there here. I'm not a huge
fan of the mysterious condition either.

The reason I didn't clean this up here (or in InlineTextBox) is because I have
another CL following this one that does a more dramatic refactor of this code
which would eliminate this condition anyhow.

https://codereview.chromium.org/15137009/diff/18001/Source/core/rendering/Ell...

Would you prefer I come up with some abstract name for this condition in the
meantime, or leave it as-is?

[Stephen Chennney, 2013/05/16 17:34:56]

Now that I've seen the other patch, it's fine to leave this as is.

+                FloatRect shadowRect(boxRect);
+                shadowRect.inflate(shadow->blur());
+                shadowRect.move(shadowOffset);
+                context->save();
+                context->clip(shadowRect);
+                extraOffset = IntSize(0, 2 * boxRect.height() + std::max(0.0f, shadowOffset.height()) + shadow->blur());
+                shadowOffset -= extraOffset;
+            }
+            context->setShadow(shadowOffset, shadow->blur(), shadow->color(), style->colorSpace());
+        } else if (!opaque)
+            context->setFillColor(actualTextColor, style->colorSpace());
+
+        context->drawText(font, textRunPaintInfo, textOrigin + extraOffset);
+
+        if (!shadow)
+            break;
+
+        if (shadow->next() || !opaque)
+            context->restore();
+        else
+            context->clearShadow();
+
+        shadow = shadow->next();
+    } while (shadow || !opaque);
 
     // Restore the regular fill color.
     if (textColor != context->fillColor())
         context->setFillColor(textColor, style->colorSpace());
 
-    if (setShadow)
-        context->clearShadow();
-
     paintMarkupBox(paintInfo, paintOffset, lineTop, lineBottom, style);
 }