cc: Remove empty viewport check in GLRenderer::BeginDrawingFrame.

cc: Remove empty viewport check in GLRenderer::BeginDrawingFrame.

It's now up to the caller to avoid calls to
::Begin/FinishDrawingFrame when it makes sense to do so.

BUG=428071
R=danakj

Committed: https://crrev.com/8b8f3285ce31b886805b3ad0f428b96539bc045b
Cr-Commit-Position: refs/heads/master@{#302149}

[reveman, 2014-10-29 18:51:28]

Looks like we can just do this for now. We'll need to make sure to use a
non-empty viewport when fixing unit tests that are currently missing a
BeginDrawingFrame call but it's better to deal with that as part of fixing the
unit tests.

[danakj, 2014-10-29 18:59:37]

Cool can you point out where in LTHI we guarantee this? I went to find it
earlier but didn't see it.

[reveman, 2014-10-29 19:28:33]

On 2014/10/29 18:59:37, danakj wrote:
> Cool can you point out where in LTHI we guarantee this? I went to find it
> earlier but didn't see it.

Actually, now that I look more closely at LayerTreeHostImpl and
CalculateRenderPasses, it's probably not true. There's not one simple
conditional that guarantees this afaict. I could add another early out to
LayerTreeHostImpl::DrawLayers for this but a rather not unless actually
necessary. I'm worried that we have code that rely on the ::DrawLayers code
executing when the viewport is empty and if I add an early out I might silently
break this.

I'm tempted to land this and just see if we hit that DCHECK. Wdyt?

[danakj, 2014-10-29 19:33:26]

On Wed, Oct 29, 2014 at 3:28 PM, <reveman@chromium.org> wrote:

> On 2014/10/29 18:59:37, danakj wrote:
>
>> Cool can you point out where in LTHI we guarantee this? I went to find it
>> earlier but didn't see it.
>>
>
> Actually, now that I look more closely at LayerTreeHostImpl and
> CalculateRenderPasses, it's probably not true. There's not one simple
> conditional that guarantees this afaict. I could add another early out to
> LayerTreeHostImpl::DrawLayers for this but a rather not unless actually
> necessary. I'm worried that we have code that rely on the ::DrawLayers code
> executing when the viewport is empty and if I add an early out I might
> silently
> break this.
>
> I'm tempted to land this and just see if we hit that DCHECK. Wdyt?
>

I'm not super excited about speculative DCHECKs. CanDraw() does not return
false when the viewport is empty, so it seems like we should be able to
draw/swap a frame to the browser in that state.

If you want to DCHECK this then we should also change CanDraw and add tests
around that, I think. But I think we're approaching diminishing returns
here also, unless you see otherwise?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-10-29 20:09:33]

On 2014/10/29 19:33:26, danakj wrote:
> On Wed, Oct 29, 2014 at 3:28 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2014/10/29 18:59:37, danakj wrote:
> >
> >> Cool can you point out where in LTHI we guarantee this? I went to find it
> >> earlier but didn't see it.
> >>
> >
> > Actually, now that I look more closely at LayerTreeHostImpl and
> > CalculateRenderPasses, it's probably not true. There's not one simple
> > conditional that guarantees this afaict. I could add another early out to
> > LayerTreeHostImpl::DrawLayers for this but a rather not unless actually
> > necessary. I'm worried that we have code that rely on the ::DrawLayers code
> > executing when the viewport is empty and if I add an early out I might
> > silently
> > break this.
> >
> > I'm tempted to land this and just see if we hit that DCHECK. Wdyt?
> >
> 
> I'm not super excited about speculative DCHECKs. CanDraw() does not return
> false when the viewport is empty, so it seems like we should be able to
> draw/swap a frame to the browser in that state.

I agree. But I'm even less excited about a speculative conditional. So unless we
can come up with some good reasoning for why a check is needed, how about we
just remove this frame->device_viewport_rect.IsEmpty() completely? Afaict,
calling GLRenderer::BeginDrawingFrame with an empty viewport should not cause
any harm except possibly a minor performance regression if it's common practice
and that doesn't seem to be the case as all tests pass with the DCHECK and I
didn't manage to hit it using some manual testing. I've updated the patch to do
this. Wdyt?

> 
> If you want to DCHECK this then we should also change CanDraw and add tests
> around that, I think. But I think we're approaching diminishing returns
> here also, unless you see otherwise?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-30 15:18:32]

Sure LGTM

[reveman, 2014-10-30 17:22:37]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:24:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678113004/20001

[commit-bot: I haz the power, 2014-10-30 19:35:17]

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

[commit-bot: I haz the power, 2014-10-30 19:35:19]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[reveman, 2014-10-30 19:38:22]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-30 19:39:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678113004/20001

[commit-bot: I haz the power, 2014-10-30 20:34:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-30 20:34:51]

Patchset 2 (id:??) landed as
https://crrev.com/8b8f3285ce31b886805b3ad0f428b96539bc045b
Cr-Commit-Position: refs/heads/master@{#302149}