Adds filter support for offscreen canvas

This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a Document,
that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2326633002
Cr-Commit-Position: refs/heads/master@{#447023}
Committed: https://chromium.googlesource.com/chromium/src/+/ee25559e75c9fa03648b4fcaf52f162baa243d2a

[fserb, 2016-09-12 19:55:40]

Description was changed from

==========
stuff


filter available, has/get


test

BUG=
==========

to

==========
stuff


filter available, has/get


test

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fserb, 2016-09-12 19:57:55]

Description was changed from

==========
stuff


filter available, has/get


test

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document, that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fserb, 2016-09-12 19:58:14]

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

[Justin Novosad, 2016-09-12 20:41:37]

junov@chromium.org changed reviewers:
+ dstockwell@chromium.org, esprehn@chromium.org

[Justin Novosad, 2016-09-12 20:41:37]

+dstockwell and esprehn to take a look at the CSS parts, which I am not familiar
enough with.

FYI: This CL results in parts of the style resolution code being invoked on a
Worker thread, without a document.  Some of this is a bit hacky, but it works.
If you have a better ideas, please share.

[esprehn, 2016-09-12 20:43:38]

not lgtm, the resolver is not thread safe, and putting a bunch of null data in
there and using it from the background thread doesn't look right at all. We
can't go through the resolver at all on a background thread, OffscreenCanvas is
going to need to do something else.

[esprehn, 2016-09-12 20:53:12]

I'd really like to see a design doc for OffscreenCanvas. I feel like there's
some big misunderstandings here where you're trying to just put TLS all over the
place and hack it up. If it was that easy we would have multi threaded the
engine a long time ago. :)

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
(right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:47:
ElementStyleResources::ElementStyleResources()
All the methods crash

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FontBuilder.h (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FontBuilder.h:44: FontBuilder();
A FontBuilder without a document crashes all over the place

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:160: if (!element)
In general core/paint is not thread safe, this code would need to be refactored
out, someone will just break it.

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:115:
ComputedStyle::ComputedStyle()
Why? Performance here is pretty important. :)

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:400: new
ThreadSpecific<ComputedStyle*>());
Can't put TLS here, it's too slow

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/NinePieceImage.cpp (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/NinePieceImage.cpp:33: new
ThreadSpecific<DataRef<NinePieceImageData>>);
ditto

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/SVGComputedStyle.cpp (right):

https://codereview.chromium.org/2326633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/SVGComputedStyle.cpp:36:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<SVGComputedStyle*>, style,
ditto

[Justin Novosad, 2016-09-12 20:53:30]

On 2016/09/12 20:43:38, esprehn wrote:
> not lgtm, the resolver is not thread safe, and putting a bunch of null data in
> there and using it from the background thread doesn't look right at all. We
> can't go through the resolver at all on a background thread, OffscreenCanvas
is
> going to need to do something else.

All we are trying to do is get a string parsed that describes a css filter, can
you think of a reasonable way to access CSS parsing without going through the
resolver?

[esprehn, 2016-09-12 20:56:16]

On 2016/09/12 at 20:53:30, junov wrote:
> On 2016/09/12 20:43:38, esprehn wrote:
> > not lgtm, the resolver is not thread safe, and putting a bunch of null data
in
> > there and using it from the background thread doesn't look right at all. We
> > can't go through the resolver at all on a background thread, OffscreenCanvas
is
> > going to need to do something else.
> 
> All we are trying to do is get a string parsed that describes a css filter,
can you think of a reasonable way to access CSS parsing without going through
the resolver?

Invoke the CSSParser directly then? CSSPropertyParser::parseSingleValue?

[Justin Novosad, 2016-09-12 20:59:38]

On 2016/09/12 20:53:12, esprehn wrote:
> I'd really like to see a design doc for OffscreenCanvas. I feel like there's
> some big misunderstandings here where you're trying to just put TLS all over
the
> place and hack it up. If it was that easy we would have multi threaded the
> engine a long time ago. :)

