Add SkRRect::transform.

Add SkRRect::transform.

Much like SkPath::transform, it transforms an SkRRect based on an
SkMatrix. Unlike SkPath::transform, it will fail for matrices that
contain perspective or skewing.

Required by a future change (https://codereview.chromium.org/48623006)
to speed up drawing large blurry rounded rectangles by using ninepatches.

TODO: This could easily support 90 degree rotations, if desired.

BUG=https://b.corp.google.com/issue?id=11174385
R=reed@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12132

[robertphillips, 2013-10-30 23:55:02]

Questions

https://codereview.chromium.org/52703003/diff/1/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/52703003/diff/1/include/core/SkRRect.h#newcod...
include/core/SkRRect.h:285: *      scale and/or translate, or this call will
fail.
this is a const method?! Are we really going to alter this?

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp
File src/core/SkRRect.cpp (right):

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode264
src/core/SkRRect.cpp:264: bool SkRRect::transform(const SkMatrix& matrix,
SkRRect* dst) const {
This seems like a complete abrogation of the const contract.

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode282
src/core/SkRRect.cpp:282: 
Will this do the right thing if the matrix is a reflection and the RR isn't
symmetric?

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode291
src/core/SkRRect.cpp:291: }
Do we actually need "radii" here? Can't we do this in place in dst?

[scroggo, 2013-10-31 17:40:34]

New patch fixes negative scaling and adds a test.

https://codereview.chromium.org/52703003/diff/1/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/52703003/diff/1/include/core/SkRRect.h#newcod...
include/core/SkRRect.h:285: *      scale and/or translate, or this call will
fail.
On 2013/10/30 23:55:02, robertphillips wrote:
> this is a const method?! Are we really going to alter this?

This seemed like an odd pattern to me, but I was keeping it consistent with
SkPath::transform. SkPath::transform is overloaded to include a non-const
version to modify self, but if you call path.transform(matrix, &path) or
path.transform(matrix, NULL) (using the const function) it will modify the path
as well.

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp
File src/core/SkRRect.cpp (right):

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode264
src/core/SkRRect.cpp:264: bool SkRRect::transform(const SkMatrix& matrix,
SkRRect* dst) const {
On 2013/10/30 23:55:02, robertphillips wrote:
> This seems like a complete abrogation of the const contract.

Agreed. This is to be consistent with SkPath::transform. If I change this one,
should I fix SkPath or have them work differently?

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode282
src/core/SkRRect.cpp:282: 
On 2013/10/30 23:55:02, robertphillips wrote:
> Will this do the right thing if the matrix is a reflection and the RR isn't
> symmetric?

No. Fixed.

https://codereview.chromium.org/52703003/diff/1/src/core/SkRRect.cpp#newcode291
src/core/SkRRect.cpp:291: }
On 2013/10/30 23:55:02, robertphillips wrote:
> Do we actually need "radii" here? Can't we do this in place in dst?

Done.

[robertphillips, 2013-10-31 18:25:26]

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp
File tests/RoundRectTest.cpp (right):

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:457: const SkMatrix& matrix) {
This comment seems to contradict the code.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:554: REPORTER_ASSERT(reporter, origLL.fX == dstLR.fX);
Don't think this is right.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:580: GET_RADII;
Don't think this is right.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:599: 
Should test simultaneous -x & -y scales.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:629: {
This is an awfully short RRect (1 pixel high)?

[scroggo, 2013-10-31 19:20:10]

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp
File tests/RoundRectTest.cpp (right):

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:457: const SkMatrix& matrix) {
On 2013/10/31 18:25:26, robertphillips wrote:
> This comment seems to contradict the code.

Fixed.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:554: REPORTER_ASSERT(reporter, origLL.fX == dstLR.fX);
On 2013/10/31 18:25:26, robertphillips wrote:
> Don't think this is right.

The behavior and test have been fixed.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:580: GET_RADII;
On 2013/10/31 18:25:26, robertphillips wrote:
> Don't think this is right.

The behavior and test have been fixed.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:599: 
On 2013/10/31 18:25:26, robertphillips wrote:
> Should test simultaneous -x & -y scales.

Done.

https://codereview.chromium.org/52703003/diff/70001/tests/RoundRectTest.cpp#n...
tests/RoundRectTest.cpp:629: {
On 2013/10/31 18:25:26, robertphillips wrote:
> This is an awfully short RRect (1 pixel high)?

Taller now.

[robertphillips, 2013-10-31 21:42:44]

lgtm

[scroggo, 2013-11-01 22:03:04]

On 2013/10/31 21:42:44, robertphillips wrote:
> lgtm

Mike, can you take a look for the API change?

[reed1, 2013-11-04 20:21:09]

lgtm

btw -- we *could* support 90-degree rotations, and then we could just use
matrix::rectStaysRect() to know which matrix variants were valid...

https://codereview.chromium.org/52703003/diff/170001/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/52703003/diff/170001/include/core/SkRRect.h#n...
include/core/SkRRect.h:286: *  @param dst SkRRect to store the result. It is an
error to use this,
FYI -- we explicitly support modifying self in SkPath, allowing the 2nd
parameter to be this. We even offer a built-in helper for this:

e.g.

bool SkRRect::transform(const SkMatrix& matrix) {
    return this->transform(matrix, this);
}

[scroggo, 2013-11-04 22:29:55]

On 2013/11/04 20:21:09, reed1 wrote:
> btw -- we *could* support 90-degree rotations, and then we could just use
> matrix::rectStaysRect() to know which matrix variants were valid...

Agreed - I put a comment in the change description that we could support 90
degree rotations. I left it out, though, as it wasn't needed by my use case.

https://codereview.chromium.org/52703003/diff/170001/include/core/SkRRect.h
File include/core/SkRRect.h (right):

https://codereview.chromium.org/52703003/diff/170001/include/core/SkRRect.h#n...
include/core/SkRRect.h:286: *  @param dst SkRRect to store the result. It is an
error to use this,
On 2013/11/04 20:21:09, reed1 wrote:
> FYI -- we explicitly support modifying self in SkPath, allowing the 2nd
> parameter to be this. We even offer a built-in helper for this:
> 
> e.g.
> 
> bool SkRRect::transform(const SkMatrix& matrix) {
>     return this->transform(matrix, this);
> }
> 

In patch set 1 I allowed modifying the SkRRect, following the pattern of SkPath
(though I didn't add the helper), but Robert and I ultimately agreed that we
didn't like that API, which allows modifying a const object through a const
method.

[robertphillips, 2013-11-05 14:15:07]

If we do want to go with the self-modifying API we should get rid of the consts.
As is I think the self-modifying API violates the principle of least surprise.

[scroggo, 2013-11-05 15:54:48]

Committed patchset #5 manually as r12132 (presubmit successful).