Made isEqual in GrFragmentProcessor recursive

Made isEqual in GrFragmentProcessor recursive

Added comment about how computeInvariantOutput() is non-recursive in GrFragmentProcessor


Made isEqual() recursive in GrFragmentProcessor

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

[wangyix, 2015-08-14 20:25:49]

Patchset #1 (id:1) has been deleted

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

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

[joshualitt, 2015-08-17 17:20:03]

lgtm

[tomhudson, 2015-08-17 17:33:10]

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:88: * what invariants its output can have
given its children.
Please clarify: It's hard for me to understand how any proc can return a value
here without inspecting its children.

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

https://codereview.chromium.org/1287343005/diff/20001/src/gpu/GrProcessor.cpp...
src/gpu/GrProcessor.cpp:139: if (this->numTransforms() != that.numTransforms())
{
Why does the number of transforms matter IF we're told to ignore them?

[joshualitt, 2015-08-17 17:38:42]

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:88: * what invariants its output can have
given its children.
On 2015/08/17 17:33:10, tomhudson wrote:
> Please clarify: It's hard for me to understand how any proc can return a value
> here without inspecting its children.

I think the confusion here is that the parent can combine its children's output
anyway it wants, including a recursive implementation.  Maybe we want to say 'it
is up to the parent to fully implement this function, no default recursive
implementation is supplied'

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

https://codereview.chromium.org/1287343005/diff/20001/src/gpu/GrProcessor.cpp...
src/gpu/GrProcessor.cpp:139: if (this->numTransforms() != that.numTransforms())
{
On 2015/08/17 17:33:10, tomhudson wrote:
> Why does the number of transforms matter IF we're told to ignore them?

Its a bit of a misnomer, it actually just ignores the CoordTransform matrices.

[bsalomon, 2015-08-17 18:04:16]

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:88: * what invariants its output can have
given its children.
On 2015/08/17 17:38:41, joshualitt wrote:
> On 2015/08/17 17:33:10, tomhudson wrote:
> > Please clarify: It's hard for me to understand how any proc can return a
value
> > here without inspecting its children.
> 
> I think the confusion here is that the parent can combine its children's
output
> anyway it wants, including a recursive implementation.  Maybe we want to say
'it
> is up to the parent to fully implement this function, no default recursive
> implementation is supplied'

Perhaps the comment should be on the virtual and not on the public non-virtual
since this is info for a subclass implementor and not for someone writing code
that uses this class.

[wangyix, 2015-08-17 19:07:46]

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/20001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:88: * what invariants its output can have
given its children.
On 2015/08/17 18:04:16, bsalomon wrote:
> On 2015/08/17 17:38:41, joshualitt wrote:
> > On 2015/08/17 17:33:10, tomhudson wrote:
> > > Please clarify: It's hard for me to understand how any proc can return a
> value
> > > here without inspecting its children.
> > 
> > I think the confusion here is that the parent can combine its children's
> output
> > anyway it wants, including a recursive implementation.  Maybe we want to say
> 'it
> > is up to the parent to fully implement this function, no default recursive
> > implementation is supplied'
> 
> Perhaps the comment should be on the virtual and not on the public non-virtual
> since this is info for a subclass implementor and not for someone writing code
> that uses this class.
> 

Done.

[bsalomon, 2015-08-18 14:15:43]

otherwise lgtm

https://codereview.chromium.org/1287343005/diff/40001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/40001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:87: * Note: this function will NOT be
recursive; it will be up to the parent proc to figure out
Let's remove the comment here and just leave the one on onCIO.

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

https://codereview.chromium.org/1287343005/diff/40001/include/gpu/GrFragmentP...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1287343005/diff/40001/include/gpu/GrFragmentP...
include/gpu/GrFragmentProcessor.h:87: * Note: this function will NOT be
recursive; it will be up to the parent proc to figure out
On 2015/08/18 14:15:43, bsalomon wrote:
> Let's remove the comment here and just leave the one on onCIO.

Done.

[wangyix, 2015-08-18 14:21:07]

The CQ bit was checked by wangyix@google.com

[wangyix, 2015-08-18 14:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1287343005/#ps60001
(title: "removed comment on computeInvariantOutput")

[commit-bot: I haz the power, 2015-08-18 14:21:19]

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

[commit-bot: I haz the power, 2015-08-18 14:21:19]

This CL has an open dependency (Issue 1286293002 Patch 80001). Please resolve
the dependency and try again.

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

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2015-08-18 14:27:52]

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

[commit-bot: I haz the power, 2015-08-18 14:39:36]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/54017d7e5b09e7ca58346ea72ff065677fcd7be1