Generating gl processor key is now recursive

For a frag proc, its key will be a concatenation of all its descendant procs' keys in postorder traversal.  

BUG=skia:4182
Committed: https://skia.googlesource.com/skia/+/a7f4c435bc1dcd845990a5515828bbe8cccfab41

[wangyix, 2015-08-19 14:01:30]

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

[wangyix, 2015-08-19 14:01:30]

I haven't added the number of children to the metakey yet; should all procs have
that field added or just fragment processors?

[tomhudson, 2015-08-19 14:27:29]

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcessor.h
File src/gpu/GrPrimitiveProcessor.h (right):

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcesso...
src/gpu/GrPrimitiveProcessor.h:210: uint32_t getTransformKey(const
SkTArray<const GrCoordTransform*, true>&, int count) const;
Count of what? Purpose of parameter is neither obvious from name nor documented
in function comment.

(index? might be slightly better)

(It's really not obvious why they can't just do array[count]. Is the right name
for this function *get*TransformKey(), or is it really *compute*TransformKey()?)

This function doesn't seem to depend on any GrPrimitiveProcessor instance
members; it's a function of a GrCoordTransform. Why is it not on that class, or
a static/namespaced helper function?

https://codereview.chromium.org/1297503007/diff/1/src/gpu/gl/GrGLProgramDesc.cpp
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/1297503007/diff/1/src/gpu/gl/GrGLProgramDesc....
src/gpu/gl/GrGLProgramDesc.cpp:101: //**** use glslCaps here?
Either mark this as a TODO and file a bug, or remove, or otherwise answer the
question before landing.

[wangyix, 2015-08-19 17:53:48]

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcessor.h
File src/gpu/GrPrimitiveProcessor.h (right):

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcesso...
src/gpu/GrPrimitiveProcessor.h:210: uint32_t getTransformKey(const
SkTArray<const GrCoordTransform*, true>&, int count) const;
On 2015/08/19 14:27:29, tomhudson wrote:
> Count of what? Purpose of parameter is neither obvious from name nor
documented
> in function comment.
> 
> (index? might be slightly better)
> 
> (It's really not obvious why they can't just do array[count]. Is the right
name
> for this function *get*TransformKey(), or is it really
*compute*TransformKey()?)
> 
> This function doesn't seem to depend on any GrPrimitiveProcessor instance
> members; it's a function of a GrCoordTransform. Why is it not on that class,
or
> a static/namespaced helper function?


This function does call this->hasExplicitLocalCoords(), but that's the only
place it depends on the instance.

We are indeed computing something in this function, but the other methods that
compute parts of the key are also start with "get": getGLProcessorKey,
get_meta_key.

Josh, should we change these so they don't say "get"?

[joshualitt, 2015-08-19 19:07:48]

lgtm

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcessor.h
File src/gpu/GrPrimitiveProcessor.h (right):

https://codereview.chromium.org/1297503007/diff/1/src/gpu/GrPrimitiveProcesso...
src/gpu/GrPrimitiveProcessor.h:210: uint32_t getTransformKey(const
SkTArray<const GrCoordTransform*, true>&, int count) const;
On 2015/08/19 17:53:48, wangyix wrote:
> On 2015/08/19 14:27:29, tomhudson wrote:
> > Count of what? Purpose of parameter is neither obvious from name nor
> documented
> > in function comment.
> > 
> > (index? might be slightly better)
> > 
> > (It's really not obvious why they can't just do array[count]. Is the right
> name
> > for this function *get*TransformKey(), or is it really
> *compute*TransformKey()?)
> > 
> > This function doesn't seem to depend on any GrPrimitiveProcessor instance
> > members; it's a function of a GrCoordTransform. Why is it not on that class,
> or
> > a static/namespaced helper function?
> 
> 
> This function does call this->hasExplicitLocalCoords(), but that's the only
> place it depends on the instance.
> 
> We are indeed computing something in this function, but the other methods that
> compute parts of the key are also start with "get": getGLProcessorKey,
> get_meta_key.
> 
> Josh, should we change these so they don't say "get"?

I think a comment is a good idea, with a TODO noting we plan on changing this
design because it is clunky.

As Yixin mentions, this function does depend on hasExplicitLocalCoords.  We need
to figure out how to handle that more elegantly, but it outside the scope of
this change.

Lets leave the 'get' for the time being.  I do think compute would be better,
but we already use get* everywhere else.

[wangyix, 2015-08-19 19:27:57]

Patchset #1 (id:1) has been deleted

[wangyix, 2015-08-20 14:18:27]

The CQ bit was checked by wangyix@google.com

[wangyix, 2015-08-20 14:18:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com
Link to the patchset: https://codereview.chromium.org/1297503007/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2015-08-20 14:18:38]

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

[commit-bot: I haz the power, 2015-08-20 14:25:05]

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