Retire setGradientSpaceTransform, setPatternSpaceTransform

Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.

R=fs@opera.com,junov@chromium.org
BUG=614368
Committed: https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b
Cr-Commit-Position: refs/heads/master@{#396557}

[f(malita), 2016-05-24 13:47:43]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

WIP

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

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

WIP

BUG=614368
==========

[f(malita), 2016-05-24 14:26:57]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

WIP

BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

=======================================================================

Most gradient/pattern clients always reset the transform before
applying to a paint, while some (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

[f(malita), 2016-05-24 14:27:31]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

=======================================================================

Most gradient/pattern clients always reset the transform before
applying to a paint, while some (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while some (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

[f(malita), 2016-05-24 17:11:15]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while some (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

[f(malita), 2016-05-24 17:12:51]

fmalita@chromium.org changed reviewers:
+ fs@opera.com, junov@chromium.org

[f(malita), 2016-05-24 17:15:10]

3 trivial rebaselines (1-2 pixel diffs), most likely due to matrix precision
differences:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

[fs, 2016-05-24 18:32:03]

This WFM. core/layout/svg LGTM (platform/ non-owner LGTM.)

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

https://codereview.chromium.org/2007553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasStyle.cpp:127: break;
Nit: Exchange this line with the one above?

[f(malita), 2016-05-24 18:42:27]

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

https://codereview.chromium.org/2007553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasStyle.cpp:127: break;
On 2016/05/24 18:32:03, fs wrote:
> Nit: Exchange this line with the one above?

Done.

[f(malita), 2016-05-25 12:57:16]

fmalita@chromium.org changed reviewers:
+ schenney@chromium.org

[f(malita), 2016-05-26 14:09:28]

fmalita@chromium.org changed reviewers:
+ senorblanco@chromium.org
- schenney@chromium.org

[f(malita), 2016-05-26 19:57:47]

senorblanco and/or junov - can you take a look at the canvas & platform/graphics
bits?  Thanks!

[Stephen White, 2016-05-26 20:36:26]

Looks good, but I'll leave for Justin to take a look.

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp
(right):

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:70:
m_transform = transform * m_transform;
Is there any danger that this function will be called more than once? The
previous version seemed like it would apply the same transform if called twice,
whereas this one will accumulate.

Maybe we should assert? Or perhaps it's just caveat caller?

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:123: sk_sp<SkShader>
createShader(const SkMatrix* localMatrix);
I'm be tempted to say, just make this a const SkMatrix&, and force callers to
pass identity if they don't want a transform. The callee does that anyway, so I
don't think there's a speed win from allowing a nullptr.

[fs, 2016-05-26 21:35:44]

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp
(right):

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:70:
m_transform = transform * m_transform;
On 2016/05/26 at 20:36:26, Stephen White wrote:
> Is there any danger that this function will be called more than once? The
previous version seemed like it would apply the same transform if called twice,
whereas this one will accumulate.

The semantics are unchanged, maybe easier to see if considering
fooSpaceTransform() as returning a mutable reference and writing as:

m_foo->fooSpaceTransform() = transform * m_foo->fooSpaceTransform();

There's only one callsite still though (and none more planned for atm; with this
simplification maybe we can do away with it entirely instead.)

[Stephen White, 2016-05-26 21:38:28]

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp
(right):

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp:70:
m_transform = transform * m_transform;
On 2016/05/26 21:35:43, fs wrote:
> On 2016/05/26 at 20:36:26, Stephen White wrote:
> > Is there any danger that this function will be called more than once? The
> previous version seemed like it would apply the same transform if called
twice,
> whereas this one will accumulate.
> 
> The semantics are unchanged, maybe easier to see if considering
> fooSpaceTransform() as returning a mutable reference and writing as:
> 
> m_foo->fooSpaceTransform() = transform * m_foo->fooSpaceTransform();
> 
> There's only one callsite still though (and none more planned for atm; with
this
> simplification maybe we can do away with it entirely instead.)

You're right; it's the same as before. Ignore my comment.

[Justin Novosad, 2016-05-27 15:02:45]

lgtm

[f(malita), 2016-05-27 15:09:42]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an optional argument to applyToPaint().  This simplifies the
gradient/pattern impl and clarifies that clients are responsible for
tracking the local matrix.  The local matrix is still used as part of
the shader cache key - but since SkShaders already track their local
matrix, there is no need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

[f(malita), 2016-05-27 15:18:40]

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:123: sk_sp<SkShader>
createShader(const SkMatrix* localMatrix);
On 2016/05/26 20:36:26, Stephen White wrote:
> I'm be tempted to say, just make this a const SkMatrix&, and force callers to
> pass identity if they don't want a transform. The callee does that anyway, so
I
> don't think there's a speed win from allowing a nullptr.

Thanks, that actually gets rid of quite a bit of boilerplate (at the cost of
adding a handful of SkMatrix::MakeScale(1)s - wish SkMatrix had a MakeIdentity
or a global identity matrix, maybe we should add that).

Done.

[fs, 2016-05-27 15:30:57]

On 2016/05/27 at 15:18:40, fmalita wrote:
>
https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/Gradient.h (right):
> 
>
https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/Gradient.h:123: sk_sp<SkShader>
createShader(const SkMatrix* localMatrix);
> On 2016/05/26 20:36:26, Stephen White wrote:
> > I'm be tempted to say, just make this a const SkMatrix&, and force callers
to
> > pass identity if they don't want a transform. The callee does that anyway,
so I
> > don't think there's a speed win from allowing a nullptr.
> 
> Thanks, that actually gets rid of quite a bit of boilerplate (at the cost of
adding a handful of SkMatrix::MakeScale(1)s - wish SkMatrix had a MakeIdentity
or a global identity matrix, maybe we should add that).

SkMatrix::I()?

[f(malita), 2016-05-27 15:37:59]

On 2016/05/27 15:30:57, fs wrote:
> On 2016/05/27 at 15:18:40, fmalita wrote:
> >
>
https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/graphics/Gradient.h (right):
> > 
> >
>
https://codereview.chromium.org/2007553002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/Gradient.h:123: sk_sp<SkShader>
> createShader(const SkMatrix* localMatrix);
> > On 2016/05/26 20:36:26, Stephen White wrote:
> > > I'm be tempted to say, just make this a const SkMatrix&, and force callers
> to
> > > pass identity if they don't want a transform. The callee does that anyway,
> so I
> > > don't think there's a speed win from allowing a nullptr.
> > 
> > Thanks, that actually gets rid of quite a bit of boilerplate (at the cost of
> adding a handful of SkMatrix::MakeScale(1)s - wish SkMatrix had a MakeIdentity
> or a global identity matrix, maybe we should add that).
> 
> SkMatrix::I()?

doh!

Thanks, done.

[f(malita), 2016-05-27 21:02:38]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-05-27 21:02:39]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2007553002/#ps140001
(title: "fmalita discovers SkMatrix::I()")

[commit-bot: I haz the power, 2016-05-27 21:02:57]

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

[commit-bot: I haz the power, 2016-05-27 21:07:16]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

[commit-bot: I haz the power, 2016-05-27 21:07:18]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-27 21:08:55]

Description was changed from

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.


R=fs@opera.com,junov@chromium.org
BUG=614368
==========

to

==========
Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.

R=fs@opera.com,junov@chromium.org
BUG=614368

Committed: https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b
Cr-Commit-Position: refs/heads/master@{#396557}
==========

[commit-bot: I haz the power, 2016-05-27 21:08:58]

Patchset 8 (id:??) landed as
https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b
Cr-Commit-Position: refs/heads/master@{#396557}