cc: Clean up LayerTreeTest and TestHooks.

cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

Committed: https://crrev.com/94b7e4fcc1fb51a468d3df292c622905c67405c5
Cr-Commit-Position: refs/heads/master@{#406450}

[danakj, 2016-07-18 20:44:18]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

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

[danakj, 2016-07-18 20:45:09]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

[danakj, 2016-07-18 20:45:16]

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

[commit-bot: I haz the power, 2016-07-18 20:46:01]

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

[danakj, 2016-07-18 20:46:03]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

[danakj, 2016-07-18 20:47:38]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

[danakj, 2016-07-18 20:47:42]

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

[commit-bot: I haz the power, 2016-07-18 20:48:42]

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

[danakj, 2016-07-18 21:01:55]

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

[commit-bot: I haz the power, 2016-07-18 21:03:32]

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

[danakj, 2016-07-18 21:04:30]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-07-18 21:06:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[danakj, 2016-07-18 21:06:29]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are named __OnTread() and take
a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

[danakj, 2016-07-18 21:07:31]

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

[commit-bot: I haz the power, 2016-07-18 21:08:21]

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

[danakj, 2016-07-18 21:11:22]

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

[commit-bot: I haz the power, 2016-07-18 21:12:16]

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

[danakj, 2016-07-18 21:12:51]

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

[danakj, 2016-07-18 21:12:52]

https://codereview.chromium.org/2158973002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2158973002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2621: class OnDrawOutputSurface : public
OutputSurface {
+boliu FYI: i made these LTH tests more like real webview, by having the output
surface do swap(complete) the way webview does.

[danakj, 2016-07-18 21:13:05]

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

[commit-bot: I haz the power, 2016-07-18 21:13:50]

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-07-18 21:48:03]

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:420: test_hooks_->DidSetNeedsCommit();
What is this used for?

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:427: test_hooks_->DidSetNeedsUpdateLayers();
This looks like it's used by only one test.  Is it really needed at all?

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_proxy.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_proxy.cc:281: LOG(ERROR) << "DidCommit " <<
layer_tree_host()->source_frame_number();
<_< here and elsewhere

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:274: layer_tree_host_impl_->BeginMainFrameCompleted();
Just from a readability perspective, it's a little weird to have this here.  I
would have expected BeginMainFrameCompleted to occur at the end of
BeginMainFrame, and anything happening here to be called StartCommitOnImpl, as
it was called before.

[commit-bot: I haz the power, 2016-07-18 22:40:09]

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

[commit-bot: I haz the power, 2016-07-18 22:40:10]

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...)

[danakj, 2016-07-18 23:14:13]

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

[commit-bot: I haz the power, 2016-07-18 23:15:31]

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

[danakj, 2016-07-18 23:20:10]

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:420: test_hooks_->DidSetNeedsCommit();
On 2016/07/18 21:48:02, enne wrote:
> What is this used for?

I just added this cuz I added it below. <_<

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:427: test_hooks_->DidSetNeedsUpdateLayers();
On 2016/07/18 21:48:02, enne wrote:
> This looks like it's used by only one test.  Is it really needed at all?

That test is checking that layer_tree_host()->ApplyScrollAndScale() does
SetNeedsUpdateLayers() when appropriate, and doesn't when not.
LayerTreeHost::SetNeedsUpdateLayers just calls the proxy and notifies swap
promises. Perhaps I could reimplement the test with swap promises, but it would
not be able to distinguish LayerTreeHost::SetNeedsUpdateLayers() from
LayerTreeHost::SetNeedsAnimate() from LayerTreeHost::SetNeedsCommit().

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_proxy.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_proxy.cc:281: LOG(ERROR) << "DidCommit " <<
layer_tree_host()->source_frame_number();
On 2016/07/18 21:48:02, enne wrote:
> <_< here and elsewhere

Woops.

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:274: layer_tree_host_impl_->BeginMainFrameCompleted();
On 2016/07/18 21:48:02, enne wrote:
> Just from a readability perspective, it's a little weird to have this here.  I
> would have expected BeginMainFrameCompleted to occur at the end of
> BeginMainFrame, and anything happening here to be called StartCommitOnImpl, as
> it was called before.

It needs to happen before NotifyReadyToCommit and on the impl thread. This
translates to BeginMainFrameCompletedOnThread() in tests which is more clear
(the OnThread part is kinda implicit here).

