getFramebufferAttachmentParameter should return the original WebGLTexture/WebGLRenderbuffer instead of creating new ones sharing names.

[Zhenyao Mo]

Having multiple CanvasObject share the same resource name makes it very difficult to track the resources.

[Zhenyao Mo]

Created attachment 54716 [details]
patch

[Darin Adler]

Comment on attachment 54716 [details]
patch

Generally speaking member functions should be as private as possible. If you already have a WebGLFramebuffer* it's a programming error to call isFramebuffer, so it's better to make the WebGLFramebuffer::isFramebuffer private even though the function it is overriding in the base class, CanvasObject::isFramebuffer, is public.

[Zhenyao Mo]

Created attachment 54825 [details]
revised patch : responding to Darin Adler's review

[Kenneth Russell]

Comment on attachment 54825 [details]
revised patch : responding to Darin Adler's review

Looks good overall. It's good that you were able to reuse the m_canvasObjects table to reduce the risk of changing the memory management of the WebGLRenderingContext class. I think that we should reexamine the lifetime of the CanvasObjects / WebGLObjects, but that should wait for a subsequent bug.

One comment:
WebCore/html/canvas/WebGLRenderingContext.cpp:2844
 +              RefPtr<WebGLTexture> tex = reinterpret_cast<WebGLTexture*>((*it).get());
Can you try to write instead:

return reinterpret_cast<RefPtr<WebGLTexture> >(*it).release();

Perhaps you've already tried this and it doesn't work.


WebCore/html/canvas/WebGLRenderingContext.cpp:2856
 +              RefPtr<WebGLRenderbuffer> buffer = reinterpret_cast<WebGLRenderbuffer*>((*it).get());
If the above suggestion works, please use it here too.

[Zhenyao Mo]

(In reply to comment #4)
> (From update of attachment 54825 [details])
> Looks good overall. It's good that you were able to reuse the m_canvasObjects
> table to reduce the risk of changing the memory management of the
> WebGLRenderingContext class. I think that we should reexamine the lifetime of
> the CanvasObjects / WebGLObjects, but that should wait for a subsequent bug.
> 
> One comment:
> WebCore/html/canvas/WebGLRenderingContext.cpp:2844
>  +              RefPtr<WebGLTexture> tex =
> reinterpret_cast<WebGLTexture*>((*it).get());
> Can you try to write instead:
> 
> return reinterpret_cast<RefPtr<WebGLTexture> >(*it).release();
> 

I tried.  The cast is invalid.

> Perhaps you've already tried this and it doesn't work.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2856
>  +              RefPtr<WebGLRenderbuffer> buffer =
> reinterpret_cast<WebGLRenderbuffer*>((*it).get());
> If the above suggestion works, please use it here too.

[Kenneth Russell]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 54825 [details] [details])
> > Looks good overall. It's good that you were able to reuse the m_canvasObjects
> > table to reduce the risk of changing the memory management of the
> > WebGLRenderingContext class. I think that we should reexamine the lifetime of
> > the CanvasObjects / WebGLObjects, but that should wait for a subsequent bug.
> > 
> > One comment:
> > WebCore/html/canvas/WebGLRenderingContext.cpp:2844
> >  +              RefPtr<WebGLTexture> tex =
> > reinterpret_cast<WebGLTexture*>((*it).get());
> > Can you try to write instead:
> > 
> > return reinterpret_cast<RefPtr<WebGLTexture> >(*it).release();
> > 
> 
> I tried.  The cast is invalid.

OK. You can still write "return tex.release();" instead of explicitly constructing the PassRefPtr out of the RefPtr. In fact I think you should do this to reduce churn on the reference counts.

> 
> > Perhaps you've already tried this and it doesn't work.
> > 
> > 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:2856
> >  +              RefPtr<WebGLRenderbuffer> buffer =
> > reinterpret_cast<WebGLRenderbuffer*>((*it).get());
> > If the above suggestion works, please use it here too.

[Zhenyao Mo]

Created attachment 55025 [details]
revised patch: responding to Ken Russell's review

[Kenneth Russell]

Comment on attachment 55025 [details]
revised patch: responding to Ken Russell's review

Looks good. Thanks for cleaning this up.

[Dimitri Glazkov (Google)]

Comment on attachment 55025 [details]
revised patch: responding to Ken Russell's review

ok.

[WebKit Commit Bot]

Comment on attachment 55025 [details]
revised patch: responding to Ken Russell's review

Clearing flags on attachment: 55025

Committed r58790: <http://trac.webkit.org/changeset/58790>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.