Move the shadows out of paintTextWithShadows.

Source/core/rendering/InlineTextBox.cpp

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 311 matching lines @@
     return -1;
 }
 
 static Color textColorForWhiteBackground(Color textColor)
 {
     int distanceFromWhite = differenceSquared(textColor, Color::white);
     // semi-arbitrarily chose 65025 (255^2) value here after a few tests;
     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) {
         TextDrawingModeFlags newMode = mode | TextModeStroke;
         if (mode != newMode) {
             context->setTextDrawingMode(newMode);
             mode = newMode;
         }
     }
 
     if (mode & TextModeFill && fillColor != context->fillColor())
         context->setFillColor(fillColor);
 
     if (mode & TextModeStroke) {
         if (strokeColor != context->strokeColor())
             context->setStrokeColor(strokeColor);
         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
 {
     return renderer().isBR() || (renderer().style()->preserveNewline() && len() == 1 && (*renderer().text().impl())[start()] == '\n');
 }
 
 bool InlineTextBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit /* lineTop */, LayoutUnit /*lineBottom*/)
 {
     if (isLineBreak())
         return false;
 
     FloatPoint boxOrigin = locationIncludingFlipping();
     boxOrigin.moveBy(accumulatedOffset);
     FloatRect rect(boxOrigin, size());
     if (m_truncation != cFullTruncation && visibleToHitTestRequest(request) && locationInContainer.intersects(rect)) {
         renderer().updateHitTestResult(result, flipForWritingMode(locationInContainer.point() - toLayoutSize(accumulatedOffset)));
         if (!result.addNodeToRectBasedTestResult(renderer().node(), request, locationInContainer, rect))
             return true;
     }
     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) {
         textRunPaintInfo.from = startOffset;
         textRunPaintInfo.to = endOffset;
         if (emphasisMark.isEmpty())
             context->drawText(font, textRunPaintInfo, textOrigin);
         else
             context->drawEmphasisMarks(font, textRunPaintInfo, emphasisMark, textOrigin + IntSize(0, emphasisMarkOffset));
     } else {
         if (endOffset > 0) {
             textRunPaintInfo.from = 0;
             textRunPaintInfo.to = endOffset;
             if (emphasisMark.isEmpty())
                 context->drawText(font, textRunPaintInfo, textOrigin);
             else
                 context->drawEmphasisMarks(font, textRunPaintInfo, emphasisMark, textOrigin + IntSize(0, emphasisMarkOffset));
         }
         if (startOffset < truncationPoint) {
             textRunPaintInfo.from = startOffset;
             textRunPaintInfo.to = truncationPoint;
             if (emphasisMark.isEmpty())
                 context->drawText(font, textRunPaintInfo, textOrigin);
             else
                 context->drawEmphasisMarks(font, textRunPaintInfo, emphasisMark, textOrigin + IntSize(0, emphasisMarkOffset));
         }
     }
-
-    if (hasShadow)
-        context->clearDrawLooper();
 }
 
 static void paintEmphasisMark(GraphicsContext* context,
     const AtomicString& emphasisMark, int emphasisMarkOffset,
     int startOffset, int endOffset, int paintRunLength,
     const Font& font, Color emphasisMarkColor, Color textStrokeColor, float textStrokeWidth, const ShadowList* textShadow,
     RenderCombineText* combinedText, const TextRun& textRun,
     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
 {
     // This function returns true if there are text emphasis marks and they are suppressed by ruby text.
     if (style->textEmphasisMark() == TextEmphasisMarkNone)
         return false;
 
     emphasisPosition = style->textEmphasisPosition();
     if (emphasisPosition == TextEmphasisPositionUnder)
@@ skipping 237 matching lines @@
         int endOffset = length;
         if (paintSelectedTextSeparately && ePos > sPos) {
             startOffset = ePos;
             endOffset = sPos;
         }
 
         // For stroked painting, we have to change the text drawing mode.  It's probably dangerous to leave that mutated as a side
         // 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());
     }
 
     if ((paintSelectedTextOnly || paintSelectedTextSeparately) && sPos < ePos) {
         // 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());
     }
 
     // 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);
         if (combinedText)
             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);
 
         // Paint custom underlines for compositions.
         if (useCustomUnderlines) {
             const Vector<CompositionUnderline>& underlines = renderer().frame()->inputMethodController().customCompositionUnderlines();
             CompositionUnderlineRangeFilter filter(underlines, start(), end());
             for (CompositionUnderlineRangeFilter::ConstIterator it = filter.begin(); it != filter.end(); ++it) {
                 if (it->color == Color::transparent)
                     continue;
@@ skipping 792 matching lines @@
     printedCharacters = fprintf(stderr, "\t%s %p", obj.renderName(), &obj);
     const int rendererCharacterOffset = 24;
     for (; printedCharacters < rendererCharacterOffset; printedCharacters++)
         fputc(' ', stderr);
     fprintf(stderr, "(%d,%d) \"%s\"\n", start(), start() + len(), value.utf8().data());
 }
 
 #endif
 
 } // namespace blink