Simplify adding attributes to GrGeometryProcessor

Simplify adding attributes to GrGeometryProcessor
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248323003

Committed: https://skia.googlesource.com/skia/+/6cb807bf99ac0f8f166e1790f91bcb3afbfb5458

[bsalomon, 2016-08-17 15:54:29]

Description was changed from

==========
Simplify adding attributes to GrGeometryProcessor
==========

to

==========
Simplify adding attributes to GrGeometryProcessor
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248323003
==========

[bsalomon, 2016-08-17 15:59:00]

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

[bsalomon, 2016-08-17 15:59:01]

Last cleanup CL.

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrGeometryProcessor.h
File src/gpu/GrGeometryProcessor.h (left):

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrGeometryProcessor...
src/gpu/GrGeometryProcessor.h:56: const Attribute& addVertexAttrib(const
Attribute& attribute) {
This is the real change. Everything else is changing the callers (and removing
default Attribute cons arg value in GrPrimitiveProcessor.h)

[robertphillips, 2016-08-17 16:11:32]

lgtm

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrDefaultGeoProcFac...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrDefaultGeoProcFac...
src/gpu/GrDefaultGeoProcFactory.cpp:236: bool hasCoverage = SkToBool(gpTypeFlags
& kCoverage_GPFlag);
So, err, can't future adds move the array in memory and invalidate these
pointers?

I know the base class has a preallocated block of memory but still ...

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
File tests/PrimitiveProcessorTest.cpp (right):

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
tests/PrimitiveProcessorTest.cpp:54: SkASSERT(numAttribs > 1);
fAttribNames doesn't seem particularly useful.

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
tests/PrimitiveProcessorTest.cpp:58: for (auto i = 0; i < numAttribs; ++i) {
rm attribute ?

[bsalomon, 2016-08-17 16:14:31]

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrDefaultGeoProcFac...
File src/gpu/GrDefaultGeoProcFactory.cpp (right):

https://codereview.chromium.org/2248323003/diff/1/src/gpu/GrDefaultGeoProcFac...
src/gpu/GrDefaultGeoProcFactory.cpp:236: bool hasCoverage = SkToBool(gpTypeFlags
& kCoverage_GPFlag);
On 2016/08/17 16:11:32, robertphillips wrote:
> So, err, can't future adds move the array in memory and invalidate these
> pointers?
> 
> I know the base class has a preallocated block of memory but still ...

Indeed! Will address in another CL.

[bsalomon, 2016-08-17 16:17:44]

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
File tests/PrimitiveProcessorTest.cpp (right):

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
tests/PrimitiveProcessorTest.cpp:54: SkASSERT(numAttribs > 1);
On 2016/08/17 16:11:32, robertphillips wrote:
> fAttribNames doesn't seem particularly useful.

Done.

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
tests/PrimitiveProcessorTest.cpp:58: for (auto i = 0; i < numAttribs; ++i) {
On 2016/08/17 16:11:32, robertphillips wrote:
> rm attribute ?

Done.

[bsalomon, 2016-08-17 16:19:27]

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
File tests/PrimitiveProcessorTest.cpp (right):

https://codereview.chromium.org/2248323003/diff/1/tests/PrimitiveProcessorTes...
tests/PrimitiveProcessorTest.cpp:54: SkASSERT(numAttribs > 1);
On 2016/08/17 16:17:44, bsalomon wrote:
> On 2016/08/17 16:11:32, robertphillips wrote:
> > fAttribNames doesn't seem particularly useful.
> 
> Done.

Actually, it sadly is useful. The Attribute struct just stores a const char* and
so fAttribNames keeps the pointers valid.

[bsalomon, 2016-08-17 16:21:07]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-08-17 16:21:07]

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/2248323003/#ps40001
(title: "Revert removal of fAttribNames")

[commit-bot: I haz the power, 2016-08-17 16:21:15]

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

[commit-bot: I haz the power, 2016-08-17 16:21:16]

This CL has an open dependency (Issue 2250563004 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[bsalomon, 2016-08-17 17:51:26]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2016-08-17 17:51:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 18:33:41]

Description was changed from

==========
Simplify adding attributes to GrGeometryProcessor
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248323003
==========

to

==========
Simplify adding attributes to GrGeometryProcessor
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248323003

Committed:
https://skia.googlesource.com/skia/+/6cb807bf99ac0f8f166e1790f91bcb3afbfb5458
==========

[commit-bot: I haz the power, 2016-08-17 18:33:42]

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