DrawingBuffer: Clean up GL state restoration

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
index 2844f45682b8ab3a4195f816f74bd025998e89e4..32c0cf7a69680591bda05c46bee10ad6a95e0292 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -714,7 +714,7 @@ PassRefPtr<Image> WebGLRenderingContextBase::getImage(
   if (!drawingBuffer())
     return nullptr;
 
-  drawingBuffer()->commit();
+  drawingBuffer()->resolveAndBindForReadAndDraw();
   IntSize size = clampedCanvasSize();
   OpacityMode opacityMode =
       creationAttributes().hasAlpha() ? NonOpaque : Opaque;
@@ -1053,7 +1053,7 @@ PassRefPtr<DrawingBuffer> WebGLRenderingContextBase::createDrawingBuffer(
     NOTREACHED();
   }
   return DrawingBuffer::create(
-      std::move(contextProvider), clampedCanvasSize(), premultipliedAlpha,
+      std::move(contextProvider), this, clampedCanvasSize(), premultipliedAlpha,
       wantAlphaChannel, wantDepthBuffer, wantStencilBuffer, wantAntialiasing,
       preserve, webGLVersion, chromiumImageUsage);
 }
@@ -1084,12 +1084,10 @@ void WebGLRenderingContextBase::initializeNewContext() {
   m_numGLErrorsToConsoleAllowed = maxGLErrorsAllowedToConsole;
 
   m_clearColor[0] = m_clearColor[1] = m_clearColor[2] = m_clearColor[3] = 0;
-  drawingBuffer()->setClearColor(m_clearColor);
   m_scissorEnabled = false;
   m_clearDepth = 1;
   m_clearStencil = 0;
   m_colorMask[0] = m_colorMask[1] = m_colorMask[2] = m_colorMask[3] = true;
-  drawingBuffer()->setColorMask(m_colorMask);
 
   GLint numCombinedTextureImageUnits = 0;
   contextGL()->GetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS,
@@ -1375,9 +1373,6 @@ WebGLRenderingContextBase::clearIfComposited(GLbitfield mask) {
       true, true, true,
       !drawingBuffer()->defaultBufferRequiresAlphaChannelToBePreserved());
   drawingBuffer()->clearFramebuffers(clearMask);
-
-  restoreStateAfterClear();
-  drawingBuffer()->restoreFramebufferBindings();
   drawingBuffer()->setBufferClearNeeded(false);
 
   return combinedClear ? CombinedClear : JustClear;
@@ -1410,27 +1405,6 @@ void WebGLRenderingContextBase::restoreClearColor() {
                           m_clearColor[3]);
 }
 
-void WebGLRenderingContextBase::restoreClearDepthf() {
-  if (isContextLost())
-    return;
-
-  contextGL()->ClearDepthf(m_clearDepth);
-}
-
-void WebGLRenderingContextBase::restoreClearStencil() {
-  if (isContextLost())
-    return;
-
-  contextGL()->ClearStencil(m_clearStencil);
-}
-
-void WebGLRenderingContextBase::restoreStencilMaskSeparate() {
-  if (isContextLost())
-    return;
-
-  contextGL()->StencilMaskSeparate(GL_FRONT, m_stencilMask);
-}
-
 void WebGLRenderingContextBase::restoreColorMask() {
   if (isContextLost())
     return;
@@ -1439,24 +1413,6 @@ void WebGLRenderingContextBase::restoreColorMask() {
                          m_colorMask[3]);
 }
 
