Summary: | getFramebufferAttachmentParameter should return the original WebGLTexture/WebGLRenderbuffer instead of creating new ones sharing names. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, fishd, kbr, oliver | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-04-27 17:31:16 PDT
Created attachment 54716 [details]
patch
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.
Created attachment 54825 [details]
revised patch : responding to Darin Adler's review
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.
(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. (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. Created attachment 55025 [details]
revised patch: responding to Ken Russell's review
Comment on attachment 55025 [details]
revised patch: responding to Ken Russell's review
Looks good. Thanks for cleaning this up.
Comment on attachment 55025 [details]
revised patch: responding to Ken Russell's review
ok.
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> All reviewed patches have been landed. Closing bug. |