It's 1:1 with the NotifyBeginMainFrameStarted() to the scheduler, which is to
say main frame is done on the thread.

This function is called StartCommitOnImpl for historical reasons, but it won't
really start a commit, it's more like
"MainThreadIsBlockedAndMainFrameIsDoneOnImpl" or something. Maybe I should
rename it to just BeginMainFrameCompletedOnImpl?

[danakj, 2016-07-18 23:22:12]

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

[commit-bot: I haz the power, 2016-07-18 23:22:48]

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

[danakj, 2016-07-18 23:24:12]

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:427: test_hooks_->DidSetNeedsUpdateLayers();
On 2016/07/18 23:20:10, danakj wrote:
> On 2016/07/18 21:48:02, enne wrote:
> > This looks like it's used by only one test.  Is it really needed at all?
> 
> That test is checking that layer_tree_host()->ApplyScrollAndScale() does
> SetNeedsUpdateLayers() when appropriate, and doesn't when not.
> LayerTreeHost::SetNeedsUpdateLayers just calls the proxy and notifies swap
> promises. Perhaps I could reimplement the test with swap promises, but it
would
> not be able to distinguish LayerTreeHost::SetNeedsUpdateLayers() from
> LayerTreeHost::SetNeedsAnimate() from LayerTreeHost::SetNeedsCommit().

It could also maybe not be a LayerTreeTest and just make a LayerTreeHost that it
overrides that method itself, then no TestHooks needed. Not sure how much stuff
I need to get a LayerTreeHost like that going we have a buffet of fake LTH
harnesses.

[danakj, 2016-07-19 00:56:24]

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:420: test_hooks_->DidSetNeedsCommit();
On 2016/07/18 23:20:10, danakj wrote:
> On 2016/07/18 21:48:02, enne wrote:
> > What is this used for?
> 
> I just added this cuz I added it below. <_<

Removed

https://codereview.chromium.org/2158973002/diff/100001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:427: test_hooks_->DidSetNeedsUpdateLayers();
On 2016/07/18 23:24:11, danakj wrote:
> On 2016/07/18 23:20:10, danakj wrote:
> > On 2016/07/18 21:48:02, enne wrote:
> > > This looks like it's used by only one test.  Is it really needed at all?
> > 
> > That test is checking that layer_tree_host()->ApplyScrollAndScale() does
> > SetNeedsUpdateLayers() when appropriate, and doesn't when not.
> > LayerTreeHost::SetNeedsUpdateLayers just calls the proxy and notifies swap
> > promises. Perhaps I could reimplement the test with swap promises, but it
> would
> > not be able to distinguish LayerTreeHost::SetNeedsUpdateLayers() from
> > LayerTreeHost::SetNeedsAnimate() from LayerTreeHost::SetNeedsCommit().
> 
> It could also maybe not be a LayerTreeTest and just make a LayerTreeHost that
it
> overrides that method itself, then no TestHooks needed. Not sure how much
stuff
> I need to get a LayerTreeHost like that going we have a buffet of fake LTH
> harnesses.

Removed along with the test since it is covered by AbortBeginFrame tests in
lth_unittest_scroll.cc

[danakj, 2016-07-19 00:56:48]

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

[commit-bot: I haz the power, 2016-07-19 00:57:13]

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-07-19 01:09:00]

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:274: layer_tree_host_impl_->BeginMainFrameCompleted();
On 2016/07/18 at 23:20:10, danakj wrote:
> On 2016/07/18 21:48:02, enne wrote:
> > Just from a readability perspective, it's a little weird to have this here. 
I
> > would have expected BeginMainFrameCompleted to occur at the end of
> > BeginMainFrame, and anything happening here to be called StartCommitOnImpl,
as
> > it was called before.
> 
> It needs to happen before NotifyReadyToCommit and on the impl thread. This
translates to BeginMainFrameCompletedOnThread() in tests which is more clear
(the OnThread part is kinda implicit here).
> 
> It's 1:1 with the NotifyBeginMainFrameStarted() to the scheduler, which is to
say main frame is done on the thread.
> 
> This function is called StartCommitOnImpl for historical reasons, but it won't
really start a commit, it's more like
"MainThreadIsBlockedAndMainFrameIsDoneOnImpl" or something. Maybe I should
rename it to just BeginMainFrameCompletedOnImpl?