-void WebGLRenderingContextBase::restoreDepthMask() {
-  if (isContextLost())
-    return;
-
-  contextGL()->DepthMask(m_depthMask);
-}
-
-void WebGLRenderingContextBase::restoreStateAfterClear() {
-  // Restore clear-related state items back to what the context had set.
-  restoreScissorEnabled();
-  restoreClearColor();
-  restoreColorMask();
-  restoreClearDepthf();
-  restoreClearStencil();
-  restoreStencilMaskSeparate();
-  restoreDepthMask();
-}
-
 void WebGLRenderingContextBase::markLayerComposited() {
   if (!isContextLost())
     drawingBuffer()->setBufferClearNeeded(true);
@@ -1492,7 +1448,7 @@ bool WebGLRenderingContextBase::paintRenderingResultsToCanvas(
   ScopedTexture2DRestorer restorer(this);
   ScopedFramebufferRestorer fboRestorer(this);
 
-  drawingBuffer()->commit();
+  drawingBuffer()->resolveAndBindForReadAndDraw();
   if (!canvas()->buffer()->copyRenderingResultsFromDrawingBuffer(
           drawingBuffer(), sourceBuffer)) {
     // Currently, copyRenderingResultsFromDrawingBuffer is expected to always
@@ -1514,7 +1470,7 @@ ImageData* WebGLRenderingContextBase::paintRenderingResultsToImageData(
     return nullptr;
 
   clearIfComposited();
-  drawingBuffer()->commit();
+  drawingBuffer()->resolveAndBindForReadAndDraw();
   ScopedFramebufferRestorer restorer(this);
   int width, height;
   WTF::ArrayBufferContents contents;
@@ -1562,17 +1518,7 @@ void WebGLRenderingContextBase::reshape(int width, int height) {
 
   // We don't have to mark the canvas as dirty, since the newly created image
   // buffer will also start off clear (and this matches what reshape will do).
-  drawingBuffer()->reset(IntSize(width, height));

[ccameron, 2016/10/09 21:10:53]

This callsite has the most sin of all of them...

To start, why is this called "reset" ... what exactly are we re-setting? Was
there something wrong with calling it "resize" or "reshape"??

-  restoreStateAfterClear();
-
-  contextGL()->BindTexture(

[ccameron, 2016/10/09 21:10:53]

Note that we already have the helper function
WebGLRenderingContextBase::restoreCurrentTexture2D to do this!

AND we have the DrawingBuffer helper function
DrawingBuffer::restoreTextureBindings to do this, too!

And yet we call neither of them, and re-roll it here!!

-      GL_TEXTURE_2D,
-      objectOrZero(
-          m_textureUnits[m_activeTextureUnit].m_texture2DBinding.get()));
-  contextGL()->BindRenderbuffer(GL_RENDERBUFFER,

[ccameron, 2016/10/09 21:10:54]

This line is also precious, because we track the current renderbuffer binding in
DrawingBuffer (as DrawingBuffer::m_renderbufferBinding), but we never have any
methods that use that variable! We're tracking the state for fun in
DrawingBuffer, and restoring it explicitly  here!

-                                objectOrZero(m_renderbufferBinding.get()));
-  drawingBuffer()->restoreFramebufferBindings();

[ccameron, 2016/10/09 21:10:53]

We already have a method WebGLRenderingContextBase::restoreCurrentFramebuffer to
do EXACTLY this... why are we implementing it in DrawingBuffer and then calling
that????

-  drawingBuffer()->restorePixelUnpackBufferBindings();
+  drawingBuffer()->resize(IntSize(width, height));
 }
 
 int WebGLRenderingContextBase::drawingBufferWidth() const {
@@ -1593,8 +1539,6 @@ void WebGLRenderingContextBase::activeTexture(GLenum texture) {
   }
   m_activeTextureUnit = texture - GL_TEXTURE0;
   contextGL()->ActiveTexture(texture);
-
-  drawingBuffer()->setActiveTextureUnit(texture);
 }
 
 void WebGLRenderingContextBase::attachShader(WebGLProgram* program,
@@ -1686,10 +1630,6 @@ void WebGLRenderingContextBase::bindBuffer(GLenum target, WebGLBuffer* buffer) {
   }
   if (!validateAndUpdateBufferBindTarget("bindBuffer", target, buffer))
     return;
-
-  if (target == GL_PIXEL_UNPACK_BUFFER) {
-    drawingBuffer()->setPixelUnpackBufferBinding(objectOrZero(buffer));
-  }
   contextGL()->BindBuffer(target, objectOrZero(buffer));
 }
 
@@ -1729,9 +1669,6 @@ void WebGLRenderingContextBase::bindRenderbuffer(
   }
   m_renderbufferBinding = renderBuffer;
   contextGL()->BindRenderbuffer(target, objectOrZero(renderBuffer));
-
-  drawingBuffer()->setRenderbufferBinding(objectOrZero(renderBuffer));
-
   if (renderBuffer)
     renderBuffer->setHasEverBeenBound();
 }
@@ -1754,9 +1691,6 @@ void WebGLRenderingContextBase::bindTexture(GLenum target,
 
   if (target == GL_TEXTURE_2D) {
     m_textureUnits[m_activeTextureUnit].m_texture2DBinding = texture;
-
-    if (!m_activeTextureUnit)
-      drawingBuffer()->setTexture2DBinding(objectOrZero(texture));
   } else if (target == GL_TEXTURE_CUBE_MAP) {
     m_textureUnits[m_activeTextureUnit].m_textureCubeMapBinding = texture;
   } else if (isWebGL2OrHigher() && target == GL_TEXTURE_2D_ARRAY) {
@@ -2018,7 +1952,6 @@ void WebGLRenderingContextBase::clearColor(GLfloat r,
   m_clearColor[1] = g;
   m_clearColor[2] = b;
   m_clearColor[3] = a;
-  drawingBuffer()->setClearColor(m_clearColor);
   contextGL()->ClearColor(r, g, b, a);
 }
 
@@ -2046,7 +1979,6 @@ void WebGLRenderingContextBase::colorMask(GLboolean red,
   m_colorMask[1] = green;
   m_colorMask[2] = blue;
   m_colorMask[3] = alpha;
-  drawingBuffer()->setColorMask(m_colorMask);
   contextGL()->ColorMask(red, green, blue, alpha);
 }
 
@@ -2257,10 +2189,8 @@ bool WebGLRenderingContextBase::deleteObject(WebGLObject* object) {
 }
 
 void WebGLRenderingContextBase::deleteBuffer(WebGLBuffer* buffer) {
-  GLuint bufferName = objectOrZero(buffer);
   if (!deleteObject(buffer))
     return;
-  drawingBuffer()->notifyBufferDeleted(bufferName);
   removeBoundBuffer(buffer);
 }
 
@@ -2270,7 +2200,6 @@ void WebGLRenderingContextBase::deleteFramebuffer(
     return;
   if (framebuffer == m_framebufferBinding) {
     m_framebufferBinding = nullptr;
-    drawingBuffer()->setFramebufferBinding(GL_FRAMEBUFFER, 0);
     // Have to call drawingBuffer()->bind() here to bind back to internal fbo.
     drawingBuffer()->bind(GL_FRAMEBUFFER);
   }
@@ -2288,7 +2217,6 @@ void WebGLRenderingContextBase::deleteRenderbuffer(
     return;
   if (renderbuffer == m_renderbufferBinding) {
     m_renderbufferBinding = nullptr;
-    drawingBuffer()->setRenderbufferBinding(0);
   }
   if (m_framebufferBinding)
     m_framebufferBinding->removeAttachmentFromBoundFramebuffer(GL_FRAMEBUFFER,
@@ -2312,8 +2240,6 @@ void WebGLRenderingContextBase::deleteTexture(WebGLTexture* texture) {
     if (texture == m_textureUnits[i].m_texture2DBinding) {
       m_textureUnits[i].m_texture2DBinding = nullptr;
       maxBoundTextureIndex = i;
-      if (!i)
-        drawingBuffer()->setTexture2DBinding(0);
     }
     if (texture == m_textureUnits[i].m_textureCubeMapBinding) {
       m_textureUnits[i].m_textureCubeMapBinding = nullptr;
@@ -2391,10 +2317,8 @@ void WebGLRenderingContextBase::disable(GLenum cap) {
     applyStencilTest();
     return;
   }
-  if (cap == GL_SCISSOR_TEST) {
+  if (cap == GL_SCISSOR_TEST)
     m_scissorEnabled = false;
-    drawingBuffer()->setScissorEnabled(m_scissorEnabled);
-  }
   contextGL()->Disable(cap);
 }
 
@@ -2530,10 +2454,8 @@ void WebGLRenderingContextBase::enable(GLenum cap) {
     applyStencilTest();
     return;
   }
-  if (cap == GL_SCISSOR_TEST) {
+  if (cap == GL_SCISSOR_TEST)
     m_scissorEnabled = true;
-    drawingBuffer()->setScissorEnabled(m_scissorEnabled);
-  }
   contextGL()->Enable(cap);
 }
 
@@ -3934,7 +3856,6 @@ void WebGLRenderingContextBase::pixelStorei(GLenum pname, GLint param) {
       if (param == 1 || param == 2 || param == 4 || param == 8) {
         if (pname == GL_PACK_ALIGNMENT) {
           m_packAlignment = param;
-          drawingBuffer()->setPackAlignment(param);
         } else {  // GL_UNPACK_ALIGNMENT:
           m_unpackAlignment = param;
         }
@@ -5953,12 +5874,6 @@ void WebGLRenderingContextBase::loseContextImpl(
   ASSERT(m_contextLostMode != NotLostContext);
   m_autoRecoveryMethod = autoRecoveryMethod;
 
-  // Make absolutely sure we do not refer to an already-deleted texture or
-  // framebuffer.
-  drawingBuffer()->setTexture2DBinding(0);
-  drawingBuffer()->setFramebufferBinding(GL_FRAMEBUFFER, 0);
-  drawingBuffer()->setRenderbufferBinding(0);
-
   detachAndRemoveAllObjects();
 
   // Lose all the extensions.
@@ -6115,6 +6030,56 @@ void WebGLRenderingContextBase::stop() {
   }
 }
 
+bool WebGLRenderingContextBase::DrawingBufferClientIsBoundForDraw() {
+  return !m_framebufferBinding;
+}
+
+void WebGLRenderingContextBase::DrawingBufferClientRestoreScissorTest() {
+  if (m_scissorEnabled)
+    contextGL()->Enable(GL_SCISSOR_TEST);
+  else
+    contextGL()->Disable(GL_SCISSOR_TEST);
+}
+
+void WebGLRenderingContextBase::DrawingBufferClientRestoreMaskAndClearValues() {
+  contextGL()->ColorMask(m_colorMask[0], m_colorMask[1], m_colorMask[2],
+                         m_colorMask[3]);
+  contextGL()->DepthMask(m_depthMask);
+  contextGL()->StencilMaskSeparate(GL_FRONT, m_stencilMask);
+
+  contextGL()->ClearColor(m_clearColor[0], m_clearColor[1], m_clearColor[2],
+                          m_clearColor[3]);
+  contextGL()->ClearDepthf(m_clearDepth);
+  contextGL()->ClearStencil(m_clearStencil);
+}
+
+void WebGLRenderingContextBase::DrawingBufferClientRestorePixelPackAlignment() {
+  contextGL()->PixelStorei(GL_PACK_ALIGNMENT, m_packAlignment);
+}
+
+void WebGLRenderingContextBase::DrawingBufferClientRestoreTexture2DBinding() {
+  if (m_activeTextureUnit < m_textureUnits.size()) {
+    contextGL()->BindTexture(
+        GL_TEXTURE_2D,
+        objectOrZero(
+            m_textureUnits[m_activeTextureUnit].m_texture2DBinding.get()));
+  }
+}
+
+void WebGLRenderingContextBase::
+    DrawingBufferClientRestoreRenderbufferBinding() {
+  contextGL()->BindRenderbuffer(GL_RENDERBUFFER,
+                                objectOrZero(m_renderbufferBinding.get()));
+}
+
+void WebGLRenderingContextBase::DrawingBufferClientRestoreFramebufferBinding() {
+  contextGL()->BindFramebuffer(GL_FRAMEBUFFER,
+                               objectOrZero(m_framebufferBinding.get()));
+}
+
+void WebGLRenderingContextBase::
+    DrawingBufferClientRestorePixelUnpackBufferBinding() {}
+
 ScriptValue WebGLRenderingContextBase::getBooleanParameter(
     ScriptState* scriptState,
     GLenum pname) {
@@ -7408,9 +7373,6 @@ void WebGLRenderingContextBase::setFramebuffer(GLenum target,
     m_framebufferBinding = buffer;
     applyStencilTest();
   }
-  drawingBuffer()->setFramebufferBinding(
-      target, objectOrZero(getFramebufferBinding(target)));
-
   if (!buffer) {
     // Instead of binding fb 0, bind the drawing buffer.
     drawingBuffer()->bind(target);