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

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

GLRenderer::RestoreGLState() doesn't restore scissor box correctly when
the scissor was disabled. The following call sequence triggers this bug:

renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));
renderer->EnsureScissorTestDisabled();
ScopedUseGrContext::ScopedUseGrContext()
gl_->Scissor(1, 2, 3, 4);
ScopedUseGrContext::~ScopedUseGrContext()
renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));

The last call to SetScissorTestRect will enable scissor test but not
setting scissor rect because it thought it was already 800x600.

This CL fixes it by unconditionally restore scissor rect in RestoreGLState,
and scissor_rect_needs_reset_ is no longer needed.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/db79085ae904b4b9f9c24f1c5ae26f9de83ae750
Cr-Commit-Position: refs/heads/master@{#434121}

[trchen, 2016-11-19 06:50:53]

Description was changed from

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

GLRenderer::RestoreGLState() doesn't restore scissor box correctly when
the scissor was disabled. The following call sequence triggers this bug:

renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));
renderer->EnsureScissorTestDisabled();
ScopedUseGrContext::ScopedUseGrContext()
gl_->Scissor(1, 2, 3, 4);
ScopedUseGrContext::~ScopedUseGrContext()
renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));

The last call to SetScissorTestRect will enable scissor test but not
setting scissor rect because it thought it was already 800x600.

This CL fixes it by unconditionally restore scissor rect in RestoreGLState,
and scissor_rect_needs_reset_ is no longer needed.
==========

to

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

GLRenderer::RestoreGLState() doesn't restore scissor box correctly when
the scissor was disabled. The following call sequence triggers this bug:

renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));
renderer->EnsureScissorTestDisabled();
ScopedUseGrContext::ScopedUseGrContext()
gl_->Scissor(1, 2, 3, 4);
ScopedUseGrContext::~ScopedUseGrContext()
renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));

The last call to SetScissorTestRect will enable scissor test but not
setting scissor rect because it thought it was already 800x600.

This CL fixes it by unconditionally restore scissor rect in RestoreGLState,
and scissor_rect_needs_reset_ is no longer needed.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[trchen, 2016-11-19 06:51:01]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 06:51:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[trchen, 2016-11-19 06:51:13]

trchen@chromium.org changed reviewers:
+ enne@chromium.org

[commit-bot: I haz the power, 2016-11-19 07:06:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-19 07:06:35]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

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

Is there a BUG= or a failing test here?

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:3733: scissor_rect_ = gfx::Rect(0, 0, 0, 0);
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.

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:3770: gl_->Scissor(scissor_rect_.x(),
scissor_rect_.y(), scissor_rect_.width(),
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]

On 2016/11/21 00:47:44, enne wrote:
> Is there a BUG= or a failing test here?

Found this bug when I ran layout test with a SPv2 CL.
https://codereview.chromium.org/2495973002/
compositing/culling/filter-occlusion-blur.html

I feel the added DCHECK will be good enough for test coverage.

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:3733: scissor_rect_ = gfx::Rect(0, 0, 0, 0);
On 2016/11/21 00:47:44, enne wrote:
> 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.

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

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:3770: gl_->Scissor(scissor_rect_.x(),
scissor_rect_.y(), scissor_rect_.width(),
On 2016/11/21 00:47:44, enne wrote:
> I think a patch that just does this line of code this would be great without
the
> other changes.

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

[trchen, 2016-11-21 22:41:50]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 22:42:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Sure, lgtm

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2516063002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:3733: scissor_rect_ = gfx::Rect(0, 0, 0, 0);
On 2016/11/21 at 22:36:15, trchen wrote:
> On 2016/11/21 00:47:44, enne wrote:
> > 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.
> 
> Shouldn't it early out because the scissor rect is already reset to empty?

Ok sure, I guess ReinitializeGLState always calls RestoreGLState.

[commit-bot: I haz the power, 2016-11-21 23:20:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 23:20:58]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[trchen, 2016-11-22 00:28:44]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 00:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 01:16:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 01:16:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[trchen, 2016-11-22 04:46:41]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 04:47:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 05:32:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 05:32:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[trchen, 2016-11-22 21:14:39]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 21:15:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 21:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 21:43:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[trchen, 2016-11-23 00:21:21]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 00:22:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 02:32:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 02:32:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2016-11-23 03:16:33]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-11-23 03:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2516063002/#ps80001
(title: "fix tests for real (why do you want me to suffer, GetIntegerv?)")

[commit-bot: I haz the power, 2016-11-23 03:16:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 04:34:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 04:34:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2016-11-23 04:48:15]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-11-23 04:48:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 05:34:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479876495244150,
"parent_rev": "2178df1f70a8ec26170ef7bbebfd2c066dafdeda", "commit_rev":
"9250cda393aaa58e8701f6561398299b8b17c6e7"}

[commit-bot: I haz the power, 2016-11-23 05:35:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-23 05:39:12]

Description was changed from

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

GLRenderer::RestoreGLState() doesn't restore scissor box correctly when
the scissor was disabled. The following call sequence triggers this bug:

renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));
renderer->EnsureScissorTestDisabled();
ScopedUseGrContext::ScopedUseGrContext()
gl_->Scissor(1, 2, 3, 4);
ScopedUseGrContext::~ScopedUseGrContext()
renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));

The last call to SetScissorTestRect will enable scissor test but not
setting scissor rect because it thought it was already 800x600.

This CL fixes it by unconditionally restore scissor rect in RestoreGLState,
and scissor_rect_needs_reset_ is no longer needed.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

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

GLRenderer::RestoreGLState() doesn't restore scissor box correctly when
the scissor was disabled. The following call sequence triggers this bug:

renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));
renderer->EnsureScissorTestDisabled();
ScopedUseGrContext::ScopedUseGrContext()
gl_->Scissor(1, 2, 3, 4);
ScopedUseGrContext::~ScopedUseGrContext()
renderer->SetScissorTestRect(gfx::Rect(0, 0, 800, 600));

The last call to SetScissorTestRect will enable scissor test but not
setting scissor rect because it thought it was already 800x600.

This CL fixes it by unconditionally restore scissor rect in RestoreGLState,
and scissor_rect_needs_reset_ is no longer needed.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/db79085ae904b4b9f9c24f1c5ae26f9de83ae750
Cr-Commit-Position: refs/heads/master@{#434121}
==========

[commit-bot: I haz the power, 2016-11-23 05:39:13]

Patchset 5 (id:??) landed as
https://crrev.com/db79085ae904b4b9f9c24f1c5ae26f9de83ae750
Cr-Commit-Position: refs/heads/master@{#434121}