Bug 72327 – WebGLRenderingContext: Remove duplicate DrawingBuffer binding code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Jeff Timanus]

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

[Jeff Timanus]

Created attachment 115058 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115058 [details]
Patch

Attachment 115058 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10484102

[Early Warning System Bot]

Comment on attachment 115058 [details]
Patch

Attachment 115058 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10372339

[Jeff Timanus]

Created attachment 115081 [details]
Patch

[Jeff Timanus]

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.

[Stephen White]

Comment on attachment 115081 [details]
Patch

Nice cleanup!  r=me

[Julien Chaffraix]

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.

[Jeff Timanus]

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.

[Julien Chaffraix]

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).

[Jeff Timanus]

Created attachment 115195 [details]
Patch

[Julien Chaffraix]

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!

[WebKit Review Bot]

Comment on attachment 115195 [details]
Patch

Clearing flags on attachment: 115195

Committed r100308: <http://trac.webkit.org/changeset/100308>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jeff Timanus]

Created attachment 115301 [details]
Patch

[Jeff Timanus]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

Comment on attachment 115301 [details]
Patch

Nice cleanup and good catch. r=me

[Jeff Timanus]

(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.

[Kenneth Russell]

Comment on attachment 115301 [details]
Patch

Sounds great. Marking cq+.

[WebKit Review Bot]

Comment on attachment 115301 [details]
Patch

Clearing flags on attachment: 115301

Committed r100399: <http://trac.webkit.org/changeset/100399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jeff Timanus]

Created attachment 116399 [details]
Patch

[Jeff Timanus]

(In reply to comment #22)
> Created an attachment (id=116399) [details]
> Patch

Disregard this patch.  This patch was not intended for this issue.