Move the shadows out of paintTextWithShadows.

Source/core/rendering/InlineTextBox.cpp

Index: Source/core/rendering/InlineTextBox.cpp
diff --git a/Source/core/rendering/InlineTextBox.cpp b/Source/core/rendering/InlineTextBox.cpp
index e1a36d330861201212f24d637545988a23063ba9..0fe2fa660ac1ef5cf18e2beb56bccbd8005d17da 100644
--- a/Source/core/rendering/InlineTextBox.cpp
+++ b/Source/core/rendering/InlineTextBox.cpp
@@ -329,7 +329,7 @@ static Color textColorForWhiteBackground(Color textColor)
     return distanceFromWhite > 65025 ? textColor : textColor.dark();
 }
 
-static void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness)
+static void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness, const ShadowList* textShadow = 0, bool horizontal = true)
 {
     TextDrawingModeFlags mode = context->textDrawingMode();
     if (strokeThickness > 0) {
@@ -349,6 +349,12 @@ static void updateGraphicsContext(GraphicsContext* context, const Color& fillCol
         if (strokeThickness != context->strokeThickness())
             context->setStrokeThickness(strokeThickness);
     }
+
+    // Text shadows are disabled when printing. http://crbug.com/258321
+    if (textShadow && !context->printing())
+        context->setDrawLooper(textShadow->createDrawLooper(DrawLooperBuilder::ShadowIgnoresAlpha, horizontal));
+    else
+        context->clearDrawLooper();

[f(malita), 2014/08/15 02:01:06]

Is clearing required here? I don't think we were doing it before.

[jbroman, 2014/08/19 18:00:43]

It's a little finicky. I've refactored to just use a state saver in a way that
doesn't seem to have a significant adverse performance impact.

 }
 
 bool InlineTextBox::isLineBreak() const
@@ -372,18 +378,12 @@ bool InlineTextBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re
     return false;
 }
 
-static void paintTextWithShadows(GraphicsContext* context,
+static void paintText(GraphicsContext* context,
     const Font& font, const TextRun& textRun,
     const AtomicString& emphasisMark, int emphasisMarkOffset,
     int startOffset, int endOffset, int truncationPoint,
-    const FloatPoint& textOrigin, const FloatRect& boxRect,
-    const ShadowList* shadowList, bool horizontal)
+    const FloatPoint& textOrigin, const FloatRect& boxRect)
 {
-    // Text shadows are disabled when printing. http://crbug.com/258321
-    bool hasShadow = shadowList && !context->printing();
-    if (hasShadow)
-        context->setDrawLooper(shadowList->createDrawLooper(DrawLooperBuilder::ShadowIgnoresAlpha, horizontal));
-
     TextRunPaintInfo textRunPaintInfo(textRun);
     textRunPaintInfo.bounds = boxRect;
     if (startOffset <= endOffset) {
@@ -411,9 +411,6 @@ static void paintTextWithShadows(GraphicsContext* context,
                 context->drawEmphasisMarks(font, textRunPaintInfo, emphasisMark, textOrigin + IntSize(0, emphasisMarkOffset));
         }
     }
-
-    if (hasShadow)
-        context->clearDrawLooper();
 }
 
 static void paintEmphasisMark(GraphicsContext* context,
@@ -424,17 +421,19 @@ static void paintEmphasisMark(GraphicsContext* context,
     const FloatPoint& textOrigin, const FloatRect& boxRect, bool horizontal)
 {
     ASSERT(!emphasisMark.isEmpty());
-    updateGraphicsContext(context, emphasisMarkColor, textStrokeColor, textStrokeWidth);
+    updateGraphicsContext(context, emphasisMarkColor, textStrokeColor, textStrokeWidth, textShadow, horizontal);
 
     if (combinedText) {
         DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun, (&objectReplacementCharacter, 1));
         FloatPoint emphasisMarkTextOrigin(boxRect.x() + boxRect.width() / 2, boxRect.y() + font.fontMetrics().ascent());
         context->concatCTM(InlineTextBox::rotation(boxRect, InlineTextBox::Clockwise));
-        paintTextWithShadows(context, combinedText->originalFont(), objectReplacementCharacterTextRun, emphasisMark, emphasisMarkOffset, 0, 1, 1, emphasisMarkTextOrigin, boxRect, textShadow, horizontal);
+        paintText(context, combinedText->originalFont(), objectReplacementCharacterTextRun, emphasisMark, emphasisMarkOffset, 0, 1, 1, emphasisMarkTextOrigin, boxRect);
         context->concatCTM(InlineTextBox::rotation(boxRect, InlineTextBox::Counterclockwise));
     } else {
-        paintTextWithShadows(context, font, textRun, emphasisMark, emphasisMarkOffset, startOffset, endOffset, paintRunLength, textOrigin, boxRect, textShadow, horizontal);
+        paintText(context, font, textRun, emphasisMark, emphasisMarkOffset, startOffset, endOffset, paintRunLength, textOrigin, boxRect);
     }
+
+    context->clearShadow();

[f(malita), 2014/08/15 02:01:06]

Also, is it correct to do this unconditionally (can we ever inherit a shadow
looper and accidentally clear it here)? Not really introduced in this patch, but
at least previously we were only clearing when we had a local shadow.

[f(malita), 2014/08/15 02:01:06]

This feels unbalanced: we're setting up shadows in updateGC but clearing them in
the parent frame.

Is it possible to push the setup logic up to the same level (in paint() maybe)?

[jbroman, 2014/08/19 18:00:43]

We'd have a number of issues were this possible. I'm not aware of any way this
can happen in the HTML case, at least. I don't think it can for SVG either, but
I don't know SVG well.

(I do wish GraphicsContext were less stateful so we didn't even have to ask
questions like these.)

[jbroman, 2014/08/19 18:00:43]

I tried to move this, but it's logic common to painting the text and the
decorations, and I couldn't find a clean way. What I have done is refactored to
use GraphicsContextStateSaver. I'd originally put the exact condition for saving
(textStroke || shadow) in there, but in the spirit of this comment, I'm now
passing the GCSS pointer and letting updateGraphicsContext choose whether a
reset will be required.

I'm not quite thrilled about the latter; what do you think?

 }
 
 bool InlineTextBox::getEmphasisMarkPosition(RenderStyle* style, TextEmphasisPosition& emphasisPosition) const
