cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

This moves calls from the proxies that happen between
LayerTreeHostImpl's DrawLayers and SwapBuffers to happen after
SwapBuffers, and moves all code at the bottom of DrawLayers, which
is after DelegatingRenderer::DrawFrame to the bottom of SwapBuffers,
which makes it after DelegatingRenderer::SwapBuffers.

R=enne
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/097919e7138f21a14a577688752ea955c47b16fa
Cr-Commit-Position: refs/heads/master@{#417034}

[danakj, 2016-09-07 00:22:08]

Description was changed from

==========
cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

This moves calls from the proxies that happen between
LayerTreeHostImpl's DrawLayers and SwapBuffers to happen after
SwapBuffers, and moves all code at the bottom of DrawLayers, which
is after DelegatingRenderer::DrawFrame to the bottom of SwapBuffers,
which makes it after DelegatingRenderer::SwapBuffers.

R=enne
BUG=606056
==========

to

==========
cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

This moves calls from the proxies that happen between
LayerTreeHostImpl's DrawLayers and SwapBuffers to happen after
SwapBuffers, and moves all code at the bottom of DrawLayers, which
is after DelegatingRenderer::DrawFrame to the bottom of SwapBuffers,
which makes it after DelegatingRenderer::SwapBuffers.

R=enne
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-09-07 00:22:49]

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

[commit-bot: I haz the power, 2016-09-07 00:23:13]

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

[danakj, 2016-09-07 00:25:35]

