Refactor shadow rendering logic.

Refactor shadow rendering logic to take better advantage of the SkDrawLooper abstraction in Skia. Change text-shadow to use this newly added WebCore::DrawLooper.

A quick synthetic web page which exercises text with multiple text-shadows suggests that doing this is a significant performance win (with impl-side painting, this benefits main thread record time the most). See bug for details.

A clip which unnecessary removed 1px from the border of some shadows was affected; affected layout tests will need re-baselining.

BUG=240297
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150805

[jbroman, 2013-05-17 15:10:56]

Now that non-Skia platforms don't exist anymore, we don't need to work around
the limitations of other WK graphics backends.

This CL starts to change logic for rendering multiple shadows to use a single
SkDrawLooper (as opposed to repeatedly rendering the text and clipping out
certain copies of it). A WebCore wrapper is added to avoid leaking Skia types
outside platform/.

[Stephen Chennney, 2013-05-17 15:35:34]

There is code in ImageSkia.cpp that grabs the draw looper in GraphicsContext and
adds it to a paint. So right now you have set/get for DrawLooper and set/get for
SkDrawLooper. They should be unified, which you can do in a follow up patch or
in this one. If you want to do it in a follow-up, file a bug so we have
something to track.

https://codereview.chromium.org/15137009/diff/26013/Source/core/platform/grap...
File Source/core/platform/graphics/DrawLooper.cpp (right):

https://codereview.chromium.org/15137009/diff/26013/Source/core/platform/grap...
Source/core/platform/graphics/DrawLooper.cpp:65: // for perf reasons.
Take this out. We clearly haven't seen problems in the wild, so I don't see an y
reason to keep this TODO.

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:58: bool hasShadow = !!shadow;
Why !!? To avoid a perf warning on Windows?

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:64: } while ((shadow = shadow->next()));
Why do you create and fill a vector then loop over it then delete it. Why not
create add the shadows directly while walking the shadow list?

[jbroman, 2013-05-17 15:43:18]

https://codereview.chromium.org/15137009/diff/26013/Source/core/platform/grap...
File Source/core/platform/graphics/DrawLooper.cpp (right):

https://codereview.chromium.org/15137009/diff/26013/Source/core/platform/grap...
Source/core/platform/graphics/DrawLooper.cpp:65: // for perf reasons.
On 2013/05/17 15:35:34, Stephen Chenney wrote:
> Take this out. We clearly haven't seen problems in the wild, so I don't see an
y
> reason to keep this TODO.

Sure. I just migrated it from the old home of this code; didn't want to lose a
TODO I didn't know anything about.

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:58: bool hasShadow = !!shadow;
On 2013/05/17 15:35:34, Stephen Chenney wrote:
> Why !!? To avoid a perf warning on Windows?

Some people prefer to add !! to say "Hey, this is a boolean! I'm not
accidentally assigning a pointer to a boolean!". I'm happy to get rid of it.

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:64: } while ((shadow = shadow->next()));
On 2013/05/17 15:35:34, Stephen Chenney wrote:
> Why do you create and fill a vector then loop over it then delete it. Why not
> create add the shadows directly while walking the shadow list?

Because ShadowData is a linked list of the shadows from bottom to top, and
SkDrawLooper adds layers from top to bottom. This is what the FIXME is referring
to; it would be nice if I didn't have to reverse the list before adding the
shadows (possibly by having the style resolution code link it in the opposite
order, but I don't know how other uses of ShadowData would be affected by doing
that).

[jbroman, 2013-05-17 19:16:55]

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15137009/diff/26013/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:64: } while ((shadow = shadow->next()));
On 2013/05/17 15:43:18, jbroman wrote:
> On 2013/05/17 15:35:34, Stephen Chenney wrote:
> > Why do you create and fill a vector then loop over it then delete it. Why
not
> > create add the shadows directly while walking the shadow list?
> 
> Because ShadowData is a linked list of the shadows from bottom to top, and
> SkDrawLooper adds layers from top to bottom. This is what the FIXME is
referring
> to; it would be nice if I didn't have to reverse the list before adding the
> shadows (possibly by having the style resolution code link it in the opposite
> order, but I don't know how other uses of ShadowData would be affected by
doing
> that).

