Re-land: cc: Make Overlay unit tests work when sync query extension is available.

Re-land: cc: Make Overlay unit tests work when sync query extension is available.

GLRenderer expects a BeginDrawingFrame before each
FinishDrawingFrame call.

This also makes sure GLRenderer::BeginDrawingFrame doesn't
early out before creating the read lock fence.

BUG=428071
R=danakj
TEST=cc_unittests --gtest_filter=GLRendererWithOverlaysTest.ResourcesExportedAndReturned

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

[reveman, 2014-10-29 01:44:08]

reveman@chromium.org changed reviewers:
+ alexst@chromium.org

[reveman, 2014-10-29 01:44:09]

+alexst who wrote the test

[reveman, 2014-10-29 01:45:03]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-29 01:46:50]

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

[commit-bot: I haz the power, 2014-10-29 02:32:05]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-29 02:32:51]

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

[danakj, 2014-10-29 15:18:27]

Please don't TBR stuff like this

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

https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:499: if (frame->device_viewport_rect.IsEmpty())
If there's an empty viewport can you not just make FinishDrawingFrame early out
and not do a bunch of work here?

https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.c...
cc/output/overlay_unittest.cc:540: virtual void BeginDrawingFrame(DrawingFrame*
frame) override {
using GLRenderer::BeginDrawingFrame instead.

[reveman, 2014-10-29 15:34:28]

On 2014/10/29 15:18:27, danakj wrote:
> Please don't TBR stuff like this

Sorry, I'll revert this and re-land after addressing your feedback.

> 
> https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc#new...
> cc/output/gl_renderer.cc:499: if (frame->device_viewport_rect.IsEmpty())
> If there's an empty viewport can you not just make FinishDrawingFrame early
out
> and not do a bunch of work here?
> 
> https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.cc
> File cc/output/overlay_unittest.cc (right):
> 
>
https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.c...
> cc/output/overlay_unittest.cc:540: virtual void
BeginDrawingFrame(DrawingFrame*
> frame) override {
> using GLRenderer::BeginDrawingFrame instead.

[reveman, 2014-10-29 15:35:58]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/684133005/ by reveman@chromium.org.

The reason for reverting is: Not properly reviewed..

[reveman, 2014-10-29 16:14:52]

PTAL

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

https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:499: if (frame->device_viewport_rect.IsEmpty())
On 2014/10/29 15:18:26, danakj wrote:
> If there's an empty viewport can you not just make FinishDrawingFrame early
out
> and not do a bunch of work here?

Maybe. It would assume that we're not performing any operations that require a
read lock fence in these cases. It's not clear from this code if that is safe.

Alternatively, we could just remove this check and leave it up to the caller.

https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.cc
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/669233003/diff/1/cc/output/overlay_unittest.c...
cc/output/overlay_unittest.cc:540: virtual void BeginDrawingFrame(DrawingFrame*
frame) override {
On 2014/10/29 15:18:26, danakj wrote:
> using GLRenderer::BeginDrawingFrame instead.

Sure.

[danakj, 2014-10-29 17:04:38]

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

https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:499: if (frame->device_viewport_rect.IsEmpty())
On 2014/10/29 16:14:52, reveman wrote:
> On 2014/10/29 15:18:26, danakj wrote:
> > If there's an empty viewport can you not just make FinishDrawingFrame early
> out
> > and not do a bunch of work here?
> 
> Maybe. It would assume that we're not performing any operations that require a
> read lock fence in these cases. It's not clear from this code if that is safe.
> 
> Alternatively, we could just remove this check and leave it up to the caller.

ie make DirectRenderer check this? That sounds good.

[reveman, 2014-10-29 17:09:49]

On 2014/10/29 17:04:38, danakj wrote:
> https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/669233003/diff/1/cc/output/gl_renderer.cc#new...
> cc/output/gl_renderer.cc:499: if (frame->device_viewport_rect.IsEmpty())
> On 2014/10/29 16:14:52, reveman wrote:
> > On 2014/10/29 15:18:26, danakj wrote:
> > > If there's an empty viewport can you not just make FinishDrawingFrame
early
> > out
> > > and not do a bunch of work here?
> > 
> > Maybe. It would assume that we're not performing any operations that require
a
> > read lock fence in these cases. It's not clear from this code if that is
safe.
> > 
> > Alternatively, we could just remove this check and leave it up to the
caller.
> 
> ie make DirectRenderer check this? That sounds good.

Ok, I'll do that in a separate patch and remove it from this patch.

[reveman, 2014-10-31 23:13:04]

PTAL

[alexst (slow to review), 2014-11-03 15:43:30]

lgtm

[danakj, 2014-11-03 15:47:33]

LGTM

[reveman, 2014-11-05 00:35:24]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:36:30]

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

[commit-bot: I haz the power, 2014-11-05 01:24:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-05 01:25:05]

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