The general problem of style resolution is orders of magnitude harder to do in a
thread than what we are trying to do here.  In this context, there is no
cascading of styles, there is no DOM, all we really need is the ability to parse
a single style value in isolation. In this case, a string that describes a
sequence of filters.

[fserb, 2016-10-24 18:05:21]

On 2016/09/12 at 20:59:38, junov wrote:
> On 2016/09/12 20:53:12, esprehn wrote:
> > I'd really like to see a design doc for OffscreenCanvas. I feel like there's
> > some big misunderstandings here where you're trying to just put TLS all over
the
> > place and hack it up. If it was that easy we would have multi threaded the
> > engine a long time ago. :)
> 
> The general problem of style resolution is orders of magnitude harder to do in
a thread than what we are trying to do here.  In this context, there is no
cascading of styles, there is no DOM, all we really need is the ability to parse
a single style value in isolation. In this case, a string that describes a
sequence of filters.

PTAL

[Justin Novosad, 2016-10-24 18:51:23]

I feel we are getting closer.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:122:
StyleResolverState* state,
This approach seems reasonable to me, but I'll defer to esprehn@ on this.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:217:
stdDeviation = Length(firstValue->getDoubleValue(), Fixed);
This part is doubtful. We probably want some kind of default converisionData to
kick in in order to get the right unit conversions. Physical units like cm/in
don't really make a lot of sense for an object that is off screen.  'em' doesn't
make a lot of sense either.  We need to think about this. Do we want to allow
only 'px' in the case of OffscreenCanvas?  I am okay with solving this in a
follow-up CL, but we can't ship  until this is resolved.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:234:
blur = item.blur ? item.blur->getIntValue() : 0;
More doubtful conversions.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:303:
nullptr, FloatRect((FloatPoint()), FloatSize(canvasSize)),
This nullptr lacks readability.  Please use a temp var or an /*inline comment*/
so we know what it is.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:98:
SkImageFilter* getFilterWithoutDocument(IntSize canvasSize) const;
I'd rather this be called getFilterForOffscreenCanvas, to make it clear that
this is a special case path that should not be used for anything else but
OffscreenCanvas.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:99:
bool hasFilterWithoutDocument(IntSize canvasSize) const;
same here.

[Justin Novosad, 2016-10-25 15:38:15]

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

[Justin Novosad, 2016-10-25 15:38:16]

+meade to review use of CSS parser off the main thread

[Justin Novosad, 2016-10-25 15:48:55]

On 2016/10/25 15:38:16, Justin Novosad wrote:
> +meade to review use of CSS parser off the main thread

@meade: the call site that invokes the parser is in
BaseRenderingContext2D::setFilter which did not change in this CL, but starting
with this CL, that method will get called off the main thread. Please let us
know whether you think there is anything unsafe about that.  Based on the tests
in this CL, we know it works, but that does not mean it is 100% safe.

[meade_UTC10, 2016-10-26 06:57:34]

I'm not super familiar with the threaded parts of this, so I'd trust Elliott's
judgement on that one. You could also as from advice from fs@opera, who also did
some work on filters relatively recently,

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:138:
continue;
This for loop is big and kind of hard to reason about, so I think adding more
complexity to it is not so great. Is there some way you can modularise what
you're trying to do here to make it easier to read?

For example, this continue statement causes everything from here to line 250 to
be skipped, not just to line 157 as I first thought. Instead there could be a
method for handling a URIValue (136-157), and another method for handling a
FunctionValue (159-25). This could make it easier to unit test the behaviour,
which is also good :)

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:291:
FilterOperationResolver::createFilterOperations(nullptr, *m_filterValue);
Nit: don't abbreviate variable names.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:305:
&fillPaintForFilter, &strokePaintForFilter);
It looks like these get stored in the FilterEffectBuilder. Is it ok that they
are temporary variables?

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:203:
return f;
Nit: This could just be

return state().getFilterWithoutDocument(getOffscreenCanvas()->size());

[fserb, 2016-10-26 19:08:26]

