Bug 40176 - Fix WebGLRenderingContext helper functions find{Texture/Renderbuffer/Buffer}
Summary: Fix WebGLRenderingContext helper functions find{Texture/Renderbuffer/Buffer}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-04 11:37 PDT by Zhenyao Mo
Modified: 2010-06-18 08:14 PDT (History)
5 users (show)

See Also:


Attachments
patch (10.12 KB, patch)
2010-06-09 13:00 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: find*() returns raw pointer now (11.58 KB, patch)
2010-06-15 13:52 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2010-06-04 11:37:06 PDT
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.
Comment 1 Zhenyao Mo 2010-06-09 13:00:31 PDT
Created attachment 58283 [details]
patch
Comment 2 Kenneth Russell 2010-06-10 15:21:03 PDT
Comment on attachment 58283 [details]
patch

Looks good.
Comment 3 Dimitri Glazkov (Google) 2010-06-10 15:51:01 PDT
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?
Comment 4 Zhenyao Mo 2010-06-10 16:48:39 PDT
(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.
Comment 5 Kenneth Russell 2010-06-10 16:57:20 PDT
(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.
Comment 6 Kenneth Russell 2010-06-15 10:13:21 PDT
(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?
Comment 7 Zhenyao Mo 2010-06-15 13:52:40 PDT
Created attachment 58814 [details]
revised patch: find*() returns raw pointer now
Comment 8 Kenneth Russell 2010-06-15 14:01:34 PDT
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 9 Dimitri Glazkov (Google) 2010-06-15 14:20:40 PDT
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 10 WebKit Commit Bot 2010-06-18 08:14:20 PDT
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>
Comment 11 WebKit Commit Bot 2010-06-18 08:14:25 PDT
All reviewed patches have been landed.  Closing bug.