List all child surfaces (including occluded) in CompositorFrame

List all child surfaces (including occluded) in CompositorFrame

Then any occluded child surface with a copy request can be aggregated
(and have its copy done) whenever its parent draws.

BUG=529378
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/915e16b393182f9ccc0ddc4caadd5f7744b25fd4
Cr-Commit-Position: refs/heads/master@{#360896}

[jbauman, 2015-11-10 22:50:42]

Description was changed from

==========
List all surfaces (including occluded) in CompositorFrame, and also aggregate
occluded surfaces with copy requests.



BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
List all child surfaces (including occluded) in CompositorFrame

Then any occluded child surface with a copy request can be aggregated (and have
its copy done) whenever its parent draws.

BUG=529378
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbauman, 2015-11-12 01:26:01]

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

[danakj, 2015-11-13 22:40:24]

> Then any occluded child surface with a copy request can be aggregated (and
have its copy done) whenever its parent draws.

Wrap at 72 pls

[jbauman, 2015-11-13 23:52:57]

Description was changed from

==========
List all child surfaces (including occluded) in CompositorFrame

Then any occluded child surface with a copy request can be aggregated (and have
its copy done) whenever its parent draws.

BUG=529378
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
List all child surfaces (including occluded) in CompositorFrame

Then any occluded child surface with a copy request can be aggregated
(and have its copy done) whenever its parent draws.


BUG=529378
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2015-11-16 21:45:46]

LGTM w/ a few comments

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:599: for (const SurfaceId surface_id :
did you mean const&?

or maybe just auto&?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:611: void
SurfaceAggregator::CopyUnreferencedSurfaces(
CopyUndrawnSurfaces?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:613: std::vector<SurfaceId> surfaces_to_copy(
Can you leave a comment here to the effect of how the Prewalk step may have
skipped surfaces that would not be drawn, and any such surface would have been
added to this surfaces_to_copy list. But that each of those surfaces may have
children that it would have skipped implicitly, so they won't appear in this
list and we'll have to look for them here?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:623: bool has_copy_requests = false;
can you name this somehow differently to not almost-collide with
has_copy_requests_? surface_has_copy_requests?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:630: for (const SurfaceId child_id :
ditto?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:632: if
(!prewalk_result->undrawn_surfaces.count(child_id)) {
Why this if? How could a child of an undrawn surface be drawn itself?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:64: PrewalkResult();
do you need a non-default constructor/destructor here?

https://codereview.chromium.org/1430363002/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:66: bool has_copy_requests;
could you just = false here instead of constructor?

https://codereview.chromium.org/1430363002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1430363002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1277: // TODO(danakj): Remove this once crash is
found crbug.com/309777
Oh, I should do this :)

Can you add a similar DCHECK in your Add method?

[jbauman, 2015-11-17 02:02:55]

jbauman@chromium.org changed reviewers:
+ kenrb@chromium.org

[jbauman, 2015-11-17 02:02:56]

kenrb@: IPC change review.

[kenrb, 2015-11-18 16:27:13]

ipc lgtm

[jbauman, 2015-11-19 21:54:01]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2015-11-19 21:54:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1430363002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-11-19 21:56:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430363002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430363002/80001

[commit-bot: I haz the power, 2015-11-19 22:03:59]

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

[commit-bot: I haz the power, 2015-11-19 22:04:00]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2015-11-20 19:58:58]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2015-11-20 19:58:59]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/1430363002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-11-20 19:59:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430363002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430363002/120001

[commit-bot: I haz the power, 2015-11-20 20:12:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-20 20:13:57]

Patchset 7 (id:??) landed as
https://crrev.com/915e16b393182f9ccc0ddc4caadd5f7744b25fd4
Cr-Commit-Position: refs/heads/master@{#360896}

[jbauman, 2015-11-25 07:53:05]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1475643006/ by jbauman@chromium.org.

The reason for reverting is: Seems to have broken resize of some OOPIF.

BUG=560237.