Move code that populates program desc to GrGLProgram. Move color and coverage flush to GrGLProgram

Move code that builds GrGLProgram::Desc to GrGLProgram. Move color and coverage flush to GrGLProgram.
Committed: https://code.google.com/p/skia/source/detail?r=7708

[bsalomon, 2013-02-12 18:31:31]

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.h
File src/gpu/gl/GrGLProgram.h (right):

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.h#new...
src/gpu/gl/GrGLProgram.h:270: friend class GrGpuGL; // TODO: remove this by
adding getters and moving functionality.
This CL almost lets us end this friendship. The next one does.

[robertphillips, 2013-02-12 19:03:21]

lgtm + suggestions.

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:73: 
the. -> the

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:82: 
one line for this comment?

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:91: bool colorIsSolidWhite = (blendOpts &
GrDrawState::kEmitCoverage_BlendOptFlag) ||
symbolic version of 0xffffffff?

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:116: 
space after &? Do we need a ToBool?

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:142: // other than pass through values from the VS to
the FS anyway).
Is it initialized to a canonical value somewhere?

https://codereview.appspot.com/7322058/diff/2001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/2001/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:65: }
what's up with this else block?

[bsalomon, 2013-02-12 19:36:37]

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:73: 
On 2013/02/12 19:03:21, robertphillips wrote:
> the. -> the

Done.

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:82: 
On 2013/02/12 19:03:21, robertphillips wrote:
> one line for this comment?

Done.

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:91: bool colorIsSolidWhite = (blendOpts &
GrDrawState::kEmitCoverage_BlendOptFlag) ||
On 2013/02/12 19:03:21, robertphillips wrote:
> symbolic version of 0xffffffff?

Hmm... we don't have one for GrColor and we use 0xffffffff lots of places. Since
this is a code move I think I'll defer to another CL that cleans this up across
the board.

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:116: 
On 2013/02/12 19:03:21, robertphillips wrote:
> space after &? Do we need a ToBool?

Added space. Shouldn't need ToBool unless we assign to a bool (or return as a
bool)

https://codereview.appspot.com/7322058/diff/2001/src/gpu/gl/GrGLProgram.cpp#n...
src/gpu/gl/GrGLProgram.cpp:142: // other than pass through values from the VS to
the FS anyway).
On 2013/02/12 19:03:21, robertphillips wrote:
> Is it initialized to a canonical value somewhere?

Fixed.

https://codereview.appspot.com/7322058/diff/2001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/2001/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:65: }
On 2013/02/12 19:03:21, robertphillips wrote:
> what's up with this else block?

Duh... removed.

[JimVV, 2013-02-12 19:54:40]

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:41: fEmitsPointSize = random->nextF() > .5f;
Why not use random->nextBool()?

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:48: 
In the short term the routines above will be less random than using the old
random_int(), unless you want to try switching to SkMWCRandom.

[bsalomon, 2013-02-12 19:57:54]

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:41: fEmitsPointSize = random->nextF() > .5f;
On 2013/02/12 19:54:41, JimVV wrote:
> Why not use random->nextBool()?

no good reason... this is copied from really old code that used the now
nonexistent class GrRandom which had a nextBool() that alternated true/false.
Will change.

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:48: 
On 2013/02/12 19:54:41, JimVV wrote:
> In the short term the routines above will be less random than using the old
> random_int(), unless you want to try switching to SkMWCRandom.

will switch.

[bsalomon, 2013-02-12 21:31:53]

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:41: fEmitsPointSize = random->nextF() > .5f;
On 2013/02/12 19:57:54, bsalomon wrote:
> On 2013/02/12 19:54:41, JimVV wrote:
> > Why not use random->nextBool()?
> 
> no good reason... this is copied from really old code that used the now
> nonexistent class GrRandom which had a nextBool() that alternated true/false.
> Will change.

Done.

https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:48: 
On 2013/02/12 19:57:54, bsalomon wrote:
> On 2013/02/12 19:54:41, JimVV wrote:
> > In the short term the routines above will be less random than using the old
> > random_int(), unless you want to try switching to SkMWCRandom.
> 
> will switch.

Done.

https://codereview.appspot.com/7322058/diff/8002/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.appspot.com/7322058/diff/8002/tests/GLProgramsTest.cpp#new...
tests/GLProgramsTest.cpp:28: // TODO: Make GrTestFactory use SkMWCRandom and
simply pass along param random
Passing along an SkMWCRandom to the effect factories means changing a whole lot
more files. I'll submit that as a follow-up CL.

[JimVV, 2013-02-12 21:44:39]

LGTM

On 2013/02/12 21:31:53, bsalomon wrote:
> https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp
> File tests/GLProgramsTest.cpp (right):
> 
>
https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
> tests/GLProgramsTest.cpp:41: fEmitsPointSize = random->nextF() > .5f;
> On 2013/02/12 19:57:54, bsalomon wrote:
> > On 2013/02/12 19:54:41, JimVV wrote:
> > > Why not use random->nextBool()?
> > 
> > no good reason... this is copied from really old code that used the now
> > nonexistent class GrRandom which had a nextBool() that alternated
true/false.
> > Will change.
> 
> Done.
> 
>
https://codereview.appspot.com/7322058/diff/6002/tests/GLProgramsTest.cpp#new...
> tests/GLProgramsTest.cpp:48: 
> On 2013/02/12 19:57:54, bsalomon wrote:
> > On 2013/02/12 19:54:41, JimVV wrote:
> > > In the short term the routines above will be less random than using the
old
> > > random_int(), unless you want to try switching to SkMWCRandom.
> > 
> > will switch.
> 
> Done.
> 
> https://codereview.appspot.com/7322058/diff/8002/tests/GLProgramsTest.cpp
> File tests/GLProgramsTest.cpp (right):
> 
>
https://codereview.appspot.com/7322058/diff/8002/tests/GLProgramsTest.cpp#new...
> tests/GLProgramsTest.cpp:28: // TODO: Make GrTestFactory use SkMWCRandom and
> simply pass along param random
> Passing along an SkMWCRandom to the effect factories means changing a whole
lot
> more files. I'll submit that as a follow-up CL.