Another, possibly easier option, would be to get a method added to
SkLayerDrawLooper which allows prepending layers. That might prove easier and
more generally useful.

[Stephen Chennney, 2013-05-17 19:45:50]

On 2013/05/17 19:16:55, jbroman wrote:
> Another, possibly easier option, would be to get a method added to
> SkLayerDrawLooper which allows prepending layers. That might prove easier and
> more generally useful.

That was my thought. Maybe file something on the Skia tracker and indicate that
this part of Blink would benefit.

[Stephen Chennney, 2013-05-17 19:47:13]

lgtm

[commit-bot: I haz the power, 2013-05-17 19:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/15137009/28002

[commit-bot: I haz the power, 2013-05-17 21:21:51]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-21 14:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/15137009/28002

[commit-bot: I haz the power, 2013-05-21 14:52:14]

Failed to apply patch for Source/core/rendering/InlineTextBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/InlineTextBox.cpp
  Hunk #3 succeeded at 420 (offset 8 lines).
  Hunk #4 succeeded at 430 (offset 8 lines).
  Hunk #5 FAILED at 1174.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/rendering/InlineTextBox.cpp.rej

Patch:       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
3aa2c706bab014733dc8fab25043773941d762b7..0a3fc4dd0338f849f9f2e02a299abc3fae70b73f
100644
--- a/Source/core/rendering/InlineTextBox.cpp
+++ b/Source/core/rendering/InlineTextBox.cpp
@@ -34,6 +34,7 @@
 #include "core/page/Frame.h"
 #include "core/page/Page.h"
 #include "core/page/Settings.h"
+#include "core/platform/graphics/DrawLooper.h"
 #include "core/platform/graphics/FontCache.h"
 #include "core/platform/graphics/GraphicsContextStateSaver.h"
 #include "core/rendering/EllipsisBox.h"
@@ -47,8 +48,10 @@
 #include "core/rendering/RenderRubyText.h"
 #include "core/rendering/RenderTheme.h"
 #include "core/rendering/break_lines.h"
+#include "core/rendering/style/ShadowData.h"
 #include "core/rendering/svg/SVGTextRunRenderingContext.h"
-#include <wtf/text/CString.h>
+#include "wtf/Vector.h"
+#include "wtf/text/CString.h"
 
 using namespace std;
 
@@ -409,7 +412,7 @@ FloatSize
InlineTextBox::applyShadowToGraphicsContext(GraphicsContext* context,
         shadowOffset -= extraOffset;
     }
 
-    context->setShadow(shadowOffset, shadowBlur, shadowColor,
context->fillColorSpace());
+    context->setShadow(shadowOffset, shadowBlur, shadowColor);
     return extraOffset;
 }
 
