Always restore GL scissor box in GLRenderer::RestoreGLState()

cc/output/gl_renderer.cc

Index: cc/output/gl_renderer.cc
diff --git a/cc/output/gl_renderer.cc b/cc/output/gl_renderer.cc
index 6045e2c7684ce862275ca5b15b5d064c96eec363..0401ebbb821327fdc1d042b54786a59b1ba2f6e2 100644
--- a/cc/output/gl_renderer.cc
+++ b/cc/output/gl_renderer.cc
@@ -380,7 +380,6 @@ GLRenderer::GLRenderer(const RendererSettings* settings,
       context_support_(output_surface->context_provider()->ContextSupport()),
       texture_mailbox_deleter_(texture_mailbox_deleter),
       is_scissor_enabled_(false),
-      scissor_rect_needs_reset_(true),
       stencil_shadow_(false),
       blend_shadow_(false),
       highp_threshold_min_(highp_threshold_min),
@@ -3224,15 +3223,22 @@ void GLRenderer::SetScissorTestRect(const gfx::Rect& scissor_rect) {
 
   // Don't unnecessarily ask the context to change the scissor, because it
   // may cause undesired GPU pipeline flushes.
-  if (scissor_rect == scissor_rect_ && !scissor_rect_needs_reset_)
+  if (scissor_rect == scissor_rect_) {
+#if DCHECK_IS_ON()
+    GLint actual_scissor[4];
+    gl_->GetIntegerv(GL_SCISSOR_BOX, actual_scissor);
+    DCHECK(scissor_rect.x() == actual_scissor[0] &&
+           scissor_rect.y() == actual_scissor[1] &&
+           scissor_rect.width() == actual_scissor[2] &&
+           scissor_rect.height() == actual_scissor[3]);
+#endif
     return;
+  }
 
   scissor_rect_ = scissor_rect;
   FlushTextureQuadCache(SHARED_BINDING);
   gl_->Scissor(scissor_rect.x(), scissor_rect.y(), scissor_rect.width(),
                scissor_rect.height());
-
-  scissor_rect_needs_reset_ = false;
 }
 
 void GLRenderer::SetViewport() {
@@ -3724,7 +3730,7 @@ void GLRenderer::CleanupSharedObjects() {
 
 void GLRenderer::ReinitializeGLState() {
   is_scissor_enabled_ = false;
-  scissor_rect_needs_reset_ = true;
+  scissor_rect_ = gfx::Rect(0, 0, 0, 0);

[enne (OOO), 2016/11/21 00:47:44]

I think this is not correct.  If something sets the scissor to the empty rect,
then this will early out and not actually set the scissor.

[trchen, 2016/11/21 22:36:15]

Shouldn't it early out because the scissor rect is already reset to empty?

[enne (OOO), 2016/11/21 23:08:05]

Ok sure, I guess ReinitializeGLState always calls RestoreGLState.

   stencil_shadow_ = false;
   blend_shadow_ = true;
   program_shadow_ = 0;
@@ -3756,13 +3762,13 @@ void GLRenderer::RestoreGLState() {
   else
     gl_->Disable(GL_BLEND);
 
-  if (is_scissor_enabled_) {
+  if (is_scissor_enabled_)
     gl_->Enable(GL_SCISSOR_TEST);
-    gl_->Scissor(scissor_rect_.x(), scissor_rect_.y(), scissor_rect_.width(),
-                 scissor_rect_.height());
-  } else {
+  else
     gl_->Disable(GL_SCISSOR_TEST);
-  }
+
+  gl_->Scissor(scissor_rect_.x(), scissor_rect_.y(), scissor_rect_.width(),

[enne (OOO), 2016/11/21 00:47:44]

I think a patch that just does this line of code this would be great without the
other changes.

[trchen, 2016/11/21 22:36:15]

Sure it will still fix the bug. I don't see why the extra complexity is needed
though...

+               scissor_rect_.height());
 }
 
 bool GLRenderer::IsContextLost() {