Cleanup Default Geo Proc API

Cleanup Default Geo Proc API

TBR=bsalomon@google.com
BUG=skia:

Committed: https://skia.googlesource.com/skia/+/e9d6095674979a9b843c291b85746922e9ac181a

[joshualitt, 2015-07-27 16:06:54]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com

[joshualitt, 2015-07-27 16:06:54]

Brian, this is just a short CL to get your feedback on the API redesign before I
roll it out.  Thoughts?

[joshualitt, 2015-07-27 16:07:27]

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

[joshualitt, 2015-07-27 16:07:28]

adding Robert who might also have some thoughts.

[bsalomon, 2015-07-27 16:19:24]

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
File src/gpu/GrDefaultGeoProcFactory.h (right):

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:87: if (kAttribute_Type == type) {
??

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:112: struct LocalCoords {
Why a struct?

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:116: kHasExplicit_Type,
Is this where we'd add another option, something like kPreTransformed for the
case where we applied CTs on the CPU? I guess we'd need a count too so that the
GP knows how many CTs there are.

[joshualitt, 2015-07-27 17:21:51]

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
File src/gpu/GrDefaultGeoProcFactory.h (right):

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:87: if (kAttribute_Type == type) {
On 2015/07/27 16:19:24, bsalomon wrote:
> ??

This really is just temporary.  I have to do this until
GrDefaultGeoProcFactory's class takes the color struct itself

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:112: struct LocalCoords {
On 2015/07/27 16:19:24, bsalomon wrote:
> Why a struct?

For one, just to have consistency with Color and Coverage.  I could also just do
a namespace.  For another, just in case we want to do something like add a count
for the number of transforms applied on the CPU.

If you prefer just the enum I can change it.

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
src/gpu/GrDefaultGeoProcFactory.h:116: kHasExplicit_Type,
On 2015/07/27 16:19:24, bsalomon wrote:
> Is this where we'd add another option, something like kPreTransformed for the
> case where we applied CTs on the CPU? I guess we'd need a count too so that
the
> GP knows how many CTs there are. 

Yea, exactly.  I think we could either do a count or have kPreTransformed1, 2,
etc enums.  Probably be a bit clunky though, I prefer the explicit count.

[robertphillips, 2015-07-27 17:30:40]

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

https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrRectBatch.cpp...
src/gpu/GrRectBatch.cpp:27: const SkMatrix* localMatrix,
remove usesLocalCoords ?

[joshualitt, 2015-07-27 18:01:07]

On 2015/07/27 17:30:40, robertphillips wrote:
> https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrRectBatch.cpp
> File src/gpu/GrRectBatch.cpp (right):
> 
>
https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrRectBatch.cpp...
> src/gpu/GrRectBatch.cpp:27: const SkMatrix* localMatrix,
> remove usesLocalCoords ?

done

[bsalomon, 2015-07-27 18:12:09]

On 2015/07/27 17:21:51, joshualitt wrote:
>
https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
> File src/gpu/GrDefaultGeoProcFactory.h (right):
> 
>
https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
> src/gpu/GrDefaultGeoProcFactory.h:87: if (kAttribute_Type == type) {
> On 2015/07/27 16:19:24, bsalomon wrote:
> > ??
> 
> This really is just temporary.  I have to do this until
> GrDefaultGeoProcFactory's class takes the color struct itself
> 
>
https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
> src/gpu/GrDefaultGeoProcFactory.h:112: struct LocalCoords {
> On 2015/07/27 16:19:24, bsalomon wrote:
> > Why a struct?
> 
> For one, just to have consistency with Color and Coverage.  I could also just
do
> a namespace.  For another, just in case we want to do something like add a
count
> for the number of transforms applied on the CPU.
> 
> If you prefer just the enum I can change it.
> 
>
https://codereview.chromium.org/1253393002/diff/20001/src/gpu/GrDefaultGeoPro...
> src/gpu/GrDefaultGeoProcFactory.h:116: kHasExplicit_Type,
> On 2015/07/27 16:19:24, bsalomon wrote:
> > Is this where we'd add another option, something like kPreTransformed for
the
> > case where we applied CTs on the CPU? I guess we'd need a count too so that
> the
> > GP knows how many CTs there are. 
> 
> Yea, exactly.  I think we could either do a count or have kPreTransformed1, 2,
> etc enums.  Probably be a bit clunky though, I prefer the explicit count.

All sgtm. If we're going to add a count (which I think we should) then I'd say
keep the struct, otherwise I'd make an enum.

[joshualitt, 2015-07-27 18:38:28]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-07-27 18:38:33]

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

[commit-bot: I haz the power, 2015-07-27 18:38:34]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-07-28 00:38 UTC

[joshualitt, 2015-07-27 19:12:38]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2015-07-27 19:12:38]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-07-27 19:12:46]

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

[commit-bot: I haz the power, 2015-07-27 19:13:16]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/e9d6095674979a9b843c291b85746922e9ac181a