Thanks for the comments. I'll work on addressing the two major issues and I'll
ptal later.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:138:
continue;
On 2016/10/26 at 06:57:34, Eddy wrote:
> This for loop is big and kind of hard to reason about, so I think adding more
complexity to it is not so great. Is there some way you can modularise what
you're trying to do here to make it easier to read?
> 
> For example, this continue statement causes everything from here to line 250
to be skipped, not just to line 157 as I first thought. Instead there could be a
method for handling a URIValue (136-157), and another method for handling a
FunctionValue (159-25). This could make it easier to unit test the behaviour,
which is also good :)

True. Although there's nothing to do if there's a URIValue and no State/document
(and the specs are fine with that, it seems). We should really skip everything
and not return a proper operation if we get a url. But yes, you are right that
this makes the code a bit confusing. I'll try to address that.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:217:
stdDeviation = Length(firstValue->getDoubleValue(), Fixed);
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> This part is doubtful. We probably want some kind of default converisionData
to kick in in order to get the right unit conversions. Physical units like cm/in
don't really make a lot of sense for an object that is off screen.  'em' doesn't
make a lot of sense either.  We need to think about this. Do we want to allow
only 'px' in the case of OffscreenCanvas?  I am okay with solving this in a
follow-up CL, but we can't ship  until this is resolved.

I did try to create a deafult ConversionData. I'm up for that. But I couldn't do
anything that worked, I can try again.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:234:
blur = item.blur ? item.blur->getIntValue() : 0;
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> More doubtful conversions.

I'll try that.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:291:
FilterOperationResolver::createFilterOperations(nullptr, *m_filterValue);
On 2016/10/26 at 06:57:34, Eddy wrote:
> Nit: don't abbreviate variable names.

done

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:303:
nullptr, FloatRect((FloatPoint()), FloatSize(canvasSize)),
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> This nullptr lacks readability.  Please use a temp var or an /*inline
comment*/ so we know what it is.

done

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:305:
&fillPaintForFilter, &strokePaintForFilter);
On 2016/10/26 at 06:57:34, Eddy wrote:
> It looks like these get stored in the FilterEffectBuilder. Is it ok that they
are temporary variables?

yep. They are used the same way on getFilter(). They are only used on
buildFilterEffect and it doesn't keep anything remotely.

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:98:
SkImageFilter* getFilterWithoutDocument(IntSize canvasSize) const;
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> I'd rather this be called getFilterForOffscreenCanvas, to make it clear that
this is a special case path that should not be used for anything else but
OffscreenCanvas.

done

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:99:
bool hasFilterWithoutDocument(IntSize canvasSize) const;
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> same here.

done

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:203:
return f;
On 2016/10/26 at 06:57:34, Eddy wrote:
> Nit: This could just be
> 
> return state().getFilterWithoutDocument(getOffscreenCanvas()->size());

done.

[fserb, 2016-11-08 20:40:51]

I've done the change for ConversionData, but not for the loop. PTAL?

[Justin Novosad, 2016-11-08 20:59:23]

This look reasonable to me.
@esprehn: WDYT?

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:135:
static CSSToLengthConversionData::FontSizes fontSizes(10, 16, &font);
These hard-coded values should be in constants.  Actually we might already have
constants defined somewhere for this already?

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:144: if
(!state)
Add a comment here to explain the the !state case is for
OffscreenCanvasRenderingContext2D, which does not support reference filters.

[Justin Novosad, 2016-11-14 14:55:55]

Ping! esprehn

[esprehn, 2016-11-22 01:38:37]

You're making all shadows black... :P

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:122:
StyleResolverState* state,
On 2016/10/24 at 18:51:22, Justin Novosad wrote:
> This approach seems reasonable to me, but I'll defer to esprehn@ on this.

