drawinfo carries bufferinfo

drawinfo carries bufferinfo

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7eb8c7b00a5d776bebaf33a8687357df95c1aa43

[joshualitt, 2014-11-18 18:21:40]

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

[joshualitt, 2014-11-18 18:21:41]

Brian, it'd be nice to have drawinfo hold the buffer refs, but I couldn't figure
out how to get GrGpuTResource to work with a const pointer

[bsalomon, 2014-11-18 18:41:25]

what breaks w/ GrGpuTResource holding a const ptr?

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

https://codereview.chromium.org/737723003/diff/1/src/gpu/GrDrawTarget.h#newco...
src/gpu/GrDrawTarget.h:564: void setVertexBuffer(const GrVertexBuffer* vb) {
fVertexBuffer = vb; }
This has to be ref'ed, right?

[bsalomon, 2014-11-18 18:41:25]

what breaks w/ GrGpuTResource holding a const ptr?

[joshualitt, 2014-11-18 18:42:51]

On 2014/11/18 18:41:25, bsalomon wrote:
> what breaks w/ GrGpuTResource holding a const ptr?

void setResource(GrGpuResource*, GrIOType);

[bsalomon, 2014-11-18 18:49:00]

On 2014/11/18 18:42:51, joshualitt wrote:
> On 2014/11/18 18:41:25, bsalomon wrote:
> > what breaks w/ GrGpuTResource holding a const ptr?
> 
> void setResource(GrGpuResource*, GrIOType);

Maybe the base class can be made to use const*? I don't remember all the use
cases for it off the top of my head. Or maybe we need a const version of it.

[joshualitt, 2014-11-18 19:18:51]

On 2014/11/18 18:49:00, bsalomon wrote:
> On 2014/11/18 18:42:51, joshualitt wrote:
> > On 2014/11/18 18:41:25, bsalomon wrote:
> > > what breaks w/ GrGpuTResource holding a const ptr?
> > 
> > void setResource(GrGpuResource*, GrIOType);
> 
> Maybe the base class can be made to use const*? I don't remember all the use
> cases for it off the top of my head. Or maybe we need a const version of it.

because of the 'getResource' call, its not trivial to deal with this in the base
class.  The solution I have here uses a const derived class which const casts to
set resource.  I bet we could templatize GrGpuResourceRef, but I'm not sure what
the implications are.

Feedback sort of incorporated.

[bsalomon, 2014-11-18 19:23:48]

On 2014/11/18 19:18:51, joshualitt wrote:
> On 2014/11/18 18:49:00, bsalomon wrote:
> > On 2014/11/18 18:42:51, joshualitt wrote:
> > > On 2014/11/18 18:41:25, bsalomon wrote:
> > > > what breaks w/ GrGpuTResource holding a const ptr?
> > > 
> > > void setResource(GrGpuResource*, GrIOType);
> > 
> > Maybe the base class can be made to use const*? I don't remember all the use
> > cases for it off the top of my head. Or maybe we need a const version of it.
> 
> because of the 'getResource' call, its not trivial to deal with this in the
base
> class.  The solution I have here uses a const derived class which const casts
to
> set resource.  I bet we could templatize GrGpuResourceRef, but I'm not sure
what
> the implications are.
> 
> Feedback sort of incorporated.


Using getResource() let's you get the resource w/out const. Is there a benefit
to the inheritance? The regular T subclass uses a base class only because we
need collections that include different GrGpuResource subclasses.

[joshualitt, 2014-11-18 20:00:10]

