cc: Glue LTHI and Scheduler changes for checker-imaging.

cc: Glue LTHI and Scheduler changes for checker-imaging.

This change glues together the scheduling and tile management changes
for checker-imaging via ProxyImpl and SingleThreadProxy for enabling
checker-imaging. And adds integration LayerTreeTests, including a
scroll test to ensure that an impl-side pending tree does not affect
SyncedProperty state synchronization.

BUG=686267, 691041
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2717553005
Cr-Commit-Position: refs/heads/master@{#453508}
Committed: https://chromium.googlesource.com/chromium/src/+/6dc91d0e5b2f135eee29fc0e2a506747a18dc439

[Khushal, 2017-02-24 20:29:19]

Description was changed from

==========
cc: Glue LTHI and Scheduler changes for checker-imaging.

This change glues together the scheduling and tile management changes
for checker-imaging via ProxyImpl and SingleThreadProxy for enabling
checker-imaging. And adds integration LayerTreeTests, including a
scroll test to ensure that an impl-side pending tree does not affect
SyncedProperty state synchronization.

BUG=686267, 691041
==========

to

==========
cc: Glue LTHI and Scheduler changes for checker-imaging.

This change glues together the scheduling and tile management changes
for checker-imaging via ProxyImpl and SingleThreadProxy for enabling
checker-imaging. And adds integration LayerTreeTests, including a
scroll test to ensure that an impl-side pending tree does not affect
SyncedProperty state synchronization.

BUG=686267, 691041
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-02-24 20:29:38]

khushalsagar@chromium.org changed reviewers:
+ enne@chromium.org, vmpstr@chromium.org

[Khushal, 2017-02-24 20:32:13]

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

[commit-bot: I haz the power, 2017-02-24 20:33:24]

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

[enne (OOO), 2017-02-24 20:43:57]

Thanks for all these tests.  These are great.  :D

lgtm % vmpstr

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:108: DCHECK(!settings_.enable_checker_imaging ||
image_worker_task_runner_);
I wish we had DCHECK_IMPLIES :C

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

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:263: DCHECK(!is_synchronous_single_threaded_ ||
!settings_.enable_checker_imaging);
Maybe move this to single thread proxy?

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

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:238: virtual void DidSendBeginMainFrame() {}  //
For Tests.
I think either name this DidSendBeginMainFrameForTesting (and make this
protected?) or remove the comment?

[commit-bot: I haz the power, 2017-02-24 22:17:02]

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

[commit-bot: I haz the power, 2017-02-24 22:17:03]

Dry run: This issue passed the CQ dry run.

[Khushal, 2017-02-24 23:19:41]

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:108: DCHECK(!settings_.enable_checker_imaging ||
image_worker_task_runner_);
On 2017/02/24 20:43:57, enne wrote:
> I wish we had DCHECK_IMPLIES :C

Yeah, that would be cool. It ends up being used a lot.

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

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:263: DCHECK(!is_synchronous_single_threaded_ ||
!settings_.enable_checker_imaging);
On 2017/02/24 20:43:57, enne wrote:
> Maybe move this to single thread proxy?

Done.

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

https://codereview.chromium.org/2717553005/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.h:238: virtual void DidSendBeginMainFrame() {}  //
For Tests.
On 2017/02/24 20:43:57, enne wrote:
> I think either name this DidSendBeginMainFrameForTesting (and make this
> protected?) or remove the comment?

Done, I added ForTesting there. Can't make it protected because the proxies call
it.

[vmpstr, 2017-02-27 21:16:57]

https://codereview.chromium.org/2717553005/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2717553005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:290: bool
block_notify_ready_to_activate_for_testing_;
while here, can you also initialize the rest of the variables?

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc#...
cc/trees/proxy_impl.cc:468:
layer_tree_host_impl_->DidSendBeginMainFrameForTesting();
I don't think this is correct. We shouldn't be calling "for testing" functions
in non-for testing code.

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:609:
layer_tree_host_impl_->DidSendBeginMainFrameForTesting();
same here