Lets refactor this into two methods, one that takes a state and one that doesn't
and move the state to be the second argument.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:137:
static CSSToLengthConversionData baseData(&ComputedStyle::initialStyle(),
I don't think you can do this, any number of worker threads can come through
here calling FilterOperationResolver::createFilterOperations at the same time,
initializing these structs in parallel doesn't look thread safe.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:160:
countFilterUse(operationType, state->document());
I think you actually want to take an ExecutionContext argument to this function,
workers can use the UseCounter as well, and what you're doing here means that we
have no use counter coverage for them.

We can fix that in a follow up though.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:225:
Color shadowColor = Color::black;
This is going to force all shadows to be black, that looks wrong since authors
likely want other colors for shadows. You need to probably pass in a
TextLinkColors instance and use it here, or at least replicate the the logic at
the start that does isColorValue()

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:284:
FilterOperationResolver::createFilterOperations(nullptr, *m_filterValue);
createOffscreen(...)

it's more clear than passing nullptr

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:296:
nullptr,  // styleResolutionHost
Probably want two constructors instead, add one that doesn't take a Node?

[fserb, 2017-01-17 17:58:45]

Addressed all issues.
Fixed shadow colors.
PTAL.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:135:
static CSSToLengthConversionData::FontSizes fontSizes(10, 16, &font);
On 2016/11/08 at 20:59:23, Justin Novosad wrote:
> These hard-coded values should be in constants.  Actually we might already
have constants defined somewhere for this already?

Couldn't find it anywhere. The default on CSS Font conversion is 0. Added
constants.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:137:
static CSSToLengthConversionData baseData(&ComputedStyle::initialStyle(),
On 2016/11/22 at 01:38:37, esprehn wrote:
> I don't think you can do this, any number of worker threads can come through
here calling FilterOperationResolver::createFilterOperations at the same time,
initializing these structs in parallel doesn't look thread safe.

I've removed the statics.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:144: if
(!state)
On 2016/11/08 at 20:59:23, Justin Novosad wrote:
> Add a comment here to explain the the !state case is for
OffscreenCanvasRenderingContext2D, which does not support reference filters.

done.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:160:
countFilterUse(operationType, state->document());
On 2016/11/22 at 01:38:37, esprehn wrote:
> I think you actually want to take an ExecutionContext argument to this
function, workers can use the UseCounter as well, and what you're doing here
means that we have no use counter coverage for them.
> 
> We can fix that in a follow up though.

added TODO.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:225:
Color shadowColor = Color::black;
On 2016/11/22 at 01:38:37, esprehn wrote:
> This is going to force all shadows to be black, that looks wrong since authors
likely want other colors for shadows. You need to probably pass in a
TextLinkColors instance and use it here, or at least replicate the the logic at
the start that does isColorValue()

yep. This code got refactored on StyleBuilderConverter, but I fixed there to do
the proper thing for offscreen (default to black and use the passed color).

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:284:
FilterOperationResolver::createFilterOperations(nullptr, *m_filterValue);
On 2016/11/22 at 01:38:37, esprehn wrote:
> createOffscreen(...)
> 
> it's more clear than passing nullptr

done.

https://codereview.chromium.org/2326633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:296:
nullptr,  // styleResolutionHost
On 2016/11/22 at 01:38:37, esprehn wrote:
> Probably want two constructors instead, add one that doesn't take a Node?

done.

[fserb, 2017-01-17 17:59:48]

The CQ bit was checked by fserb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 18:00:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:18:21]

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

[commit-bot: I haz the power, 2017-01-17 18:18:22]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fserb, 2017-01-17 21:31:06]

The CQ bit was checked by fserb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:31:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Novosad, 2017-01-17 21:59:47]

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:364:
// Checking for a non-null m_filterValue isn't sufficient, since this value
Good point.  Do we have a test that covers this?

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:80:
void setFilter(const String&);
I see no definition for this new method.

[fserb, 2017-01-17 22:10:42]

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:364:
// Checking for a non-null m_filterValue isn't sufficient, since this value
On 2017/01/17 at 21:59:47, Justin Novosad wrote:
> Good point.  Do we have a test that covers this?

it's just a small detail. It usually will happen when a filter fails to compile.
We do test for that.

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:80:
void setFilter(const String&);
On 2017/01/17 at 21:59:47, Justin Novosad wrote:
> I see no definition for this new method.

wait. Is this a tricky question?
It's right there, I think.