@@ -692,8 +691,8 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
         // effect, so only when we know we're stroking, do a save/restore.
         GraphicsContextStateSaver stateSaver(*context, textStyle.strokeWidth > 0);
 
-        updateGraphicsContext(context, textStyle.fillColor, textStyle.strokeColor, textStyle.strokeWidth);
-        paintTextWithShadows(context, font, textRun, nullAtom, 0, startOffset, endOffset, length, textOrigin, boxRect, textStyle.shadow, isHorizontal());
+        updateGraphicsContext(context, textStyle.fillColor, textStyle.strokeColor, textStyle.strokeWidth, textStyle.shadow, isHorizontal());
+        paintText(context, font, textRun, nullAtom, 0, startOffset, endOffset, length, textOrigin, boxRect);
 
         if (!emphasisMark.isEmpty())
             paintEmphasisMark(context, emphasisMark, emphasisMarkOffset, startOffset, endOffset, length, font, textStyle.emphasisMarkColor, textStyle.strokeColor, textStyle.strokeWidth, textStyle.shadow, combinedText, textRun, textOrigin, boxRect, isHorizontal());
@@ -703,8 +702,8 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
         // paint only the text that is selected
         GraphicsContextStateSaver stateSaver(*context, selectionStyle.strokeWidth > 0);
 
-        updateGraphicsContext(context, selectionStyle.fillColor, selectionStyle.strokeColor, selectionStyle.strokeWidth);
-        paintTextWithShadows(context, font, textRun, nullAtom, 0, sPos, ePos, length, textOrigin, boxRect, selectionStyle.shadow, isHorizontal());
+        updateGraphicsContext(context, selectionStyle.fillColor, selectionStyle.strokeColor, selectionStyle.strokeWidth, selectionStyle.shadow, isHorizontal());
+        paintText(context, font, textRun, nullAtom, 0, sPos, ePos, length, textOrigin, boxRect);
 
         if (!emphasisMark.isEmpty())
             paintEmphasisMark(context, emphasisMark, emphasisMarkOffset, sPos, ePos, length, font, selectionStyle.emphasisMarkColor, selectionStyle.strokeColor, textStyle.strokeWidth, selectionStyle.shadow, combinedText, textRun, textOrigin, boxRect, isHorizontal());
@@ -713,7 +712,7 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
     // Paint decorations
     TextDecoration textDecorations = styleToUse->textDecorationsInEffect();
     if (textDecorations != TextDecorationNone && paintInfo.phase != PaintPhaseSelection) {
-        updateGraphicsContext(context, textStyle.fillColor, textStyle.strokeColor, textStyle.strokeWidth);
+        updateGraphicsContext(context, textStyle.fillColor, textStyle.strokeColor, textStyle.strokeWidth, textStyle.shadow, isHorizontal());
         if (combinedText)
             context->concatCTM(rotation(boxRect, Clockwise));
         paintDecoration(context, boxOrigin, textDecorations, textStyle.shadow);
@@ -721,6 +720,10 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
             context->concatCTM(rotation(boxRect, Counterclockwise));
     }
 
+    // Text and decoration painting may have used shadows.
+    // It might be cleaner to use a GraphicsContextStateSaver.

[f(malita), 2014/08/15 02:01:06]

If shadows require nesting semantics we should definitely use a saver. I'm not
sure they do though...

[jbroman, 2014/08/19 18:00:43]

I don't think they do. There's certainly no layout test that includes such a
case.

+    context->clearDrawLooper();

[f(malita), 2014/08/15 02:01:06]

This may need to happen before paint decorations above -- looks like now we
could leave a shadow looper on during paintDecoration() whereas before we did
not (also, paintDecoration seems to do its own shadow voodoo and overrides the
looper anyway -- would be nice to consolidate but not sure it's doable. maybe
that's the reason why this works).

[jbroman, 2014/08/19 18:00:43]

We couldn't, only because of the explicit clear in updateGraphicsContext.

I've removed the shadow voodoo in the decoration code (and adjusted
TestExpectations).

It has a small change in how shadows of text and decorations overlap (but within
what the spec permits, and this is a super-weird case anyhow). If we wanted to
make shadows of text decorations work more pleasantly, that would be a larger,
separate change.

+
     if (paintInfo.phase == PaintPhaseForeground) {
         paintDocumentMarkers(context, boxOrigin, styleToUse, font, false);