The code paths in various WebGLRenderingContext methods duplicate the following structures frequently: // Commit DrawingBuffer if needed (e.g., for multisampling) if (!m_framebufferBinding && m_drawingBuffer) m_drawingBuffer->commit(); // Interesting rendering code here . . . . // Restore DrawingBuffer if needed if (!m_framebufferBinding && m_drawingBuffer) m_drawingBuffer->bind(); The code duplication should be eliminated via a scoped object. See comments in https://bugs.webkit.org/show_bug.cgi?id=53201#c56
Created attachment 115058 [details] Patch
Comment on attachment 115058 [details] Patch Attachment 115058 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10484102
Comment on attachment 115058 [details] Patch Attachment 115058 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10372339
Created attachment 115081 [details] Patch
Comment on attachment 115081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115081&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1236 > + ScopedDrawingBufferBinder(m_drawingBuffer, m_framebufferBinding); Yikes! There was a missing { } pair here that wasn't caught in my earlier submit. Fixing here.
Comment on attachment 115081 [details] Patch Nice cleanup! r=me
Comment on attachment 115081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115081&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:105 > + RefPtr<WebGLFramebuffer> m_framebufferBinding; Quick question: do we really need to hold a reference to those 2 objects here? It looks like WebGLRenderingContext already hold a strong reference to those 2 so it just adds some reference churn.
Comment on attachment 115081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115081&action=review >> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:105 >> + RefPtr<WebGLFramebuffer> m_framebufferBinding; > > Quick question: do we really need to hold a reference to those 2 objects here? It looks like WebGLRenderingContext already hold a strong reference to those 2 so it just adds some reference churn. No, there is no strict need for references to be held. Raw pointers would also suffice. Is there a preference for smart pointers versus raw? I chose smart pointers arbitrarily, because I have a slight aversion to storing raw pointers, except when necessary to prevent reference cycles or due to other implementation details.
Comment on attachment 115081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115081&action=review >>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:105 >>> + RefPtr<WebGLFramebuffer> m_framebufferBinding; >> >> Quick question: do we really need to hold a reference to those 2 objects here? It looks like WebGLRenderingContext already hold a strong reference to those 2 so it just adds some reference churn. > > No, there is no strict need for references to be held. Raw pointers would also suffice. > > Is there a preference for smart pointers versus raw? I chose smart pointers arbitrarily, because I have a slight aversion to storing raw pointers, except when necessary to prevent reference cycles or due to other implementation details. There is usually a preference towards using RefPtr only when you explicitly need to hold a reference to the object. It seems safe to switch that to raw pointers here so I would lean towards doing that (make sure you also change the constructor's arguments as PassRefPtr as the same effect).
Created attachment 115195 [details] Patch
Comment on attachment 115195 [details] Patch Forwarding Stephen's r+ as he knows the code better and I am fine with the new change. Thanks!
Comment on attachment 115195 [details] Patch Clearing flags on attachment: 115195 Committed r100308: <http://trac.webkit.org/changeset/100308>
All reviewed patches have been landed. Closing bug.
Created attachment 115301 [details] Patch
Comment on attachment 115301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115301&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1190 > + ScopedDrawingBufferBinder binder(m_drawingBuffer.get(), m_framebufferBinding.get()); The previous changes had failed to make a local of the ScopedDrawingBufferBinder instance. An anonymous instance was created, which was constructed and destroyed within the same sequence point, not at the scope block.
For the record, previous patch was rolled out in https://bugs.webkit.org/show_bug.cgi?id=72450 / http://trac.webkit.org/changeset/100394 due to conformance suite regressions.
Comment on attachment 115301 [details] Patch Nice cleanup and good catch. r=me
(In reply to comment #14) > Created an attachment (id=115301) [details] > Patch I can confirm that this change corrects all of the WebGL conformance regressions introduced by patch #3. This patch fixes chrome bug crbug.com/104407, which had been opened in response to these regressions.
Comment on attachment 115301 [details] Patch Sounds great. Marking cq+.
Comment on attachment 115301 [details] Patch Clearing flags on attachment: 115301 Committed r100399: <http://trac.webkit.org/changeset/100399>
Created attachment 116399 [details] Patch
(In reply to comment #22) > Created an attachment (id=116399) [details] > Patch Disregard this patch. This patch was not intended for this issue.