cc: Make LayerTreeHostImpl unittests use a delegating output surface.

cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

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

[danakj, 2016-07-19 22:46:20]

Description was changed from

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

R=enne, vmpstr
BUG=606056
==========

to

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

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

[danakj, 2016-07-19 22:46:41]

Oh ya I deleted some tests that should maybe become Display or GLRenderer unit
tests.

[danakj, 2016-07-19 22:46:44]

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:47:37]

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-07-19 22:58:38]

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

[commit-bot: I haz the power, 2016-07-19 22:58:39]

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

[enne (OOO), 2016-07-19 23:00:50]

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface.cc
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:57: last_swap_rect_ = gfx::Rect();
This is really empty and not the full rect?

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7012: TEST_F(LayerTreeHostImplTest,
ReshapeNotCalledUntilDraw) {
I think this is fine to delete as preparing to draw but not drawing is a
renderer only problem.

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7207: class MockContext : public
TestWebGraphicsContext3D {
I do think these should probably be display unit tests of some sort? 
Alternatively, do we cover partial swap and flipping in our pixel tests?

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7813: TEST_F(LayerTreeHostImplTest,
ForcedDrawToSoftwareDeviceBasicRender) {
Do we have a resourceless software draw test somewhere else?

[vmpstr, 2016-07-19 23:27:02]

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface.cc
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:49: last_sent_frame_ =
base::MakeUnique<CompositorFrame>(std::move(frame));
nit: maybe just

last_sent_frame_.reset(new CompositorFrame(std::move(frame)));

Also, is this a unique_ptr because it's playing a role of optional?

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:59: last_swap_rect_ =
last_sent_frame_->gl_frame_data->sub_buffer_rect;
tiny nit: can you swap else and the last else if?

I think this reads better:

if (we have delegated frame data)
  ...
else if (we have a context)
  ...
else
  ...

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:63: if (last_sent_frame_->delegated_frame_data) {
isn't 62-63 kind of corresponds to the if on line 52? Or can software frames
also have these resources held by parent?

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:45: display_ =
base::MakeUnique<Display>(
display_.reset(new Display(

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:52: // Since this OutputSurface and
the Display are tightly coupled and in the
This isn't a complete sentence? Or am I parsing it wrong?

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:89: }
bound_ = false; for posterity?

[danakj, 2016-07-20 01:03:25]

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface.cc
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:49: last_sent_frame_ =
base::MakeUnique<CompositorFrame>(std::move(frame));
On 2016/07/19 23:27:01, vmpstr wrote:
> nit: maybe just
> 
> last_sent_frame_.reset(new CompositorFrame(std::move(frame)));

Sure.

> Also, is this a unique_ptr because it's playing a role of optional?

Ya.

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:57: last_swap_rect_ = gfx::Rect();
On 2016/07/19 23:00:50, enne wrote:
> This is really empty and not the full rect?

Software can partial swap, but it does it thru the software device not thru the
output surface. It only swaps metadata. This is kinda a mess but I'd rather set
it empty than leave it set to whatever it used to be. I could DCHECK that you
don't try to query for software, and I've added that.

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:59: last_swap_rect_ =
last_sent_frame_->gl_frame_data->sub_buffer_rect;
On 2016/07/19 23:27:02, vmpstr wrote:
> tiny nit: can you swap else and the last else if?
> 
> I think this reads better:
> 
> if (we have delegated frame data)
>   ...
> else if (we have a context)
>   ...
> else
>   ...

Done.

https://codereview.chromium.org/2161323002/diff/1/cc/test/fake_output_surface...
cc/test/fake_output_surface.cc:63: if (last_sent_frame_->delegated_frame_data) {
On 2016/07/19 23:27:02, vmpstr wrote:
> isn't 62-63 kind of corresponds to the if on line 52? Or can software frames
> also have these resources held by parent?

Yes, delegated frames can be hardware or software.

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
File cc/test/test_delegating_output_surface.cc (right):

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:45: display_ =
base::MakeUnique<Display>(
On 2016/07/19 23:27:02, vmpstr wrote:
> display_.reset(new Display(

Done.

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:52: // Since this OutputSurface and
the Display are tightly coupled and in the
On 2016/07/19 23:27:02, vmpstr wrote:
> This isn't a complete sentence? Or am I parsing it wrong?

Done.

https://codereview.chromium.org/2161323002/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.cc:89: }
On 2016/07/19 23:27:02, vmpstr wrote:
> bound_ = false; for posterity? 

Done.

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7207: class MockContext : public
TestWebGraphicsContext3D {
On 2016/07/19 23:00:50, enne wrote:
> I do think these should probably be display unit tests of some sort? 
> Alternatively, do we cover partial swap and flipping in our pixel tests?

I added GLRenderer unittests. Will do some software ones tomorrow.

https://codereview.chromium.org/2161323002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:7813: TEST_F(LayerTreeHostImplTest,
ForcedDrawToSoftwareDeviceBasicRender) {
On 2016/07/19 23:00:50, enne wrote:
> Do we have a resourceless software draw test somewhere else?

LayerTreeHostTestResourcelessSoftwareDraw:
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_unittest.cc?rcl...

It tests that we're sending quads to the output surface. It doesn't test a
software device. We could add a SynchronousCompositorOutputSurfaceTest perhaps
to do that. It doesn't have unit tests currently, and if it didn't draw, I guess
we'd hear about that pretty quickly.

[danakj, 2016-07-20 01:04:05]

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

[danakj, 2016-07-20 01:04:41]

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

[danakj, 2016-07-20 01:04:41]

+piman for content/test/

[commit-bot: I haz the power, 2016-07-20 01:05:24]

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

[vmpstr, 2016-07-20 01:11:22]

lgtm

https://codereview.chromium.org/2161323002/diff/20001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2161323002/diff/20001/cc/test/fake_output_sur...
cc/test/fake_output_surface.h:50: return base::WrapUnique(
nit: MakeUnique here? Or keep it for consistency.

https://codereview.chromium.org/2161323002/diff/20001/cc/test/test_context_pr...
File cc/test/test_context_provider.cc (right):

https://codereview.chromium.org/2161323002/diff/20001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:42: DCHECK(context);
We don't usually

DCHECK(foo);
if (!foo)
 ...

do we?

[commit-bot: I haz the power, 2016-07-20 01:17:44]

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

[commit-bot: I haz the power, 2016-07-20 01:17:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[danakj, 2016-07-20 01:24:06]

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

[commit-bot: I haz the power, 2016-07-20 01:24:40]

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-20 01:24:51]

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

[danakj, 2016-07-20 01:24:51]

OK I added a SoftwareRenderer test too PTAL.

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

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-07-20 01:38:04]

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

[commit-bot: I haz the power, 2016-07-20 01:38:05]

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

[enne (OOO), 2016-07-20 18:11:22]

lgtm

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

https://codereview.chromium.org/2161323002/diff/20001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2161323002/diff/20001/cc/test/fake_output_sur...
cc/test/fake_output_surface.h:50: return base::WrapUnique(
On 2016/07/20 01:11:22, vmpstr wrote:
> nit: MakeUnique here? Or keep it for consistency.

That doesnt work for Create functions where the constructors are private.

https://codereview.chromium.org/2161323002/diff/20001/cc/test/test_context_pr...
File cc/test/test_context_provider.cc (right):

https://codereview.chromium.org/2161323002/diff/20001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:42: DCHECK(context);
On 2016/07/20 01:11:22, vmpstr wrote:
> We don't usually
> 
> DCHECK(foo);
> if (!foo)
>  ...
> 
> do we?

No, I was fishing and left that in. I'll remove the if()!

[danakj, 2016-07-20 18:59:29]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-20 18:59:30]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2161323002/#ps60001
(title: "display-lthi-tests: .")

[commit-bot: I haz the power, 2016-07-20 19:00:05]

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

[danakj, 2016-07-20 19:01:15]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-07-20 19:01:16]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2161323002/#ps80001
(title: "display-lthi-tests: .")

[commit-bot: I haz the power, 2016-07-20 19:03:16]

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

[danakj, 2016-07-20 19:05:02]

TBR=piman for the layouttest_support change, just moving some plumbing around.

[danakj, 2016-07-20 19:05:11]

Description was changed from

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

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

to

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

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

Description was changed from

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-20 20:05:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-20 20:08:22]

Description was changed from

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeHostImpl unittests use a delegating output surface.

We are removing all cases of a direct output surface in
LayerTreeHostImpl and the only remaining cases are in unit tests. This
coveres the LayerTreeHostImpl test suite.

TBR=piman
R=enne, vmpstr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-07-20 20:08:24]

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