Refactor GrGLUniformManager::UniformHandle to initialize itself by default

Refactor GrGLUniformManager::UniformHandle to initialize itself by default

Refactor GrGLUniformManager::UniformHandle to initialize itself to
"invalid" state by default. This simplifies the effect
constructors. In the future, it should also help catch potential
uninitialized uniform variable usage.

Remove unneeded explicit uniform handle validity assertions before the
handle usage. The assertion will always be made when handle is
converted to index.

BUG=skia:1492
Committed: http://code.google.com/p/skia/source/detail?r=10713

[bsalomon, 2013-08-09 13:19:40]

Mostly looks good, one request.

https://codereview.chromium.org/22340010/diff/1/src/gpu/gl/GrGLUniformManager.h
File src/gpu/gl/GrGLUniformManager.h (right):

https://codereview.chromium.org/22340010/diff/1/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.h:29: int toUniformIndex() const {
GrAssert(isValid()); return ~fHandle; }
Can we make this private to GrGLUniformManager?

I was thinking either by making it private in UniformHandle and friending
GrGLUniformManager, or by making it a static function of GrGLUniformManager?

The only reason this type really exists is to discourage effect writers from
ever doing their own uploads outside of the manager.

[Kimmo Kinnunen, 2013-08-12 08:57:17]

On 2013/08/09 13:19:40, bsalomon wrote:
> src/gpu/gl/GrGLUniformManager.h:29: int toUniformIndex() const {
> GrAssert(isValid()); return ~fHandle; }
> Can we make this private to GrGLUniformManager?

I don't think it makes sense with the current code. The handle would be
redundant. (Not that it has too much work to do even today..)

Uploaded a version which makes the index totally encapsulated to
GrGLUniformManager..

[Kimmo Kinnunen, 2013-08-12 12:53:44]

If need be, I can also upload a version which:
* Renames GrGLUniformManager to GrGLUniform
* Moves list of GrGLUniform to GrGLProgram
* Removes the back-forth mapping between emit time uniform info (BuilderUniform)
and run-time time uniform info (GrGLUniform)
* Makes a direct mapping BuilderUniform -> GrGLUniform
* Preserves BuilderUniform instances to live through emit only (similarly to
today)
* Moves BuilderUniform logic, including list maintainance, fully to Builder

That should properly contain the code related to emit time in Builder and
run-time refs in GrGLUniform.

[bsalomon, 2013-08-12 13:05:50]

On 2013/08/12 08:57:17, kkinnunen wrote:
> On 2013/08/09 13:19:40, bsalomon wrote:
> > src/gpu/gl/GrGLUniformManager.h:29: int toUniformIndex() const {
> > GrAssert(isValid()); return ~fHandle; }
> > Can we make this private to GrGLUniformManager?
> 
> I don't think it makes sense with the current code. The handle would be
> redundant. (Not that it has too much work to do even today..)
> 
> Uploaded a version which makes the index totally encapsulated to
> GrGLUniformManager..


My concern with the latest version is the pointers back into the UniformManager.
At some point we will want to stabilize the API for creating GrEffects and allow
clients to have their own GrEffect classes compiled outside of Skia. (The main
reason we haven't is that a lot of this was hastily/incrementally written and
could use some refactoring.) With opaque handles it is easier for us to change
the manager's implementation without breaking. (I suppose that can be done by
casting the pointers to indices or whatever but that feels bad.)

[Kimmo Kinnunen, 2013-08-13 09:00:20]

On 2013/08/12 13:05:50, bsalomon wrote:
> On 2013/08/12 08:57:17, kkinnunen wrote:
> > On 2013/08/09 13:19:40, bsalomon wrote:
> > > src/gpu/gl/GrGLUniformManager.h:29: int toUniformIndex() const {
> > > GrAssert(isValid()); return ~fHandle; }
> > > Can we make this private to GrGLUniformManager?
> > 
> > I don't think it makes sense with the current code. The handle would be
> > redundant. (Not that it has too much work to do even today..)
> > 
> > Uploaded a version which makes the index totally encapsulated to
> > GrGLUniformManager..
> 
> 
> My concern with the latest version is the pointers back into the
UniformManager.
> At some point we will want to stabilize the API for creating GrEffects and
allow
> clients to have their own GrEffect classes compiled outside of Skia.

I reverted to the original and added the friends.

> (The main
> reason we haven't is that a lot of this was hastily/incrementally written and
> could use some refactoring.) With opaque handles it is easier for us to change
> the manager's implementation without breaking. (I suppose that can be done by
> casting the pointers to indices or whatever but that feels bad.)

I'm certainly not well informed enough to read into the code so of course take
following for whatever it is worth:
To me it'd still appear that the code would be significantly different anyway.
Currently it seems the opaqueness is not really opaque, and the handle carries
information that is used outside the encapsulation. This manifests in the
constructs in the manager (which to me look weird)..
I'd fix it something like strawman commit
https://codereview.chromium.org/23018003/ , but of course I don't know about the
public api designs..

[Kimmo Kinnunen, 2013-08-13 09:00:33]

https://codereview.chromium.org/22340010/diff/1/src/gpu/gl/GrGLUniformManager.h
File src/gpu/gl/GrGLUniformManager.h (right):

https://codereview.chromium.org/22340010/diff/1/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.h:29: int toUniformIndex() const {
GrAssert(isValid()); return ~fHandle; }
On 2013/08/09 13:19:40, bsalomon wrote:
> Can we make this private to GrGLUniformManager?
> 
> I was thinking either by making it private in UniformHandle and friending
> GrGLUniformManager, or by making it a static function of GrGLUniformManager?
> 
> The only reason this type really exists is to discourage effect writers from
> ever doing their own uploads outside of the manager.

Done.

[bsalomon, 2013-08-14 14:28:12]

lgtm.

I'm not sure I totally get your concern with respect to opaqueness. The main
point is just to hide the index from the effect-writers so that they aren't
tempted to start calling glUniform* themselves. Someday a new version of the
manager may decide that some "uniforms" are actually vertex attribs because
that's more performant on some platform, or we've packed them somehow, etc.

[commit-bot: I haz the power, 2013-08-14 14:28:26]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/22340010/41001

[commit-bot: I haz the power, 2013-08-14 18:14:31]

Change committed as 10713