Add bounds to GrShape

Add bounds to GrShape
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1970003003

Committed: https://skia.googlesource.com/skia/+/9fb420393ee1c24fc3282b7155985fa8fa7bcad4

[bsalomon, 2016-05-12 16:49:57]

Description was changed from

==========
Add bounds to GrShape
==========

to

==========
Add bounds to GrShape
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-05-12 16:55:17]

bsalomon@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2016-05-12 17:18:47]

Questions, questions, questions

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.cpp
File src/gpu/GrShape.cpp (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.cpp#new...
src/gpu/GrShape.cpp:43: case Type::kEmpty:
Why not just return SkRect::MakeEmpty here ?

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.cpp#new...
src/gpu/GrShape.cpp:53: if (Type::kEmpty == fType &&
!fStyle.hasNonDashPathEffect()) {
Is it an empty path or a path with a single point that becomes a circle when
stroked with circular caps?

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h
File src/gpu/GrShape.h (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h#newco...
src/gpu/GrShape.h:151: /** Gets the bounds of the geometry without reflecting
the shape's styling. */
Can this be a const method ?

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h#newco...
src/gpu/GrShape.h:154: /** Gets the bounds of the geometry reflecting the
shape's styling. */
This too ?

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h
File src/gpu/GrStyle.h (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h#newco...
src/gpu/GrStyle.h:169: this->pathEffect()->computeFastBounds(dst, src);
Do we want to share the inflation code between the two code paths?

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h#newco...
src/gpu/GrStyle.h:172: SkScalar radius = fStrokeRec.getInflationRadius();
Would "dst->outset(radius, radius);" be clearer?

https://codereview.chromium.org/1970003003/diff/20001/tests/GrShapeTest.cpp
File tests/GrShapeTest.cpp (right):

https://codereview.chromium.org/1970003003/diff/20001/tests/GrShapeTest.cpp#n...
tests/GrShapeTest.cpp:42: matrix.setRectToRect(bounds, clip,
SkMatrix::kFill_ScaleToFit);
Is this outset really kosher? If we're going to use the bounds for reordering,
shouldn't it be precise as is?

[bsalomon, 2016-05-12 18:37:58]

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.cpp
File src/gpu/GrShape.cpp (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.cpp#new...
src/gpu/GrShape.cpp:43: case Type::kEmpty:
On 2016/05/12 17:18:47, robertphillips wrote:
> Why not just return SkRect::MakeEmpty here ?

Becausethe return is a const& to a SkRect, so I can't return a temporary.

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h
File src/gpu/GrShape.h (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h#newco...
src/gpu/GrShape.h:151: /** Gets the bounds of the geometry without reflecting
the shape's styling. */
On 2016/05/12 17:18:47, robertphillips wrote:
> Can this be a const method ?

Done.

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrShape.h#newco...
src/gpu/GrShape.h:154: /** Gets the bounds of the geometry reflecting the
shape's styling. */
On 2016/05/12 17:18:47, robertphillips wrote:
> This too ?

Done.

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h
File src/gpu/GrStyle.h (right):

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h#newco...
src/gpu/GrStyle.h:169: this->pathEffect()->computeFastBounds(dst, src);
On 2016/05/12 17:18:47, robertphillips wrote:
> Do we want to share the inflation code between the two code paths?

I think this line will be removed when/if the bug is fixed and having it as its
own line gives a place for the comment to hang.

https://codereview.chromium.org/1970003003/diff/20001/src/gpu/GrStyle.h#newco...
src/gpu/GrStyle.h:172: SkScalar radius = fStrokeRec.getInflationRadius();
On 2016/05/12 17:18:47, robertphillips wrote:
> Would "dst->outset(radius, radius);" be clearer?

Done.

https://codereview.chromium.org/1970003003/diff/20001/tests/GrShapeTest.cpp
File tests/GrShapeTest.cpp (right):

https://codereview.chromium.org/1970003003/diff/20001/tests/GrShapeTest.cpp#n...
tests/GrShapeTest.cpp:42: matrix.setRectToRect(bounds, clip,
SkMatrix::kFill_ScaleToFit);
On 2016/05/12 17:18:47, robertphillips wrote:
> Is this outset really kosher? If we're going to use the bounds for reordering,
> shouldn't it be precise as is?

For the current set of test cases it works at 0 (at least on my machine),
however I wouldn't be surprised if we eventually need a tolerance. We're doing
fp math on the bounds above to compute a matrix that then gets applied to the
geometry when it is drawn. The sw rasterizer will do its own rasterization with
its own approximations. I'll set it to zero for now and we can change it later
if necessary.

[bsalomon, 2016-05-12 18:41:51]

The CQ bit was checked by bsalomon@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 18:41:56]

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

[robertphillips, 2016-05-12 18:44:03]

lgtm

[commit-bot: I haz the power, 2016-05-12 18:44:26]

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

[commit-bot: I haz the power, 2016-05-12 18:44:27]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[bsalomon, 2016-05-12 19:15:47]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-12 19:15:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1970003003/#ps80001
(title: "Fix gcc warning and maybe workaround vc internal compile error")

[commit-bot: I haz the power, 2016-05-12 19:15:50]

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

[commit-bot: I haz the power, 2016-05-12 19:19:00]

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

[commit-bot: I haz the power, 2016-05-12 19:19:01]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bsalomon, 2016-05-12 19:22:33]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-12 19:22:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1970003003/#ps100001
(title: "try again to workaround vc internal error")

[commit-bot: I haz the power, 2016-05-12 19:22:40]

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

[commit-bot: I haz the power, 2016-05-12 19:25:50]

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

[commit-bot: I haz the power, 2016-05-12 19:25:51]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bsalomon, 2016-05-13 16:00:27]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-13 16:00:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1970003003/#ps120001
(title: "Work around windows compiler bug")

[commit-bot: I haz the power, 2016-05-13 16:00:30]

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

[bsalomon, 2016-05-13 16:00:42]

The CQ bit was unchecked by bsalomon@google.com

[bsalomon, 2016-05-13 16:01:52]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-13 16:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1970003003/#ps140001
(title: "put back const&")

[commit-bot: I haz the power, 2016-05-13 16:02:03]

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

[commit-bot: I haz the power, 2016-05-13 16:23:42]

Description was changed from

==========
Add bounds to GrShape
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add bounds to GrShape
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/9fb420393ee1c24fc3282b7155985fa8fa7bcad4
==========

[commit-bot: I haz the power, 2016-05-13 16:23:43]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/9fb420393ee1c24fc3282b7155985fa8fa7bcad4