fill rect batch refactor into separate batches

fill rect batch refactor into separate batches

BUG=skia:

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

Committed: https://skia.googlesource.com/skia/+/2244c27ea4db85df305fa09f664b7d75f637e7a9

[joshualitt, 2015-08-14 14:27:16]

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

[joshualitt, 2015-08-14 14:27:16]

ptal

[bsalomon, 2015-08-14 14:37:48]

General question:  Why have variants with local coords and with local coords and
local matrices? Seems like it'd be simpler conceptually and in implementation to
just track 4 explicit LCs per rect. That is, apply the LM if there is one when
recording the geometry and be done with it. No reason they shouldn't be able to
batch together.

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
File src/gpu/batches/GrBWFillRectBatch.cpp (right):

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:41: template <typename Base>
I don't Base is a good a name for the template parameter... it's not a Base
class.

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:194: * Right now we have 4 variants of
BWFillRect.  BWFillRect with no local matrix or local rect,
Maybe write this as bullets? Kind of hard to follow.

Comment on why this is has a templated Base class... pretty confusing, should be
carefully documented.

[robertphillips, 2015-08-14 14:42:57]

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
File src/gpu/batches/GrBWFillRectBatch.cpp (right):

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:102: geom.fRect.fRight,
geom.fRect.fBottom, vertexStride);
space before '(' ?

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:196: * applies its localMatrix to its
localRect
// These classes are created by having two levels of templating. The first level
creates templated versions of GrBWFillRectBatchNoLocalRectImp and
GrBWFillRectBatchLocalRectImp ...

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:215: inline static void
FillInAttributes(intptr_t vertices, size_t vertexStride,
We don't have to invoke "Base::OnFillInAttributes(coords, vertexStride, args);"
here?

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:225: bool usesLocalCoords) {
// We can always combine b.c. ...
?

https://codereview.chromium.org/1295773003/diff/1/src/gpu/batches/GrBWFillRec...
src/gpu/batches/GrBWFillRectBatch.cpp:300: inline static void
OnFillInAttributes(SkPoint* coords, size_t vertexStride,
call base class ?

[joshualitt, 2015-08-18 20:22:51]

Okay, ptal.  This isn't with the GrQuad change, but I'd like to land it before
doing the GrQuad change if possible

[bsalomon, 2015-08-18 20:31:04]

lgtm

https://codereview.chromium.org/1295773003/diff/40001/src/gpu/batches/GrAAFil...
File src/gpu/batches/GrAAFillRectBatch.cpp (right):

https://codereview.chromium.org/1295773003/diff/40001/src/gpu/batches/GrAAFil...
src/gpu/batches/GrAAFillRectBatch.cpp:190: inline static const GrIndexBuffer*
GetIndexBuffer(GrResourceProvider* rp) {
Do these all need to be explicitly inline?

[joshualitt, 2015-08-19 13:44:31]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-08-19 13:44:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1295773003/#ps60001
(title: "tweaks")

[commit-bot: I haz the power, 2015-08-19 13:44:45]

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

[commit-bot: I haz the power, 2015-08-19 13:54:11]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/ae41b3834301444cf27b8aa729b8ad36666bdc08

[joshualitt, 2015-08-20 13:48:37]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1302033003/ by joshualitt@google.com.

The reason for reverting is: revert because breaks some mac bots in floating
point texture test.

[joshualitt, 2015-08-20 15:39:45]

On 2015/08/20 13:48:37, joshualitt wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1302033003/ by mailto:joshualitt@google.com.
> 
> The reason for reverting is: revert because breaks some mac bots in floating
> point texture test.

ptal, I'll land this after the branch

[robertphillips, 2015-08-21 17:20:56]

https://codereview.chromium.org/1295773003/diff/140001/src/gpu/batches/GrAAFi...
File src/gpu/batches/GrAAFillRectBatch.cpp (right):

https://codereview.chromium.org/1295773003/diff/140001/src/gpu/batches/GrAAFi...
src/gpu/batches/GrAAFillRectBatch.cpp:197: 
Do we have a comment somewhere saying the 'Geometry' parameter needs a
'fDevRect' member ?

https://codereview.chromium.org/1295773003/diff/140001/src/gpu/batches/GrBWFi...
File src/gpu/batches/GrBWFillRectBatch.cpp (right):

https://codereview.chromium.org/1295773003/diff/140001/src/gpu/batches/GrBWFi...
src/gpu/batches/GrBWFillRectBatch.cpp:44: 
color param ?

https://codereview.chromium.org/1295773003/diff/140001/src/gpu/batches/GrBWFi...
src/gpu/batches/GrBWFillRectBatch.cpp:63: } else {
localMatrix ? localMatrix : NULL ?

[joshualitt, 2015-08-21 17:26:55]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-08-21 17:26:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1295773003/#ps180001
(title: "tweaks")

[commit-bot: I haz the power, 2015-08-21 17:27:06]

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

[commit-bot: I haz the power, 2015-08-21 17:33:20]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/2244c27ea4db85df305fa09f664b7d75f637e7a9