Separate client surface reference tracking from FrameGenerator.

Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2610063007
Cr-Commit-Position: refs/heads/master@{#442997}
Committed: https://chromium.googlesource.com/chromium/src/+/d358fb4f2ad43329a2109fde0f84e9ffe4f1feae

[kylechar, 2017-01-05 19:13:58]

Patchset #3 (id:40001) has been deleted

[kylechar, 2017-01-05 19:30:32]

Description was changed from

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to N embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

BUG=659227
==========

to

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
==========

[kylechar, 2017-01-05 19:30:40]

kylechar@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2017-01-05 19:36:19]

Looks great, but it would be nice for this to live in cc/surfaces so we can use
it immediately in Chrome, and don't depend on mojo.

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
File services/ui/ws/embedded_surface_tracker.h (right):

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
services/ui/ws/embedded_surface_tracker.h:27: class EmbeddedSurfaceTracker {
Can we move this to cc/surfaces.

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
services/ui/ws/embedded_surface_tracker.h:60:
cc::mojom::MojoCompositorFrameSink* compositor_frame_sink);
Don't depend on mojo.

[kylechar, 2017-01-06 17:07:09]

Description was changed from

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
==========

to

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-01-06 17:58:41]

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
File services/ui/ws/embedded_surface_tracker.h (right):

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
services/ui/ws/embedded_surface_tracker.h:27: class EmbeddedSurfaceTracker {
On 2017/01/05 19:36:19, Fady Samuel wrote:
> Can we move this to cc/surfaces.

Done.

https://codereview.chromium.org/2610063007/diff/60001/services/ui/ws/embedded...
services/ui/ws/embedded_surface_tracker.h:60:
cc::mojom::MojoCompositorFrameSink* compositor_frame_sink);
On 2017/01/05 19:36:19, Fady Samuel wrote:
> Don't depend on mojo.

Done.

[Fady Samuel, 2017-01-06 20:43:53]

lgtm

[kylechar, 2017-01-10 16:50:15]

kylechar@chromium.org changed reviewers:
+ jbauman@chromium.org, sky@chromium.org

[kylechar, 2017-01-10 16:50:16]

+jbauman for cc/surfaces/*
+sky for services/ui/ws/*

[sky, 2017-01-10 18:23:55]

LGTM

[jbauman, 2017-01-11 00:26:25]

lgtm

https://codereview.chromium.org/2610063007/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2610063007/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:37: // unreachable so it can be garbage
collected.
It's a bit odd that this would matter, as the compositor frame sink should keep
the display root surface alive as long as it lives, and this removes the surface
reference immediately before it dies.

[kylechar, 2017-01-11 18:04:49]

Thanks!

https://codereview.chromium.org/2610063007/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2610063007/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:37: // unreachable so it can be garbage
collected.
On 2017/01/11 00:26:25, jbauman wrote:
> It's a bit odd that this would matter, as the compositor frame sink should
keep
> the display root surface alive as long as it lives, and this removes the
surface
> reference immediately before it dies.

I hadn't really thought too much about it but you're right there is something
weird here. SurfaceManager will still keep the display root surface until it's
been marked as destroyed, even after the reference is gone, so that isn't a huge
problem. The embedded surfaces will no longer be reachable though, so if they
are marked as destroyed they could deleted before the display root and that
should be fixed.

I talked to fsamuel@ briefly and managing references from TLR could just happen
in the GPU process. GpuCompositorFrameSink has enough information to add and
remove TLR references for the display roots. It can also remove the TLR after
the display root is marked as destroyed to avoid the problem. It also simplifies
things. Will do that in a future CL.

[kylechar, 2017-01-11 18:07:29]

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

[commit-bot: I haz the power, 2017-01-11 18:08:17]

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, 2017-01-11 18:11:30]

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

[commit-bot: I haz the power, 2017-01-11 18:11:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylechar, 2017-01-11 19:15:13]

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

[commit-bot: I haz the power, 2017-01-11 19:16:13]

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

[kylechar, 2017-01-11 20:19:57]

The CQ bit was unchecked by kylechar@chromium.org

[kylechar, 2017-01-11 20:19:59]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-01-11 20:20:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, sky@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2610063007/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-01-11 20:20:25]

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-01-11 21:54:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484165999947010,
"parent_rev": "b622b0bbb1c98da5d7ce4b9e7749698959b93fa1", "commit_rev":
"d358fb4f2ad43329a2109fde0f84e9ffe4f1feae"}

[commit-bot: I haz the power, 2017-01-11 21:54:39]

Description was changed from

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Separate client surface reference tracking from FrameGenerator.

The logic to track surface references for surface embeddings was built
as part of FrameGenerator. Generalize the logic and remove from
FrameGenerator, creating the class EmbeddedSurfaceTracker.

EmbeddedSurfaceTracker keeps track of surface references from one client
surface to many embedded surfaces. It also handles generating new
references when the client surface changes. Includes unit tests.

This simplifies FrameGenerator and should make it easier to use surface
references in other clients.

BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2610063007
Cr-Commit-Position: refs/heads/master@{#442997}
Committed:
https://chromium.googlesource.com/chromium/src/+/d358fb4f2ad43329a2109fde0f84...
==========

[commit-bot: I haz the power, 2017-01-11 21:54:40]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d358fb4f2ad43329a2109fde0f84...

[Wez, 2017-02-07 05:57:00]

wez@chromium.org changed reviewers:
+ danakj@chromium.org, wez@chromium.org

[Wez, 2017-02-07 05:57:00]

Hallo danakj@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective rubber-stampy review from you:
	cc/BUILD.gn
Thanks,
Wez

[danakj, 2017-02-07 15:56:11]

cc/ LGTM