Summary: | Fix WebGLRenderingContext helper functions find{Texture/Renderbuffer/Buffer} | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, kbr, oliver | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-06-04 11:37:06 PDT
Created attachment 58283 [details]
patch
Comment on attachment 58283 [details]
patch
Looks good.
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?
(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. (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. (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? Created attachment 58814 [details]
revised patch: find*() returns raw pointer now
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. 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. 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> All reviewed patches have been landed. Closing bug. |