[Justin Novosad, 2017-01-17 22:49:02]

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:238:
BaseRenderingContext2D::setFilter(filterString);
Ah, here it is. Why is this needed?  It should not be necessary.  The public
methods of base classes are visible to the bindings layer.

[commit-bot: I haz the power, 2017-01-17 23:36:12]

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

[commit-bot: I haz the power, 2017-01-17 23:36:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fserb, 2017-01-18 15:47:59]

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2326633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:238:
BaseRenderingContext2D::setFilter(filterString);
On 2017/01/17 at 22:49:02, Justin Novosad wrote:
> Ah, here it is. Why is this needed?  It should not be necessary.  The public
methods of base classes are visible to the bindings layer.

yep. Sorry, it was a side effect from the process. Cleaned up.

[fserb, 2017-01-18 15:48:10]

The CQ bit was checked by fserb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 15:48:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fserb, 2017-01-18 17:10:40]

The CQ bit was unchecked by fserb@chromium.org

[fserb, 2017-01-18 17:13:07]

The CQ bit was checked by fserb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 17:13:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 18:59:58]

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

[commit-bot: I haz the power, 2017-01-18 18:59:59]

Dry run: This issue passed the CQ dry run.

[fserb, 2017-01-25 19:20:32]

Description was changed from

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document, that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document, that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[esprehn, 2017-01-27 19:43:58]

Description was changed from

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document, that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document,
that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[esprehn, 2017-01-27 19:44:37]

esprehn@chromium.org changed reviewers:
- dstockwell@chromium.org

[esprehn, 2017-01-27 19:44:39]

Are you sure returning ::black is correct? Otherwise this looks good.

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-filter.html
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-filter.html:3: <body>
you can leave out html and body

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:277:
shadow.overrideColor(Color::black);
Is this a regression? It used to do ->color() and now it always does black?

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:370:
return !!getFilterForOffscreenCanvas(canvasSize);
This is super wasteful since you do all the work to create the filter and then
throw it away. I see the non-off screen canvas one does this too though.

[fserb, 2017-01-27 19:54:47]

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-filter.html
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-filter.html:3: <body>
On 2017/01/27 at 19:44:39, esprehn wrote:
> you can leave out html and body

Done all.

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:277:
shadow.overrideColor(Color::black);
On 2017/01/27 at 19:44:39, esprehn wrote:
> Is this a regression? It used to do ->color() and now it always does black?

Well, on a regular document, you want the default color to be the default
current document color.
On an offscreen canvas the standard says the default is black.

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp
(right):

https://codereview.chromium.org/2326633002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:370:
return !!getFilterForOffscreenCanvas(canvasSize);
On 2017/01/27 at 19:44:39, esprehn wrote:
> This is super wasteful since you do all the work to create the filter and then
throw it away. I see the non-off screen canvas one does this too though.

The problem is there's no way to know if you actually have a proper filter
unless you try to compute it. I can create a bug to investigate this and try to
come up with a better architecture.

[esprehn, 2017-01-27 20:29:55]

lgtm

[fserb, 2017-01-30 17:24:34]

The CQ bit was checked by fserb@chromium.org

[fserb, 2017-01-30 17:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2326633002/#ps300001
(title: "Sync")

[commit-bot: I haz the power, 2017-01-30 17:24:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 19:00:15]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1485797074283940,
"parent_rev": "efa2fe86ca1c0ec34d1ddd3ae72fcdceb62a3129", "commit_rev":
"ee25559e75c9fa03648b4fcaf52f162baa243d2a"}

[commit-bot: I haz the power, 2017-01-30 19:00:54]

Description was changed from

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document,
that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
This adds support for filter on OffScreen canvas.

Most of those changes are due to offscreen canvas not having access to a
Document,
that is needed by CSS filter parser.

Also, filter(url) are still no-ops.


BUG=593838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2326633002
Cr-Commit-Position: refs/heads/master@{#447023}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee25559e75c9fa03648b4fcaf52f...
==========

[commit-bot: I haz the power, 2017-01-30 19:00:58]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/ee25559e75c9fa03648b4fcaf52f...