https://codereview.chromium.org/2317493003/diff/1/cc/output/delegating_render...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2317493003/diff/1/cc/output/delegating_render...
cc/output/delegating_renderer_unittest.cc:27: DrawResult
PrepareToDrawOnThread(LayerTreeHostImpl* host_impl,
Because DrawLayersOnThread now happens after
DisplayReceivedCompositorFrameOnThread

https://codereview.chromium.org/2317493003/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/1/cc/test/layer_tree_test.cc#...
cc/test/layer_tree_test.cc:186: bool SwapBuffers(const FrameData& frame)
override {
Eventually LTHI::SwapBuffers will become DrawLayers again. Or maybe it'll all be
PrepareToDraw we'll see.

https://codereview.chromium.org/2317493003/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2317493003/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1891: // The next frame should start by
assuming nothing has changed, and changes
I think this stuff shouldn't actually happen if has_no_damage, it didn't in
DrawLayers, will fix.

[danakj, 2016-09-07 00:32:15]

https://codereview.chromium.org/2317493003/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2317493003/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1891: // The next frame should start by
assuming nothing has changed, and changes
On 2016/09/07 00:25:35, danakj wrote:
> I think this stuff shouldn't actually happen if has_no_damage, it didn't in
> DrawLayers, will fix.

Fixed.

[danakj, 2016-09-07 00:32:16]

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

[commit-bot: I haz the power, 2016-09-07 00:32:44]

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

[danakj, 2016-09-07 00:44:43]

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

[commit-bot: I haz the power, 2016-09-07 00:45:12]

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

[danakj, 2016-09-07 00:48:06]

danakj@chromium.org changed reviewers:
+ ajuma@chromium.org

[danakj, 2016-09-07 00:48:07]

+ajuma can you have a look too? The UpdateAnimationState() is happening at a
slightly different time but seems ok to me?

[commit-bot: I haz the power, 2016-09-07 01:55:52]

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

[commit-bot: I haz the power, 2016-09-07 01:55:53]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-09-07 15:00:27]

On 2016/09/07 00:48:07, danakj wrote:
> +ajuma can you have a look too? The UpdateAnimationState() is happening at a
> slightly different time but seems ok to me?

Yeah, the change in timing of UpdateAnimationState lgtm.

[enne (OOO), 2016-09-07 18:05:52]

In general, this seems like a reasonable staging step on the way to where you're
going.  I think I want to see more cleanup in this area (which I know you're
planning on), but I think this is enough of a functional change to land
independently.

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
cc/output/delegating_renderer_unittest.cc:32: return draw_result;
Previously this wouldn't have been called if the DrawResult was that it wasn't
going to draw? Maybe you should DCHECK draw result?

Or maybe drawn_viewport_ means something else here other than actually drawn?

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
I know swap is going to go away, but why does SwapBuffers happen before
DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to preserve the
old ordering?

[danakj, 2016-09-07 18:28:12]

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
On 2016/09/07 18:05:52, enne wrote:
> I know swap is going to go away, but why does SwapBuffers happen before
> DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to preserve the
> old ordering?

What's happening is the frame is being given to the output surface inside
"DrawAndSwapCombined" step. So when we tell the test LTHI has done this step, it
has already also been told about the CompositorFrame handed to the output
surface. I double checked and no tests except those I touched in this CL were
both using DrawLayersOnThread and DisplayReceivedCompositorFrame.

[enne (OOO), 2016-09-07 18:38:08]

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
On 2016/09/07 at 18:28:12, danakj wrote:
> On 2016/09/07 18:05:52, enne wrote:
> > I know swap is going to go away, but why does SwapBuffers happen before
> > DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to preserve
the
> > old ordering?
> 
> What's happening is the frame is being given to the output surface inside
"DrawAndSwapCombined" step. So when we tell the test LTHI has done this step, it
has already also been told about the CompositorFrame handed to the output
surface. I double checked and no tests except those I touched in this CL were
both using DrawLayersOnThread and DisplayReceivedCompositorFrame.

I figured that this was safe to do in the current state of tests given that
things were green.  But, why?  Why reorder them if you don't have to?  Or do you
have to and I misunderstand?

[danakj, 2016-09-07 18:42:16]

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
cc/output/delegating_renderer_unittest.cc:32: return draw_result;
On 2016/09/07 18:05:51, enne wrote:
> Previously this wouldn't have been called if the DrawResult was that it wasn't
> going to draw? Maybe you should DCHECK draw result?
> 
> Or maybe drawn_viewport_ means something else here other than actually drawn?

Um, well, its the size of the viewport we're drawing this frame. I can dcheck
the draw result sure.

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
On 2016/09/07 18:38:08, enne wrote:
> On 2016/09/07 at 18:28:12, danakj wrote:
> > On 2016/09/07 18:05:52, enne wrote:
> > > I know swap is going to go away, but why does SwapBuffers happen before
> > > DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to preserve
> the
> > > old ordering?
> > 
> > What's happening is the frame is being given to the output surface inside
> "DrawAndSwapCombined" step. So when we tell the test LTHI has done this step,
it
> has already also been told about the CompositorFrame handed to the output
> surface. I double checked and no tests except those I touched in this CL were
> both using DrawLayersOnThread and DisplayReceivedCompositorFrame.
> 
> I figured that this was safe to do in the current state of tests given that
> things were green.  But, why?  Why reorder them if you don't have to?  Or do
you
> have to and I misunderstand?

Well, usually TestHooks happen *after* LTHI does some thing, unless it is
prefixed "Will". Before this change, after drawing, swap was not done yet. While
technically still true, swap is being merged into the draw step. So I am here
pretending that draw doesn't exist anymore, only swap (which will be true in
https://codereview.chromium.org/2317953002/ tho i named it back to draw :).
Since the hook is after the "draw and swap combined" step, it will be after
sending the frame to the output surface.

[danakj, 2016-09-07 18:52:00]

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/output/delegating_re...
cc/output/delegating_renderer_unittest.cc:32: return draw_result;
On 2016/09/07 18:42:15, danakj wrote:
> On 2016/09/07 18:05:51, enne wrote:
> > Previously this wouldn't have been called if the DrawResult was that it
wasn't
> > going to draw? Maybe you should DCHECK draw result?
> > 
> > Or maybe drawn_viewport_ means something else here other than actually
drawn?
> 
> Um, well, its the size of the viewport we're drawing this frame. I can dcheck
> the draw result sure.

Done.

[danakj, 2016-09-07 18:52:09]

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

[commit-bot: I haz the power, 2016-09-07 18:52:36]

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-09-07 19:02:58]

lgtm

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
On 2016/09/07 at 18:42:16, danakj wrote:
> On 2016/09/07 18:38:08, enne wrote:
> > On 2016/09/07 at 18:28:12, danakj wrote:
> > > On 2016/09/07 18:05:52, enne wrote:
> > > > I know swap is going to go away, but why does SwapBuffers happen before
> > > > DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to
preserve
> > the
> > > > old ordering?
> > > 
> > > What's happening is the frame is being given to the output surface inside
> > "DrawAndSwapCombined" step. So when we tell the test LTHI has done this
step, it
> > has already also been told about the CompositorFrame handed to the output
> > surface. I double checked and no tests except those I touched in this CL
were
> > both using DrawLayersOnThread and DisplayReceivedCompositorFrame.
> > 
> > I figured that this was safe to do in the current state of tests given that
> > things were green.  But, why?  Why reorder them if you don't have to?  Or do
you
> > have to and I misunderstand?
> 
> Well, usually TestHooks happen *after* LTHI does some thing, unless it is
prefixed "Will". Before this change, after drawing, swap was not done yet. While
technically still true, swap is being merged into the draw step. So I am here
pretending that draw doesn't exist anymore, only swap (which will be true in
https://codereview.chromium.org/2317953002/ tho i named it back to draw :).
Since the hook is after the "draw and swap combined" step, it will be after
sending the frame to the output surface.

