Bug 40176 – Fix WebGLRenderingContext helper functions find{Texture/Renderbuffer/Buffer}

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

1) Need to implement findBuffer and remove the second constructor in WebGLBuffer
2) For find functions, when input Platform3DObject is 0, need to return 0 instead of going through the cached set.  Otherwise, a deleted Object (whose name became 0) may be wrongly returned.

[Zhenyao Mo]

Created attachment 58283 [details]
patch

[Kenneth Russell]

Comment on attachment 58283 [details]
patch

Looks good.

[Dimitri Glazkov (Google)]

Comment on attachment 58283 [details]
patch


WebCore/html/canvas/WebGLRenderingContext.h:304
 +          PassRefPtr<WebGLBuffer> findBuffer(Platform3DObject);
Why are these all PassRefPtrs? We're not transferring ownership here, right?

[Zhenyao Mo]

(In reply to comment #3)
> (From update of attachment 58283 [details])
> 
> WebCore/html/canvas/WebGLRenderingContext.h:304
>  +          PassRefPtr<WebGLBuffer> findBuffer(Platform3DObject);
> Why are these all PassRefPtrs? We're not transferring ownership here, right?

After further investigation, I think the return type actually should be PassRefPtr.  Because the only receiver of the returned data is WebGLGetInfo, and WebGLGetInfo constructor takes PassRefPtr as input.

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 58283 [details] [details])
> > 
> > WebCore/html/canvas/WebGLRenderingContext.h:304
> >  +          PassRefPtr<WebGLBuffer> findBuffer(Platform3DObject);
> > Why are these all PassRefPtrs? We're not transferring ownership here, right?
> 
> After further investigation, I think the return type actually should be PassRefPtr.  Because the only receiver of the returned data is WebGLGetInfo, and WebGLGetInfo constructor takes PassRefPtr as input.

That's correct. WebGLGetInfo attempts to be correct with respect to reference counts, even though they are transient objects. For this reason it takes PassRefPtrs as arguments to its constructors, and is why these find* methods need to return PassRefPtr.

[Kenneth Russell]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 58283 [details] [details] [details])
> > > 
> > > WebCore/html/canvas/WebGLRenderingContext.h:304
> > >  +          PassRefPtr<WebGLBuffer> findBuffer(Platform3DObject);
> > > Why are these all PassRefPtrs? We're not transferring ownership here, right?
> > 
> > After further investigation, I think the return type actually should be PassRefPtr.  Because the only receiver of the returned data is WebGLGetInfo, and WebGLGetInfo constructor takes PassRefPtr as input.
> 
> That's correct. WebGLGetInfo attempts to be correct with respect to reference counts, even though they are transient objects. For this reason it takes PassRefPtrs as arguments to its constructors, and is why these find* methods need to return PassRefPtr.

I've talked with Dimitri and he pointed out that by convention, if a function returns an object which is shared (rather than an object whose ownership is being transferred), it should return a raw pointer. In order to follow this convention we should change the find* routines to return raw pointers, but leave WebGLGetInfo's constructors still taking PassRefPtr. Sorry about the confusion on my part. Mo, can you make this change?

[Zhenyao Mo]

Created attachment 58814 [details]
revised patch: find*() returns raw pointer now

[Kenneth Russell]

Recalling our offline conversation about the raw pointers being automatically converted to bool, could you try making the WebGLGetInfo constructor taking bool explicit ("explicit WebGLGetInfo(bool value)")? If that works, then the constructors taking PassRefPtr<T> should be picked up automatically, eliminating the need to mention the PassRefPtr<T> constructor explicitly.

[Dimitri Glazkov (Google)]

Comment on attachment 58814 [details]
revised patch: find*() returns raw pointer now

> +            return WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(findRenderbuffer(static_cast<Platform3DObject>(value))));

Sorry that the pretty implicit conversion fell apart here.

[WebKit Commit Bot]

Comment on attachment 58814 [details]
revised patch: find*() returns raw pointer now

Clearing flags on attachment: 58814

Committed r61406: <http://trac.webkit.org/changeset/61406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.