On 2014/11/18 19:23:48, bsalomon wrote:
> On 2014/11/18 19:18:51, joshualitt wrote:
> > On 2014/11/18 18:49:00, bsalomon wrote:
> > > On 2014/11/18 18:42:51, joshualitt wrote:
> > > > On 2014/11/18 18:41:25, bsalomon wrote:
> > > > > what breaks w/ GrGpuTResource holding a const ptr?
> > > > 
> > > > void setResource(GrGpuResource*, GrIOType);
> > > 
> > > Maybe the base class can be made to use const*? I don't remember all the
use
> > > cases for it off the top of my head. Or maybe we need a const version of
it.
> > 
> > because of the 'getResource' call, its not trivial to deal with this in the
> base
> > class.  The solution I have here uses a const derived class which const
casts
> to
> > set resource.  I bet we could templatize GrGpuResourceRef, but I'm not sure
> what
> > the implications are.
> > 
> > Feedback sort of incorporated.
> 
> 
> Using getResource() let's you get the resource w/out const. Is there a benefit
> to the inheritance? The regular T subclass uses a base class only because we
> need collections that include different GrGpuResource subclasses.

I think I understood this comment, feedback incorporated.

[joshualitt, 2014-11-18 20:37:19]

On 2014/11/18 20:00:10, joshualitt wrote:
> On 2014/11/18 19:23:48, bsalomon wrote:
> > On 2014/11/18 19:18:51, joshualitt wrote:
> > > On 2014/11/18 18:49:00, bsalomon wrote:
> > > > On 2014/11/18 18:42:51, joshualitt wrote:
> > > > > On 2014/11/18 18:41:25, bsalomon wrote:
> > > > > > what breaks w/ GrGpuTResource holding a const ptr?
> > > > > 
> > > > > void setResource(GrGpuResource*, GrIOType);
> > > > 
> > > > Maybe the base class can be made to use const*? I don't remember all the
> use
> > > > cases for it off the top of my head. Or maybe we need a const version of
> it.
> > > 
> > > because of the 'getResource' call, its not trivial to deal with this in
the
> > base
> > > class.  The solution I have here uses a const derived class which const
> casts
> > to
> > > set resource.  I bet we could templatize GrGpuResourceRef, but I'm not
sure
> > what
> > > the implications are.
> > > 
> > > Feedback sort of incorporated.
> > 
> > 
> > Using getResource() let's you get the resource w/out const. Is there a
benefit
> > to the inheritance? The regular T subclass uses a base class only because we
> > need collections that include different GrGpuResource subclasses.
> 
> I think I understood this comment, feedback incorporated.

feedback incorporated

[bsalomon, 2014-11-18 20:48:31]

https://codereview.chromium.org/737723003/diff/120001/include/gpu/GrGpuResour...
File include/gpu/GrGpuResourceRef.h (right):

https://codereview.chromium.org/737723003/diff/120001/include/gpu/GrGpuResour...
include/gpu/GrGpuResourceRef.h:166: void set(T* resource) {
Shouldn't this check whether the resource was already set and if so release the
pending io?

Also, this looks an awful lot like SkAutoTUnref which to me argues for nameing
the reset() rather than set().

[joshualitt, 2014-11-18 21:12:48]

On 2014/11/18 20:48:31, bsalomon wrote:
>
https://codereview.chromium.org/737723003/diff/120001/include/gpu/GrGpuResour...
> File include/gpu/GrGpuResourceRef.h (right):
> 
>
https://codereview.chromium.org/737723003/diff/120001/include/gpu/GrGpuResour...
> include/gpu/GrGpuResourceRef.h:166: void set(T* resource) {
> Shouldn't this check whether the resource was already set and if so release
the
> pending io?
> 
> Also, this looks an awful lot like SkAutoTUnref which to me argues for nameing
> the reset() rather than set().

feedback incorporated

[joshualitt, 2014-11-18 21:33:18]

feedback incorporated

[bsalomon, 2014-11-18 22:05:27]

https://codereview.chromium.org/737723003/diff/160001/include/gpu/GrGpuResour...
File include/gpu/GrGpuResourceRef.h (right):

https://codereview.chromium.org/737723003/diff/160001/include/gpu/GrGpuResour...
include/gpu/GrGpuResourceRef.h:181: if (fResource) {
why not just this->release(), since it does the check.

otherwise, lgtm.

[joshualitt, 2014-11-18 22:15:14]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-18 22:15:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/737723003/180001

[commit-bot: I haz the power, 2014-11-18 22:24:33]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/7eb8c7b00a5d776bebaf33a8687357df95c1aa43