@@ -419,57 +422,59 @@ static void paintTextWithShadows(GraphicsContext* context,
const Font& font, con
                                  const FloatPoint& textOrigin, const FloatRect&
boxRect,
                                  const ShadowData* shadow, bool stroked, bool
horizontal)
 {
+    bool hasShadow = shadow;
     Color fillColor = context->fillColor();
-    ColorSpace fillColorSpace = context->fillColorSpace();
-    bool opaque = fillColor.alpha() == 255;
-    if (!opaque)
-        context->setFillColor(Color::black, fillColorSpace);
+
+    if (hasShadow) {
+        // FIXME: it would be better if we could get the shadows top-to-bottom
from the style.
+        Vector<const ShadowData*, 4> shadows;
+        do {
+            shadows.append(shadow);
+        } while ((shadow = shadow->next()));
+
+        DrawLooper drawLooper;
+        drawLooper.addUnmodifiedContent();
+        for (int i = shadows.size() - 1; i >= 0; i--) {
+            shadow = shadows[i];
+            int shadowX = horizontal ? shadow->x() : shadow->y();
+            int shadowY = horizontal ? shadow->y() : -shadow->x();
+            FloatSize offset(shadowX, shadowY);
+            drawLooper.addShadow(offset, shadow->blur(), shadow->color(),
+                DrawLooper::ShadowRespectsTransforms,
DrawLooper::ShadowIgnoresAlpha);
+        }
+        context->setDrawLooper(drawLooper);
+    }
 
     TextRunPaintInfo textRunPaintInfo(textRun);
     textRunPaintInfo.bounds = boxRect;
-    do {
-        IntSize extraOffset;
-        if (shadow)
-            extraOffset =
roundedIntSize(InlineTextBox::applyShadowToGraphicsContext(context, shadow,
boxRect, stroked, opaque, horizontal));
-        else if (!opaque)
-            context->setFillColor(fillColor, fillColorSpace);
-
-        if (startOffset <= endOffset) {
-            textRunPaintInfo.from = startOffset;
+    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 +
extraOffset);
+                context->drawText(font, textRunPaintInfo, textOrigin);
             else
-                context->drawEmphasisMarks(font, textRunPaintInfo,
emphasisMark, textOrigin + extraOffset + IntSize(0, emphasisMarkOffset));
-        } else {
-            if (endOffset > 0) {
-                textRunPaintInfo.from = 0;
-                textRunPaintInfo.to = endOffset;
-                if (emphasisMark.isEmpty())
-                    context->drawText(font, textRunPaintInfo, textOrigin +
extraOffset);
-                else
-                    context->drawEmphasisMarks(font, textRunPaintInfo,
emphasisMark, textOrigin + extraOffset + IntSize(0, emphasisMarkOffset));
-            }
-            if (startOffset < truncationPoint) {
-                textRunPaintInfo.from = startOffset;
-                textRunPaintInfo.to = truncationPoint;
-                if (emphasisMark.isEmpty())
-                    context->drawText(font, textRunPaintInfo, textOrigin +
extraOffset);
-                else
-                    context->drawEmphasisMarks(font, textRunPaintInfo,
emphasisMark, textOrigin + extraOffset + IntSize(0, emphasisMarkOffset));
-            }
+                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 (!shadow)
-            break;
-
-        if (shadow->next() || stroked || !opaque)
-            context->restore();
-        else
-            context->clearShadow();
-
-        shadow = shadow->next();
-    } while (shadow || stroked || !opaque);
+    if (hasShadow)
+        context->clearDrawLooper();
 }
 
 bool InlineTextBox::getEmphasisMarkPosition(RenderStyle* style,
TextEmphasisPosition& emphasisPosition) const
@@ -1169,7 +1174,7 @@ void InlineTextBox::paintDecoration(GraphicsContext*
context, const FloatPoint&
             }
             int shadowX = isHorizontal() ? shadow->x() : shadow->y();
             int shadowY = isHorizontal() ? shadow->y() : -shadow->x();
-            context->setShadow(FloatSize(shadowX, shadowY - extraOffset),
shadow->blur(), shadow->color(), colorSpace);
+            context->setShadow(FloatSize(shadowX, shadowY - extraOffset),
shadow->blur(), shadow->color());
             setShadow = true;
             shadow = shadow->next();
         }

[commit-bot: I haz the power, 2013-05-21 15:45:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/15137009/54001

[commit-bot: I haz the power, 2013-05-21 16:24:55]

Change committed as 150805

[eseidel, 2013-05-21 22:45:29]

This appears to have caused a change in behavior in
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...

[danakj, 2013-05-21 22:51:40]

On 2013/05/21 22:45:29, eseidel wrote:
> This appears to have caused a change in behavior in
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...

There's a single pixel different in the 10.8 diff, it looks like it needs a
rebaseline to me. I'll do that. Must have missed it when I rebaselined the
others.

[danakj, 2013-05-21 22:55:54]

On 2013/05/21 22:51:40, danakj wrote:
> On 2013/05/21 22:45:29, eseidel wrote:
> > This appears to have caused a change in behavior in
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...
> 
> There's a single pixel different in the 10.8 diff, it looks like it needs a
> rebaseline to me. I'll do that. Must have missed it when I rebaselined the
> others.

Looks like julien beat me to it :)