Sure, DrawLayersOnThread always happens after DrawLayers.  But it's that the
DisplayReceivedCompositorFrame might happen before DrawLayersOnThread now.  I
guess if they're getting merged then this doesn't matter.  I still don't
understand why you'd reorder them in this patch, since it doesn't seem
necessary, but whatever.

[danakj, 2016-09-07 19:05:46]

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2317493003/diff/30001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:187: bool r = LayerTreeHostImpl::SwapBuffers(frame);
On 2016/09/07 19:02:58, enne wrote:
> On 2016/09/07 at 18:42:16, danakj wrote:
> > On 2016/09/07 18:38:08, enne wrote:
> > > On 2016/09/07 at 18:28:12, danakj wrote:
> > > > On 2016/09/07 18:05:52, enne wrote:
> > > > > I know swap is going to go away, but why does SwapBuffers happen
before
> > > > > DrawLayersOnThread? Shouldn't DrawLayersOnThread happen first to
> preserve
> > > the
> > > > > old ordering?
> > > > 
> > > > What's happening is the frame is being given to the output surface
inside
> > > "DrawAndSwapCombined" step. So when we tell the test LTHI has done this
> step, it
> > > has already also been told about the CompositorFrame handed to the output
> > > surface. I double checked and no tests except those I touched in this CL
> were
> > > both using DrawLayersOnThread and DisplayReceivedCompositorFrame.
> > > 
> > > I figured that this was safe to do in the current state of tests given
that
> > > things were green.  But, why?  Why reorder them if you don't have to?  Or
do
> you
> > > have to and I misunderstand?
> > 
> > Well, usually TestHooks happen *after* LTHI does some thing, unless it is
> prefixed "Will". Before this change, after drawing, swap was not done yet.
While
> technically still true, swap is being merged into the draw step. So I am here
> pretending that draw doesn't exist anymore, only swap (which will be true in
> https://codereview.chromium.org/2317953002/ tho i named it back to draw :).
> Since the hook is after the "draw and swap combined" step, it will be after
> sending the frame to the output surface.
> 
> Sure, DrawLayersOnThread always happens after DrawLayers.  But it's that the
> DisplayReceivedCompositorFrame might happen before DrawLayersOnThread now.  I
> guess if they're getting merged then this doesn't matter.  I still don't
> understand why you'd reorder them in this patch, since it doesn't seem
> necessary, but whatever.

Ah, just cuz I wanted to make tests work with that reordering, I had problems
with tests before when merging so I wanted to work thru them in smaller logical
changes.

[danakj, 2016-09-07 19:05:51]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-09-07 19:05:56]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-09-07 19:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2317493003/#ps50001
(title: "merge-draw-swap: add-draw-result-checks")

[commit-bot: I haz the power, 2016-09-07 19:06: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-09-07 19:51:12]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2016-09-07 19:53:14]

Description was changed from

==========
cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

This moves calls from the proxies that happen between
LayerTreeHostImpl's DrawLayers and SwapBuffers to happen after
SwapBuffers, and moves all code at the bottom of DrawLayers, which
is after DelegatingRenderer::DrawFrame to the bottom of SwapBuffers,
which makes it after DelegatingRenderer::SwapBuffers.

R=enne
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Toward merging DrawFrame and SwapBuffers in DelegatingRenderer.

This moves calls from the proxies that happen between
LayerTreeHostImpl's DrawLayers and SwapBuffers to happen after
SwapBuffers, and moves all code at the bottom of DrawLayers, which
is after DelegatingRenderer::DrawFrame to the bottom of SwapBuffers,
which makes it after DelegatingRenderer::SwapBuffers.

R=enne
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/097919e7138f21a14a577688752ea955c47b16fa
Cr-Commit-Position: refs/heads/master@{#417034}
==========

[commit-bot: I haz the power, 2016-09-07 19:53:16]

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