Make SVG painting independent of GraphicsContext's alpha state

Make SVG painting independent of GraphicsContext's alpha state

SVG painting relied on GraphicsContext's alpha state which caused a
painting regression when GraphicsContextStateSavers were removed in [1].
This patch switches SVG rendering to apply the alpha to the color
passed into fill/stroke on the context, removing the need to call
context->setAlphaAsFloat. SVG was one of two users of this state (the
other being canvas) so this patch gets us one step closer to removing
GraphicsContextState::m_alpha.

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

[pdr., 2015-02-02 21:37:51]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, fmalita@chromium.org, fs@opera.com,
schenney@chromium.org

[chrishtr, 2015-02-02 23:14:54]

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.cpp:220: m_alpha =
clampedAlphaForBlending(alpha);
This is the one place that reads m_alpha it seems:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Painting video into a transparency canvas? What is this?

[pdr., 2015-02-03 00:09:48]

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.cpp:220: m_alpha =
clampedAlphaForBlending(alpha);
On 2015/02/02 at 23:14:54, chrishtr wrote:
> This is the one place that reads m_alpha it seems:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Painting video into a transparency canvas? What is this?

GraphicsContext::preparePaintForDrawRectToRect is probably the most important
user. I think this is primarily for canvas where you can control the global
alpha state. Instead of passing the alpha through all of the drawImage calls, we
just set it on the canvas and then read it before drawing images/videos.

[f(malita), 2015-02-03 00:33:28]

+1000 for getting rid of global alpha.

(nit: [1] reference missing in description)

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp:64:
context.setFillColor(GraphicsContext::combineWithAlpha(m_color.rgb(),
svgStyle.fillOpacity()));
Implicit SkColor -> Color conversion seems fragile. OTOH this is probably not
the only place where we do this. Methinks Color needs to die a quick death.

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.h:150: static SkColor
combineWithAlpha(SkColor color, float alpha)
Is there any reason to keep these helpers in GC/GCS? They don't have anything to
do with GC and should probably be in SkiaUtils.

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.h:152: return
combineWithAlpha(color, clampedAlphaForBlending(alpha));
Naming nit: "combineWithAlpha" sounds pretty weird. How about applyAlpha or
applyOpacity?

[chrishtr, 2015-02-03 00:34:16]

lgtm

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:283: void
GraphicsContext::setFillGradient(PassRefPtr<Gradient> gradient, float
additionalAlpha)
Why "additional" alpha?

[fs, 2015-02-03 10:08:49]

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp:51: void
SVGPaintServer::apply(GraphicsContext& context, RenderSVGResourceMode
resourceMode, const SVGRenderStyle& svgStyle, GraphicsContextStateSaver&
stateSaver)
According to my doctrine this would rather be 'float paintAlpha' or some such.
(And I do find having to pass RenderSVGResourceMode here at all a bit of a
nuisance...)
(Only to consider if you do end up making other changes anyway though...)

[pdr., 2015-02-04 04:04:30]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-02-04 04:04:31]

Thanks for the reivews

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp:51: void
SVGPaintServer::apply(GraphicsContext& context, RenderSVGResourceMode
resourceMode, const SVGRenderStyle& svgStyle, GraphicsContextStateSaver&
stateSaver)
On 2015/02/03 at 10:08:49, fs wrote:
> According to my doctrine this would rather be 'float paintAlpha' or some such.
(And I do find having to pass RenderSVGResourceMode here at all a bit of a
nuisance...)
> (Only to consider if you do end up making other changes anyway though...)

Done

https://codereview.chromium.org/900463002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGResourcePaintServer.cpp:64:
context.setFillColor(GraphicsContext::combineWithAlpha(m_color.rgb(),
svgStyle.fillOpacity()));
On 2015/02/03 at 00:33:28, f(malita) wrote:
> Implicit SkColor -> Color conversion seems fragile. OTOH this is probably not
the only place where we do this. Methinks Color needs to die a quick death.

+1

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:283: void
GraphicsContext::setFillGradient(PassRefPtr<Gradient> gradient, float
additionalAlpha)
On 2015/02/03 at 00:34:16, chrishtr wrote:
> Why "additional" alpha?

I thought it might disambiguate that the alpha can be combined with the alpha
already present in the gradient. Lets go with just alpha though.

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.h:150: static SkColor
combineWithAlpha(SkColor color, float alpha)
On 2015/02/03 at 00:33:28, f(malita) wrote:
> Is there any reason to keep these helpers in GC/GCS? They don't have anything
to do with GC and should probably be in SkiaUtils.

Moved to the other dumping ground, SkiaUtils :)

https://codereview.chromium.org/900463002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextState.h:152: return
combineWithAlpha(color, clampedAlphaForBlending(alpha));
On 2015/02/03 at 00:33:28, f(malita) wrote:
> Naming nit: "combineWithAlpha" sounds pretty weird. How about applyAlpha or
applyOpacity?

After much fighting, I renamed this to multiplyAlpha. I originally wanted
applyAlpha but the compiler can't look up the correct function since we're
calling a global applyAlpha from a local function by the same name which
requires some ugly scoping to disambiguate the call.

[commit-bot: I haz the power, 2015-02-04 04:04:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/900463002/20001

[commit-bot: I haz the power, 2015-02-04 05:53:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 05:53:03]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/49166)

[pdr., 2015-02-04 05:54:32]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-02-04 05:55:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/900463002/20001

[commit-bot: I haz the power, 2015-02-04 07:55:33]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189465