Move common logic between BoxReflectFilterOperation and FEBoxReflect into BoxReflection.

Move common logic between BoxReflectFilterOperation and FEBoxReflect into BoxReflection.

This avoids duplication between these classes, and encapsulates the parameters
of a filter-based reflection effect into a nice little self-contained class.

BUG=436475
Committed: https://crrev.com/03ed0066d50f0ace7020d3c1bf7b6da1ac3dca77
Cr-Commit-Position: refs/heads/master@{#387934}

[jbroman, 2016-04-12 19:52:45]

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

[jbroman, 2016-04-12 19:52:46]

I intend to add the mask image as well, and the amount of redundant places the
list of box reflection parameters has to be copied is surprisingly large.

So, I'm first refactoring it to carry around a BoxReflection object, which
encapsulates much of the common logic.

[Stephen White, 2016-04-13 15:35:51]

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:22: class
PLATFORM_EXPORT BoxReflection {
<bikeshed> Could this be BoxReflectParams (my preference) or BoxReflectionParams
or BoxReflectionInfo or BoxReflectInfo or something? BoxReflection sounds to me
like it does actual reflection (rendering). </bikeshed>

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:50:
PassRefPtr<SkImageFilter> createImageFilter(PassRefPtr<SkImageFilter> input)
const;
For dependencies and symmetry, could we keep the creation of the SkImageFilter
in the SkiaImageFilterBuilder? That way, this class stays a simple bag of data
and math. AFAICT, there's no refactoring to be had here, since FEBoxReflect
calls the builder already.

[jbroman, 2016-04-13 17:59:15]

A few responses. If you still want to change, I won't resist much more. :)

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:22: class
PLATFORM_EXPORT BoxReflection {
On 2016/04/13 at 15:35:51, Stephen White wrote:
> <bikeshed> Could this be BoxReflectParams (my preference) or
BoxReflectionParams or BoxReflectionInfo or BoxReflectInfo or something?
BoxReflection sounds to me like it does actual reflection (rendering).
</bikeshed>

It could. I'd be OK with BoxReflectParams, though that implies it's a little
more structy (and knows less about math) than this. I would draw a comparison
with Gradient and Pattern.

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:50:
PassRefPtr<SkImageFilter> createImageFilter(PassRefPtr<SkImageFilter> input)
const;
On 2016/04/13 at 15:35:51, Stephen White wrote:
> For dependencies

I don't know what you mean by "for dependencies"; these are part of the same
component, so it's allowed to use Skia here.

> and symmetry, could we keep the creation of the SkImageFilter in the
SkiaImageFilterBuilder? That way, this class stays a simple bag of data and
math. AFAICT, there's no refactoring to be had here, since FEBoxReflect calls
the builder already.

Could. SkiaImageFilterBuilder seems a little weird, and this logic seems more
closely connected with the math (e.g. the math to make the reflection matrix)
than it is with any other other methods on SkiaImageFilterBuilder (which already
builds a minority of skia image filters).

[Stephen White, 2016-04-13 18:07:35]

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:22: class
PLATFORM_EXPORT BoxReflection {
On 2016/04/13 17:59:15, jbroman wrote:
> On 2016/04/13 at 15:35:51, Stephen White wrote:
> > <bikeshed> Could this be BoxReflectParams (my preference) or
> BoxReflectionParams or BoxReflectionInfo or BoxReflectInfo or something?
> BoxReflection sounds to me like it does actual reflection (rendering).
> </bikeshed>
> 
> It could. I'd be OK with BoxReflectParams, though that implies it's a little
> more structy (and knows less about math) than this. I would draw a comparison
> with Gradient and Pattern.

OK, I buy that comparison.

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:50:
PassRefPtr<SkImageFilter> createImageFilter(PassRefPtr<SkImageFilter> input)
const;
On 2016/04/13 17:59:15, jbroman wrote:
> On 2016/04/13 at 15:35:51, Stephen White wrote:
> > For dependencies
> 
> I don't know what you mean by "for dependencies"; these are part of the same
> component, so it's allowed to use Skia here.
> 
> > and symmetry, could we keep the creation of the SkImageFilter in the
> SkiaImageFilterBuilder? That way, this class stays a simple bag of data and
> math. AFAICT, there's no refactoring to be had here, since FEBoxReflect calls
> the builder already.
> 
> Could. SkiaImageFilterBuilder seems a little weird, and this logic seems more
> closely connected with the math (e.g. the math to make the reflection matrix)
> than it is with any other other methods on SkiaImageFilterBuilder (which
already
> builds a minority of skia image filters).

True, but everything SkImageFilter-related lives in platform/graphics/filters 
(except for the oddball SVGFEImage, for dependency reasons). I'd like to keep
them together if possible.

[jbroman, 2016-04-13 22:24:52]

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BoxReflection.h (right):

https://codereview.chromium.org/1870793004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BoxReflection.h:50:
PassRefPtr<SkImageFilter> createImageFilter(PassRefPtr<SkImageFilter> input)
const;
On 2016/04/13 at 18:07:35, Stephen White wrote:
> On 2016/04/13 17:59:15, jbroman wrote:
> > On 2016/04/13 at 15:35:51, Stephen White wrote:
> > > For dependencies
> > 
> > I don't know what you mean by "for dependencies"; these are part of the same
> > component, so it's allowed to use Skia here.
> > 
> > > and symmetry, could we keep the creation of the SkImageFilter in the
> > SkiaImageFilterBuilder? That way, this class stays a simple bag of data and
> > math. AFAICT, there's no refactoring to be had here, since FEBoxReflect
calls
> > the builder already.
> > 
> > Could. SkiaImageFilterBuilder seems a little weird, and this logic seems
more
> > closely connected with the math (e.g. the math to make the reflection
matrix)
> > than it is with any other other methods on SkiaImageFilterBuilder (which
already
> > builds a minority of skia image filters).
> 
> True, but everything SkImageFilter-related lives in platform/graphics/filters 
> (except for the oddball SVGFEImage, for dependency reasons). I'd like to keep
them together if possible.

Done.

[Stephen White, 2016-04-14 13:49:21]

LGTM

https://codereview.chromium.org/1870793004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):

https://codereview.chromium.org/1870793004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:330: const
BoxReflection& reflection() const { return m_reflection; }
Nit: boxReflection()?

[jbroman, 2016-04-14 15:33:16]

https://codereview.chromium.org/1870793004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h
(right):

https://codereview.chromium.org/1870793004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterOperation.h:330: const
BoxReflection& reflection() const { return m_reflection; }
On 2016/04/14 at 13:49:21, Stephen White wrote:
> Nit: boxReflection()?

Only if you want danakj to come by and immediately rename it to
"getBoxReflection" in preparation for the style rewrite (Chromium-style casing
would cause a name collision with the type name). Besides, I'd argue that the
class is already called BoxReflectFilterOperation.

[jbroman, 2016-04-14 15:33:19]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-04-18 14:30:03]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:30:13]

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

[commit-bot: I haz the power, 2016-04-18 14:32:23]

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

[commit-bot: I haz the power, 2016-04-18 14:32:24]

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

[jbroman, 2016-04-18 14:45:13]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-04-18 14:45:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1870793004/#ps80001
(title: "merge with master")

[commit-bot: I haz the power, 2016-04-18 14:45:33]

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

[commit-bot: I haz the power, 2016-04-18 17:23:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-18 17:24:51]

Description was changed from

==========
Move common logic between BoxReflectFilterOperation and FEBoxReflect into
BoxReflection.

This avoids duplication between these classes, and encapsulates the parameters
of a filter-based reflection effect into a nice little self-contained class.

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

to

==========
Move common logic between BoxReflectFilterOperation and FEBoxReflect into
BoxReflection.

This avoids duplication between these classes, and encapsulates the parameters
of a filter-based reflection effect into a nice little self-contained class.

BUG=436475

Committed: https://crrev.com/03ed0066d50f0ace7020d3c1bf7b6da1ac3dca77
Cr-Commit-Position: refs/heads/master@{#387934}
==========

[commit-bot: I haz the power, 2016-04-18 17:24:52]

Patchset 5 (id:??) landed as
https://crrev.com/03ed0066d50f0ace7020d3c1bf7b6da1ac3dca77
Cr-Commit-Position: refs/heads/master@{#387934}