Adding the 'filter' context attribute to 2D canvas

Adding the 'filter' context attribute to 2D canvas

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

[Justin Novosad, 2015-06-19 00:48:27]

junov@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Justin Novosad, 2015-06-19 00:48:28]

WIP: there is a bug with blur and drop shadow radii, and I still have to write
tests.

[Justin Novosad, 2015-06-22 06:17:53]

New patch.

[Stephen White, 2015-06-22 15:28:54]

https://codereview.chromium.org/1194733002/diff/40001/Source/core/css/resolve...
File Source/core/css/resolver/FilterOperationResolver.cpp (right):

https://codereview.chromium.org/1194733002/diff/40001/Source/core/css/resolve...
Source/core/css/resolver/FilterOperationResolver.cpp:186: Color shadowColor =
Color::black;
As discussed, it would be great to have an SVG filters test for this as well.

https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:853: // TODO(junov):
crbug.com/502921 We could use primitive bounds if we knew that the filter
There's actually a traversal to determine the minimum paint rect in the
FilterEffect DAG. The entry point is FilterEffect::determineAbsolutePaintRect(),
IIRC. It uses the virtuals mapPaintRect() and affectsTransparentPixels(). At
some point we should move it to Skia. But maybe you could use
affectsTransparentPixels() for your purposes, at a minimum.

https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:341: // StrokePaint
respectively for filters that refernece SVG.
Nit: reference

https://codereview.chromium.org/1194733002/diff/40001/Source/core/paint/Filte...
File Source/core/paint/FilterEffectBuilder.h (right):

https://codereview.chromium.org/1194733002/diff/40001/Source/core/paint/Filte...
Source/core/paint/FilterEffectBuilder.h:54: ApplyScale,
<bikedshed> Since this uses the effectiveZoom(), we should call these ApplyZoom
and IgnoreZoom. </bikeshed>

Or maybe we should just pass in a (float) zoom value, and let the caller figure
it out.

[Justin Novosad, 2015-06-23 15:06:28]

New patch

On 2015/06/22 15:28:54, Stephen White wrote:
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/css/resolve...
> File Source/core/css/resolver/FilterOperationResolver.cpp (right):
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/css/resolve...
> Source/core/css/resolver/FilterOperationResolver.cpp:186: Color shadowColor =
> Color::black;
> As discussed, it would be great to have an SVG filters test for this as well.

Turns out there already was a test for this, but it was expecting the wrong
thing: css3/filters/filter-property-computed-style.html
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:853: // TODO(junov):
> crbug.com/502921 We could use primitive bounds if we knew that the filter
> There's actually a traversal to determine the minimum paint rect in the
> FilterEffect DAG. The entry point is
FilterEffect::determineAbsolutePaintRect(),
> IIRC. It uses the virtuals mapPaintRect() and affectsTransparentPixels(). At
> some point we should move it to Skia. But maybe you could use
> affectsTransparentPixels() for your purposes, at a minimum.

I was planning in optimizing this in a follow-up CL. Ok?
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2DState.cpp:341: // StrokePaint
> respectively for filters that refernece SVG.
> Nit: reference

Done.
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/paint/Filte...
> File Source/core/paint/FilterEffectBuilder.h (right):
> 
>
https://codereview.chromium.org/1194733002/diff/40001/Source/core/paint/Filte...
> Source/core/paint/FilterEffectBuilder.h:54: ApplyScale,
> <bikedshed> Since this uses the effectiveZoom(), we should call these
ApplyZoom
> and IgnoreZoom. </bikeshed>
> 
> Or maybe we should just pass in a (float) zoom value, and let the caller
figure
> it out.

Done.

[Stephen White, 2015-06-25 18:30:27]

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-default-color.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-default-color.html:5: ctx.filter =
'drop-shadow(0 10px)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-drop-shadow-transformed.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-drop-shadow-transformed.html:7: ctx.filter
= 'drop-shadow(0 10px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-drop-shadow.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-drop-shadow.html:5: ctx.filter =
'drop-shadow(0 10px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-global-alpha.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-global-alpha.html:5: ctx.filter =
'drop-shadow(0 20px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-global-composite-operation.html
(right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-global-composite-operation.html:8:
ctx.filter = 'drop-shadow(0 10px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-liveness.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-liveness.html:6: ctx.filter =
'drop-shadow(0 .5em black)';
Nit: 0 -> 0em

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-multiple-drop-shadows.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-multiple-drop-shadows.html:6: ctx.filter =
'drop-shadow(0 10px black) drop-shadow(10px 0 #ccc)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-shadow.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-shadow.html:6: ctx.filter = 'drop-shadow(0
10px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-svg-inline.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-svg-inline.html:4: <fegaussianblur
in="SourceAlpha" stdDeviation="0"></fegaussianblur>
Nit: stddev = 0 => perhaps this node is not needed?

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-units-em.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-units-em.html:11: ctx.filter =
'drop-shadow(0 .5em black)';
Nit: 0 -> 0em

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-units-off-screen.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-units-off-screen.html:9: ctx.filter =
'drop-shadow(0 50px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-filter-units.html (right):

https://codereview.chromium.org/1194733002/diff/120001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-filter-units.html:5: ctx.filter = 'drop-shadow(0
50px black)';
Nit: 0 -> 0px

https://codereview.chromium.org/1194733002/diff/120001/Source/core/css/resolv...
File Source/core/css/resolver/FilterOperationResolver.cpp (right):

https://codereview.chromium.org/1194733002/diff/120001/Source/core/css/resolv...
Source/core/css/resolver/FilterOperationResolver.cpp:186: Color shadowColor =
Color::black;
Thanks for this catch, BTW!

https://codereview.chromium.org/1194733002/diff/120001/Source/core/html/canva...
File Source/core/html/canvas/CanvasRenderingContext2DState.cpp (right):

https://codereview.chromium.org/1194733002/diff/120001/Source/core/html/canva...
Source/core/html/canvas/CanvasRenderingContext2DState.cpp:326: SkImageFilter*
CanvasRenderingContext2DState::filter(Element* styleResolutionHost, const Font&
font) const
Naming nit: I know this is meant to look like a getter, but to me the arguments
kind of give it away as a "build something with these arguments and cache it".
So maybe it should have "get" or "build" in the name. Otherwise it could look
like it's meant to apply a filter to its arguments.

[Justin Novosad, 2015-06-26 18:58:25]

New Patch. Applied all suggested corrections.

[Stephen White, 2015-06-26 19:25:58]

LGTM.

Really excited to see this go in!

[Justin Novosad, 2015-06-26 19:33:55]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-06-26 19:34:13]

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

[commit-bot: I haz the power, 2015-06-26 20:12:49]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197938