[Khushal, 2017-02-27 21:43:01]

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc#...
cc/trees/proxy_impl.cc:468:
layer_tree_host_impl_->DidSendBeginMainFrameForTesting();
On 2017/02/27 21:16:57, vmpstr wrote:
> I don't think this is correct. We shouldn't be calling "for testing" functions
> in non-for testing code.

We kinda do that with MainFrameWillHappenForTesting in the proxies, but it does
start with test code.
Do you think I should just not bother making it ForTesting then?

[vmpstr, 2017-02-27 23:07:09]

On 2017/02/27 21:43:01, Khushal wrote:
> https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc
> File cc/trees/proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/2717553005/diff/40001/cc/trees/proxy_impl.cc#...
> cc/trees/proxy_impl.cc:468:
> layer_tree_host_impl_->DidSendBeginMainFrameForTesting();
> On 2017/02/27 21:16:57, vmpstr wrote:
> > I don't think this is correct. We shouldn't be calling "for testing"
functions
> > in non-for testing code.
> 
> We kinda do that with MainFrameWillHappenForTesting in the proxies, but it
does
> start with test code.
> Do you think I should just not bother making it ForTesting then?

The difference with MainFrameWillHappenForTesting is that we don't _call_ it in
non-test code. It's fine to have it in the interface, and it's fine to implement
it (obviously), but I want to avoid calling it in non test code. You can either
move the callsites to LayerTreeTest (if possible), or I don't think this should
be ForTesting.

[Khushal, 2017-02-28 00:39:50]

Removed the ForTesting in that case.

https://codereview.chromium.org/2717553005/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2717553005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:290: bool
block_notify_ready_to_activate_for_testing_;
On 2017/02/27 21:16:57, vmpstr wrote:
> while here, can you also initialize the rest of the variables?

Done.

[vmpstr, 2017-02-28 01:11:27]

lgtm

https://codereview.chromium.org/2717553005/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2717553005/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:110: settings_.image_decode_tasks_enabled);
can you add a TODO and/or file a bug to consider removing this flag?

[Khushal, 2017-02-28 03:16:12]

https://codereview.chromium.org/2717553005/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2717553005/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:110: settings_.image_decode_tasks_enabled);
On 2017/02/28 01:11:27, vmpstr wrote:
> can you add a TODO and/or file a bug to consider removing this flag?

Done.

[Khushal, 2017-02-28 03:16:19]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-28 03:16:19]

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/2717553005/#ps80001
(title: "..")

[commit-bot: I haz the power, 2017-02-28 03:16:42]

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, 2017-02-28 05:10:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488251779396500,
"parent_rev": "dc9d2cfd2402fe7c6e8e4b18a6cab2e03940bcb4", "commit_rev":
"6dc91d0e5b2f135eee29fc0e2a506747a18dc439"}

[commit-bot: I haz the power, 2017-02-28 05:11:12]

Description was changed from

==========
cc: Glue LTHI and Scheduler changes for checker-imaging.

This change glues together the scheduling and tile management changes
for checker-imaging via ProxyImpl and SingleThreadProxy for enabling
checker-imaging. And adds integration LayerTreeTests, including a
scroll test to ensure that an impl-side pending tree does not affect
SyncedProperty state synchronization.

BUG=686267, 691041
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Glue LTHI and Scheduler changes for checker-imaging.

This change glues together the scheduling and tile management changes
for checker-imaging via ProxyImpl and SingleThreadProxy for enabling
checker-imaging. And adds integration LayerTreeTests, including a
scroll test to ensure that an impl-side pending tree does not affect
SyncedProperty state synchronization.

BUG=686267, 691041
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2717553005
Cr-Commit-Position: refs/heads/master@{#453508}
Committed:
https://chromium.googlesource.com/chromium/src/+/6dc91d0e5b2f135eee29fc0e2a50...
==========

[commit-bot: I haz the power, 2017-02-28 05:11:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6dc91d0e5b2f135eee29fc0e2a50...