DidStartCommitOnImpl?

[danakj, 2016-07-19 01:36:37]

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2158973002/diff/100001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:274: layer_tree_host_impl_->BeginMainFrameCompleted();
On 2016/07/19 01:08:59, enne wrote:
> On 2016/07/18 at 23:20:10, danakj wrote:
> > On 2016/07/18 21:48:02, enne wrote:
> > > Just from a readability perspective, it's a little weird to have this
here. 
> I
> > > would have expected BeginMainFrameCompleted to occur at the end of
> > > BeginMainFrame, and anything happening here to be called
StartCommitOnImpl,
> as
> > > it was called before.
> > 
> > It needs to happen before NotifyReadyToCommit and on the impl thread. This
> translates to BeginMainFrameCompletedOnThread() in tests which is more clear
> (the OnThread part is kinda implicit here).
> > 
> > It's 1:1 with the NotifyBeginMainFrameStarted() to the scheduler, which is
to
> say main frame is done on the thread.
> > 
> > This function is called StartCommitOnImpl for historical reasons, but it
won't
> really start a commit, it's more like
> "MainThreadIsBlockedAndMainFrameIsDoneOnImpl" or something. Maybe I should
> rename it to just BeginMainFrameCompletedOnImpl?
> 
> DidStartCommitOnImpl?

I mean, should I rename the method on ProxyImpl to
ProxyImpl::BeginMainFrameCmopletedOnImpl?

I don't want to call the hook CommitStarted or similar, cuz that's not what this
is. This is the main thread saying it's blocked. Commit happens separately from
this when we deem it worthy. And we have a BeginCommitOnThread hook for that.

[commit-bot: I haz the power, 2016-07-19 03:11:05]

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

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

Dry run: This issue passed the CQ dry run.

[danakj, 2016-07-19 22:52:01]

PTAL

Renamed the proxy methods to NotifyReadyToCommitOnImpl. The LTHI method to
ReadyToCommit. The TestHooks to ReadyToCommitOnThread.

[danakj, 2016-07-19 22:53:03]

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

[commit-bot: I haz the power, 2016-07-19 22:53:56]

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-07-19 22:54:31]

lgtm

[danakj, 2016-07-19 22:55:42]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-19 22:55:51]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-07-19 22:56:01]

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

[danakj, 2016-07-19 22:56:10]

I added ReadyToCommitOnThread to the LayerTreeHostTestFrameOrdering test also so
we know it works for both proxies.

[danakj, 2016-07-19 22:57:02]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-19 22:57:03]

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/2158973002/#ps200001
(title: "proxy-impls: test")

[commit-bot: I haz the power, 2016-07-19 22:57:45]

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-07-20 01:49:52]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

[commit-bot: I haz the power, 2016-07-20 01:49:54]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-07-20 01:51:13]

Description was changed from

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

to

==========
cc: Clean up LayerTreeTest and TestHooks in cc_unittests.

While working on new SwapBuffers test hooks for OutputSurface vs
CompositorFrameSink, it became apparent that these two APIs have grown
much bigger than they need to be. So I spent some time cleaning them
up.

1) All TestHooks are steps on LayerTreeHost/LayerTreeHostImpl. No steps
on the proxy exist anymore. I changed tests to use LTHI hooks instead
where needed, and had to add one method on LTHI for TestHooks to
override.

2) All TestHooks on the compositor thread are (mostly) named __OnThread()
and they all take a LayerTreeHostImpl* as a parameter.

3) I deleted the Proxy{Main,Impl}ForTests and friends classes. I had to
rewrite the activation-blocked tests to not need to get access to the
ProxyImpl* directly, and the rest mostly fell out of point 1 above.

4) I deleted remote_channel_unittest.cc and threaded_channel_unittest.cc
as these were not testing logic, just checking that posttask works more
or less. I believe the correct way to test the proxies is to test that
the LayerTreeHost API behaves as you'd expect, which is what our other
tests do.

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

Committed: https://crrev.com/94b7e4fcc1fb51a468d3df292c622905c67405c5
Cr-Commit-Position: refs/heads/master@{#406450}
==========

[commit-bot: I haz the power, 2016-07-20 01:51:14]

Patchset 10 (id:??) landed as
https://crrev.com/94b7e4fcc1fb51a468d3df292c622905c67405c5
Cr-Commit-Position: refs/heads/master@{#406450}