Summary: | WebGLRenderingContext: Remove duplicate DrawingBuffer binding code | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Timanus <twiz> | ||||||||||||
Component: | WebGL | Assignee: | Jeff Timanus <twiz> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cc-bugs, dglazkov, jamesr, jchaffraix, kbr, senorblanco, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 72450 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Jeff Timanus
2011-11-14 16:40:49 PST
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> All reviewed patches have been landed. Closing bug. 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. |