Implement -webkit-box-reflect as a filter.

Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a reflection
behind the content. This allows the filter code to be substantially reused, rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
Committed: https://crrev.com/4fc4066e585e2642e2876b1a928cbe0e9c83c2c4
Cr-Commit-Position: refs/heads/master@{#384488}

[jbroman, 2016-03-24 19:21:38]

Description was changed from

==========
WIP: Implement -webkit-box-reflect as a filter.

DO NOT SUBMIT: This is super alpha prototypey code.

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

to

==========
WIP: Implement -webkit-box-reflect as a filter.

DO NOT SUBMIT: This is super alpha prototypey code.

BUG=436475
==========

[jbroman, 2016-03-24 19:22:15]

Description was changed from

==========
WIP: Implement -webkit-box-reflect as a filter.

DO NOT SUBMIT: This is super alpha prototypey code.

BUG=436475
==========

to

==========
Implement -webkit-box-reflect as a filter.



BUG=436475
==========

[jbroman, 2016-03-29 15:11:37]

Description was changed from

==========
Implement -webkit-box-reflect as a filter.



BUG=436475
==========

to

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

[jbroman, 2016-03-29 15:15:12]

jbroman@chromium.org changed reviewers:
+ pdr@chromium.org, senorblanco@chromium.org

[jbroman, 2016-03-29 15:15:13]

This could probably be cleaned up yet more, but please take a look.

This seems to me to be a viable way of simplifying -webkit-box-reflect, and also
avoids
having to handle it separately in Slimming Paint v2 (it can be treated like
other filters).

As noted in the description, missing in this patch is handling of mask image.
I also anticipate that making this work fully in all edge cases would require
adjusting the
filter code to operate in terms of a mapRect primitive, rather than the "filter
outsets" used
in some places (which do not apply to effects that can scale).

Final patchset to land will have the feature changed to off-by-default, but it's
marked as
experimental here so that you can see the current layout test failures, most of
which I hope
to resolve before turning this feature on.

[pdr., 2016-03-29 18:36:55]

This is really good, and I like your release plan.

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:721: bool hasFilter() const
{ return style() && style()->hasFilter(); }
Would it be cleaner to update paintInvalidationState::supportsCachedOffsets to
call "hasFilterInducingProperty" too?

With that change, I think we can remove hasFilter entirely. I think this will
keep us from making mistakes in the future.

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:60:
BOX_REFLECT,
Can you add a very short comment describing this and mentioning that it isn't a
web exposed filter type?

I like the way this is implemented. WDYT about suggesting "filter:
box-reflect(...)" to the specs group as a way to spec reflection?

[Stephen White, 2016-03-30 14:24:20]

Description was changed from

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

to

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

[Stephen White, 2016-03-30 14:41:35]

I'm guessing this new code will only be used by the (unprefixed) box-reflect,
hidden behind the experimental flag? And -webkit-box-reflect will continue to
use the old code? If so, what's stopping us switching over -webkit-box-reflect
to the new code now? For a feature with a pretty small usage count (0.2%), I
think it's worth the risk of regressions.

Out of curiosity, what's the spec status of this feature? I didn't see an
intent-to-implement, and all of the browser vendors on chromestatus.com are
listed as "no public signals" (even including Safari, oddly enough).

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:723: bool
hasFilterInducingProperty() const
I find this name unintuitive. Could we be more explicit, e.g.,
hasFilterOrReflection()?

Think about it from the point-of-view of someone new to this code: what is a
filter-inducing property? Is this a fundamental property or an implementation
detail? You could explain it with a comment, but Blink style is to put it in the
name.

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:111: if
(m_paintLayer.reflectionInfo() &&
!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled()) {
It seems fragile to have to check this flag in so many places. Would it be
possible to check the runtime flag once, and prevent reflectionInfo() or
hasReflection() being set in the first place?

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:45:
CSSBoxReflectFilter status=experimental
Just so I understand, this enables an (unprefixed) box-reflect behind the
experimental flag, while the legacy code lives on as -webkit-box-reflect?

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:60:
BOX_REFLECT,
On 2016/03/29 18:36:55, pdr wrote:
> Can you add a very short comment describing this and mentioning that it isn't
a
> web exposed filter type?
> 
> I like the way this is implemented. WDYT about suggesting "filter:
> box-reflect(...)" to the specs group as a way to spec reflection?

+1

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp
(right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp:159: //
Already accounted for at all call sites.
This seems fragile to callers expecting that box-reflect will behave like other
filters.

If it's not possible to express box-filter's behaviour as outsets, I think it's
finally time to convert from outsets to a rect-mapping function as Skia does
internally (see
https://bugs.chromium.org/p/chromium/issues/detail?id=523534#c2). This will
require some refactoring in Blink, but I'd very much prefer that we do that
first.

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
(right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:166:
// TODO(jbroman): Consider explaining box reflect to the compositor,
I think you're going to have to do that anyway, in order for this to be
compatible with threaded animations (ajuma@ would know for sure). Could we do
that now? Then we could move the buildBoxReflectFilter() logic into
FEBoxReflect::createImageFilter(), and have a single call site.

[jbroman, 2016-03-30 15:17:03]

On 2016/03/30 at 14:41:35, senorblanco wrote:
> I'm guessing this new code will only be used by the (unprefixed) box-reflect,
hidden behind the experimental flag? And -webkit-box-reflect will continue to
use the old code? If so, what's stopping us switching over -webkit-box-reflect
to the new code now? For a feature with a pretty small usage count (0.2%), I
think it's worth the risk of regressions.

My intention is, once this works well enough to be mostly compatible (it's
getting there), to enable this feature, which will make it the code backing
-webkit-box-reflect. The primary purpose of this work is to simplify that code;
the possibility of getting the feature standardized is gravy.

> Out of curiosity, what's the spec status of this feature? I didn't see an
intent-to-implement, and all of the browser vendors on chromestatus.com are
listed as "no public signals" (even including Safari, oddly enough).

Unchanged from before. I think this work would probably make specifying it more
reasonable (because it's mostly compatible with existing implementations of
-webkit-box-reflect, but its semantics should be easier to specify in terms of
the already-specified filter effects).

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.h:723: bool
hasFilterInducingProperty() const
> I find this name unintuitive. Could we be more explicit, e.g.,
hasFilterOrReflection()?
> 
> Think about it from the point-of-view of someone new to this code: what is a
filter-inducing property? Is this a fundamental property or an implementation
detail? You could explain it with a comment, but Blink style is to put it in the
name.

I'm trying to avoid a future in which we write
"hasFilterOrReflectionOrGlowOrGlisten". I'm open to bikeshedding a little on
this; pdr, what do you think?

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:111: if
(m_paintLayer.reflectionInfo() &&
!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled()) {
> It seems fragile to have to check this flag in so many places. Would it be
possible to check the runtime flag once, and prevent reflectionInfo() or
hasReflection() being set in the first place?

At present some of the code that does invalidation for reflections still relies
on it. My ultimate hope is that the feature will quickly be turned on, at which
point it'll be easier to remove most of reflectionInfo.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:45:
CSSBoxReflectFilter status=experimental
> Just so I understand, this enables an (unprefixed) box-reflect behind the
experimental flag, while the legacy code lives on as -webkit-box-reflect?

No, it implements the CSS '-webkit-box-reflect' property using a filter. If
there is broader interest in standardizing this property, -webkit-box-reflect
could become an alias for an unprefixed 'box-reflect' property.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:60:
BOX_REFLECT,
> On 2016/03/29 18:36:55, pdr wrote:
> > Can you add a very short comment describing this and mentioning that it
isn't a
> > web exposed filter type?
> > 
> > I like the way this is implemented. WDYT about suggesting "filter:
> > box-reflect(...)" to the specs group as a way to spec reflection?
> 
> +1
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp
(right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp:159:
// Already accounted for at all call sites.
> This seems fragile to callers expecting that box-reflect will behave like
other filters.
> 
> If it's not possible to express box-filter's behaviour as outsets, I think
it's finally time to convert from outsets to a rect-mapping function as Skia
does internally (see
https://bugs.chromium.org/p/chromium/issues/detail?id=523534#c2). This will
require some refactoring in Blink, but I'd very much prefer that we do that
first.

"first" meaning "before landing this patch (that's guarded by a flag)", "before
shipping this feature", or something else?

The core problem with outsets is that they don't deal with a filter applying a
scale.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:166:
// TODO(jbroman): Consider explaining box reflect to the compositor,
> I think you're going to have to do that anyway, in order for this to be
compatible with threaded animations (ajuma@ would know for sure).

I haven't yet checked how threaded animations work if there is a box reflect
applied (I considered that followup work, but probably blocking the feature
shipping).

FWIW, though, -webkit-box-reflect is not animatable today, so it isn't necessary
to explain to the compositor how to animate it in order to maintain feature
parity.

(It is reasonably simple to comprehend how it would animate if we wanted to, of
course.)

> Could we do that now? Then we could move the buildBoxReflectFilter() logic
into FEBoxReflect::createImageFilter(), and have a single call site.

[Stephen White, 2016-03-30 16:05:03]

On 2016/03/30 15:17:03, jbroman wrote:
> On 2016/03/30 at 14:41:35, senorblanco wrote:
> > I'm guessing this new code will only be used by the (unprefixed)
box-reflect,
> hidden behind the experimental flag? And -webkit-box-reflect will continue to
> use the old code? If so, what's stopping us switching over -webkit-box-reflect
> to the new code now? For a feature with a pretty small usage count (0.2%), I
> think it's worth the risk of regressions.
> 
> My intention is, once this works well enough to be mostly compatible (it's
> getting there), to enable this feature, which will make it the code backing
> -webkit-box-reflect. The primary purpose of this work is to simplify that
code;
> the possibility of getting the feature standardized is gravy.

Ahh, I misread the change to RuntimeEnabledFeatures.in. So this means that
enable-experimental-web-platform-features turns on the new path behind
-webkit-box-reflect?
Does this new code get test coverage now, or only when the flag check drops?

> > Out of curiosity, what's the spec status of this feature? I didn't see an
> intent-to-implement, and all of the browser vendors on http://chromestatus.com
are
> listed as "no public signals" (even including Safari, oddly enough).
> 
> Unchanged from before. I think this work would probably make specifying it
more
> reasonable (because it's mostly compatible with existing implementations of
> -webkit-box-reflect, but its semantics should be easier to specify in terms of
> the already-specified filter effects).

Acknowledged. If it's just intended to back -webkit-box-reflect, I'm fine with
that.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutObject.h:723: bool
> hasFilterInducingProperty() const
> > I find this name unintuitive. Could we be more explicit, e.g.,
> hasFilterOrReflection()?
> > 
> > Think about it from the point-of-view of someone new to this code: what is a
> filter-inducing property? Is this a fundamental property or an implementation
> detail? You could explain it with a comment, but Blink style is to put it in
the
> name.
> 
> I'm trying to avoid a future in which we write
> "hasFilterOrReflectionOrGlowOrGlisten". I'm open to bikeshedding a little on
> this; pdr, what do you think?

The fact that it uses a filter feels like an implementation detail to me.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:111: if
> (m_paintLayer.reflectionInfo() &&
> !RuntimeEnabledFeatures::cssBoxReflectFilterEnabled()) {
> > It seems fragile to have to check this flag in so many places. Would it be
> possible to check the runtime flag once, and prevent reflectionInfo() or
> hasReflection() being set in the first place?
> 
> At present some of the code that does invalidation for reflections still
relies
> on it. My ultimate hope is that the feature will quickly be turned on, at
which
> point it'll be easier to remove most of reflectionInfo.

Acknowledged; no objection now that I understand where this is going.

> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:45:
> CSSBoxReflectFilter status=experimental
> > Just so I understand, this enables an (unprefixed) box-reflect behind the
> experimental flag, while the legacy code lives on as -webkit-box-reflect?
> 
> No, it implements the CSS '-webkit-box-reflect' property using a filter. If
> there is broader interest in standardizing this property, -webkit-box-reflect
> could become an alias for an unprefixed 'box-reflect' property.

Acknowledged.

> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:60:
> BOX_REFLECT,
> > On 2016/03/29 18:36:55, pdr wrote:
> > > Can you add a very short comment describing this and mentioning that it
> isn't a
> > > web exposed filter type?
> > > 
> > > I like the way this is implemented. WDYT about suggesting "filter:
> > > box-reflect(...)" to the specs group as a way to spec reflection?
> > 
> > +1
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp:159:
> // Already accounted for at all call sites.
> > This seems fragile to callers expecting that box-reflect will behave like
> other filters.
> > 
> > If it's not possible to express box-filter's behaviour as outsets, I think
> it's finally time to convert from outsets to a rect-mapping function as Skia
> does internally (see
> https://bugs.chromium.org/p/chromium/issues/detail?id=523534#c2). This will
> require some refactoring in Blink, but I'd very much prefer that we do that
> first.
> 
> "first" meaning "before landing this patch (that's guarded by a flag)",
"before
> shipping this feature", or something else?

I would prefer "before landing" (especially when I thought this was backing a
new
property), but I could live with "before shipping".

> The core problem with outsets is that they don't deal with a filter applying a
> scale.

Understood.

>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > File
> third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:166:
> // TODO(jbroman): Consider explaining box reflect to the compositor,
> > I think you're going to have to do that anyway, in order for this to be
> compatible with threaded animations (ajuma@ would know for sure).
> 
> I haven't yet checked how threaded animations work if there is a box reflect
> applied (I considered that followup work, but probably blocking the feature
> shipping).
> 
> FWIW, though, -webkit-box-reflect is not animatable today, so it isn't
necessary
> to explain to the compositor how to animate it in order to maintain feature
> parity.

In that case, probably not a blocker.

One downside to using a reference filter is the SkComposeImageFilter that it
utilizes in the compositor: this can currently prevent Skia from doing some 
optimizations between chained filters.

> (It is reasonably simple to comprehend how it would animate if we wanted to,
of
> course.)
> 
> > Could we do that now? Then we could move the buildBoxReflectFilter() logic
> into FEBoxReflect::createImageFilter(), and have a single call site.

[Stephen White, 2016-03-30 16:59:28]

Note that you may see a performance hit on the software path with this approach
over the existing codepath. If I'm counting correctly, there
will be five blits:

- one to draw the original primitive (saveLayer)
- one to flip with the SkMatrixImageFilter
- two to composite the flipped and the original with the SkXfermodeImageFilter
- one to draw the filtered result to the canvas (restore)

(There are some plans to try to eliminate the final blit in Skia, but no one is
working on it ATM.)

If performance becomes an issue, you might consider implementing an actual
"reflect" filter over in Skia. That'd save you at least two blits, since you
could
composite the flipped and original into the same destination.

[pdr., 2016-03-30 23:51:40]

> >
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutObject.h:723: bool
> > hasFilterInducingProperty() const
> > > I find this name unintuitive. Could we be more explicit, e.g.,
> > hasFilterOrReflection()?
> > > 
> > > Think about it from the point-of-view of someone new to this code: what is
a
> > filter-inducing property? Is this a fundamental property or an
implementation
> > detail? You could explain it with a comment, but Blink style is to put it in
the
> > name.
> > 
> > I'm trying to avoid a future in which we write
> > "hasFilterOrReflectionOrGlowOrGlisten". I'm open to bikeshedding a little on
> > this; pdr, what do you think?
> 
> The fact that it uses a filter feels like an implementation detail to me.

I like both your names. Take that, bikeshed.

I think a short comment above hasFilterInducingProperty will be sufficient for
anyone curious about what it means. I agree with senorblanco that people might
not know what it means, but removing hasFilter will force people to read the
comment instead of naively using hasFilter.

I'm also okay with hasFilterOrReflection since it does what it says on the tin.
We can revisit the more general name (hasFilterInducingProperty) once the glow
or glisten features are added.


I think we'll do one more full round on this review, so I'd update this to
whichever you think is best given the discussion here.

[jbroman, 2016-03-31 17:47:19]

On 2016/03/30 at 16:05:03, senorblanco wrote:
> > >
third_party/WebKit/Source/platform/graphics/filters/FilterOperations.cpp:159:
> > // Already accounted for at all call sites.
> > > This seems fragile to callers expecting that box-reflect will behave like
> > other filters.
> > > 
> > > If it's not possible to express box-filter's behaviour as outsets, I think
> > it's finally time to convert from outsets to a rect-mapping function as Skia
> > does internally (see
> > https://bugs.chromium.org/p/chromium/issues/detail?id=523534#c2). This will
> > require some refactoring in Blink, but I'd very much prefer that we do that
> > first.
> > 
> > "first" meaning "before landing this patch (that's guarded by a flag)",
"before
> > shipping this feature", or something else?
> 
> I would prefer "before landing" (especially when I thought this was backing a
new
> property), but I could live with "before shipping".

This patch seems mostly landable (and before landing, the feature will be turned
completely
off -- I'll turn it on when it's about to ship), so I'd prefer not to rebase it
on top
of that change.

But replacing filterOutsets with mapRect (which maps better to SkImageFilter)
seems reasonable,
so I can block shipping on it.

> >
https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutObject.h:723: bool
> > hasFilterInducingProperty() const
> > > I find this name unintuitive. Could we be more explicit, e.g.,
> > hasFilterOrReflection()?
> > > 
> > > Think about it from the point-of-view of someone new to this code: what is
a
> > filter-inducing property? Is this a fundamental property or an
implementation
> > detail? You could explain it with a comment, but Blink style is to put it in
the
> > name.
> > 
> > I'm trying to avoid a future in which we write
> > "hasFilterOrReflectionOrGlowOrGlisten". I'm open to bikeshedding a little on
> > this; pdr, what do you think?
> 
> The fact that it uses a filter feels like an implementation detail to me.

Right, but that's exactly the implementation detail I'm trying to capture.
Callers use this to check things like "do I need to filter the bounds?" and
"will this require a layer to apply the filter if it is composited?", which are
trying to get at the implementation detail. I'm explicitly trying to separate
"things that specify the 'filter' property" from "things that need to be handled
as a filter".

I've added a comment; does that help? I might have called it
"paintsWithFilters", but that already exists (it checks the same thing, plus
checks what the backing layer is the legacy compositing path; that distinction
will likely go away with SPv2).

> >
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:166:
> > // TODO(jbroman): Consider explaining box reflect to the compositor,
> > > I think you're going to have to do that anyway, in order for this to be
> > compatible with threaded animations (ajuma@ would know for sure).
> > 
> > I haven't yet checked how threaded animations work if there is a box reflect
> > applied (I considered that followup work, but probably blocking the feature
> > shipping).
> > 
> > FWIW, though, -webkit-box-reflect is not animatable today, so it isn't
necessary
> > to explain to the compositor how to animate it in order to maintain feature
> > parity.
> 
> In that case, probably not a blocker.
> 
> One downside to using a reference filter is the SkComposeImageFilter that it
> utilizes in the compositor: this can currently prevent Skia from doing some 
> optimizations between chained filters.

Acknowledged, though I expect this case (an element having both
'-webkit-box-reflect'
and 'filter') to be uncommon.

> > (It is reasonably simple to comprehend how it would animate if we wanted to,
of
> > course.)
> > 
> > > Could we do that now? Then we could move the buildBoxReflectFilter() logic
> > into FEBoxReflect::createImageFilter(), and have a single call site.

On 2016/03/30 at 16:59:28, senorblanco wrote:
> Note that you may see a performance hit on the software path with this
approach over the existing codepath. If I'm counting correctly, there
> will be five blits:
> 
> - one to draw the original primitive (saveLayer)
> - one to flip with the SkMatrixImageFilter
> - two to composite the flipped and the original with the SkXfermodeImageFilter
> - one to draw the filtered result to the canvas (restore)
> 
> (There are some plans to try to eliminate the final blit in Skia, but no one
is working on it ATM.)
> 
> If performance becomes an issue, you might consider implementing an actual
"reflect" filter over in Skia. That'd save you at least two blits, since you
could
> composite the flipped and original into the same destination.

That's how I count it, too. I do expect that there may be a small performance
regression for this uncommon feature. (It may also win, though, because in the
non-composited case we would previously record and raster twice.)

In the case that there's a mask image, there's an additional two: one to draw
the mask, and another to combine it with the source image. Those seem (nearly)
unavoidable, though.

It seemed less risky to go with a combination of the existing (and presumably
mostly correct) Skia filters. (And I'm not sure whether Skia would welcome such
a non-general filter, or whether some effort to generalize it would be required.
Seemed like followup work if we wanted it.)

[Stephen White, 2016-03-31 17:55:24]

LGTM

https://codereview.chromium.org/1775013003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
(right):

https://codereview.chromium.org/1775013003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp:166:
// TODO(jbroman): Consider explaining box reflect to the compositor,
Nit: could you log a bug for this?

[pdr., 2016-03-31 17:58:05]

Overall I think this looks good, but can you explain why
ComputedStyle::hasFilter wasn't removed?

[jbroman, 2016-03-31 18:47:56]

On 2016/03/31 at 17:58:05, pdr wrote:
> Overall I think this looks good, but can you explain why
ComputedStyle::hasFilter wasn't removed?

Because we still need to be able to ask the question "is the 'filter' property
set?". Even after this CL, there are some places before we create the filter
operations for reflection that need to know the answer.

[pdr., 2016-03-31 18:51:31]

On 2016/03/31 at 18:47:56, jbroman wrote:
> On 2016/03/31 at 17:58:05, pdr wrote:
> > Overall I think this looks good, but can you explain why
ComputedStyle::hasFilter wasn't removed?
> 
> Because we still need to be able to ask the question "is the 'filter' property
set?". Even after this CL, there are some places before we create the filter
operations for reflection that need to know the answer.

Oops, I meant LayoutObject::hasFilter.

[jbroman, 2016-03-31 19:18:29]

On 2016/03/31 at 18:51:31, pdr wrote:
> On 2016/03/31 at 18:47:56, jbroman wrote:
> > On 2016/03/31 at 17:58:05, pdr wrote:
> > > Overall I think this looks good, but can you explain why
ComputedStyle::hasFilter wasn't removed?
> > 
> > Because we still need to be able to ask the question "is the 'filter'
property set?". Even after this CL, there are some places before we create the
filter operations for reflection that need to know the answer.
> 
> Oops, I meant LayoutObject::hasFilter.

Removed PaintLayer::hasFilter and LayoutObject::hasFilter; PTAL.

[pdr., 2016-03-31 19:44:52]

On 2016/03/31 at 19:18:29, jbroman wrote:
> On 2016/03/31 at 18:51:31, pdr wrote:
> > On 2016/03/31 at 18:47:56, jbroman wrote:
> > > On 2016/03/31 at 17:58:05, pdr wrote:
> > > > Overall I think this looks good, but can you explain why
ComputedStyle::hasFilter wasn't removed?
> > > 
> > > Because we still need to be able to ask the question "is the 'filter'
property set?". Even after this CL, there are some places before we create the
filter operations for reflection that need to know the answer.
> > 
> > Oops, I meant LayoutObject::hasFilter.
> 
> Removed PaintLayer::hasFilter and LayoutObject::hasFilter; PTAL.

LGTM!

[jbroman, 2016-03-31 19:47:29]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-03-31 19:47:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1775013003/#ps220001
(title: "Disable CSSBoxReflectFilter feature for landing")

[commit-bot: I haz the power, 2016-03-31 19:47:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775013003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775013003/220001

[jbroman, 2016-03-31 21:03:26]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-03-31 21:03:27]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1775013003/#ps240001
(title: "make msvc dbg happy")

[commit-bot: I haz the power, 2016-03-31 21:03:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775013003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775013003/240001

[commit-bot: I haz the power, 2016-03-31 23:52:03]

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

[commit-bot: I haz the power, 2016-03-31 23:52:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbroman, 2016-04-01 03:44:37]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-04-01 03:45:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775013003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775013003/240001

[commit-bot: I haz the power, 2016-04-01 05:14:44]

Description was changed from

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

to

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

[commit-bot: I haz the power, 2016-04-01 05:14:46]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-04-01 05:16:16]

Description was changed from

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475
==========

to

==========
Implement -webkit-box-reflect as a filter.

This separates the notion of having the 'filter' property from having a
"filter-inducing property" (i.e. a CSS property which is implemented as a
filter operation).

Then -webkit-box-reflect is made to be such a property (behind a runtime flag),
so that specifying it adds an additional filter operation which draws a
reflection
behind the content. This allows the filter code to be substantially reused,
rather
than requiring separate logic to handle reflection, as exists in the current
implementation.

This will likely cause minor rendering changes, but it also fixes some bugs,
simplifies the code (at least, once the legacy code can be removed), and
rationalizes
the definition of this property.

Notably not implemented in this patch is support for the mask image
parameter; this will be added in a subsequent patch.

BUG=436475

Committed: https://crrev.com/4fc4066e585e2642e2876b1a928cbe0e9c83c2c4
Cr-Commit-Position: refs/heads/master@{#384488}
==========

[commit-bot: I haz the power, 2016-04-01 05:16:18]

Patchset 13 (id:??) landed as
https://crrev.com/4fc4066e585e2642e2876b1a928cbe0e9c83c2c4
Cr-Commit-Position: refs/heads/master@{#384488}

[jbroman, 2016-04-05 15:51:56]

one comment I failed to publish earlier

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):

https://codereview.chromium.org/1775013003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:60:
BOX_REFLECT,
On 2016/03/30 at 14:41:35, Stephen White wrote:
> On 2016/03/29 18:36:55, pdr wrote:
> > Can you add a very short comment describing this and mentioning that it
isn't a
> > web exposed filter type?
> > 
> > I like the way this is implemented. WDYT about suggesting "filter:
> > box-reflect(...)" to the specs group as a way to spec reflection?
> 
> +1

I'd think that specifying something more similar to what developers actually use
today
would be more practical. I'm not sure there actually is demand for arbitrarily
interleaving
these. It's also a little funny in that it's sensitive to the border box (it
uses it to position
itself), which seems a little stranger in the general case.

I think I'd envision specifying (or at least internally thinking of it as)
something like this:

  Name: box-reflect
  Value: none | [ above | below | right | left ]? <length>? <image>?
  Initial: none
  Applies to: all elements
  Inherited: no
  (etc)

  If the value of the 'box-reflect' property is not 'none', then it renders
  as though there were an additional filter function at the end of the 'filter'
  property, which behaves as follows: <details of reflection, alignment, etc.
here>

I could imagine going further and adding it as an explicit filter function, but
that seems
like more work that I'm not sure would be valued.