cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

In the future, we'd like to merge Surface client ID registration and
SurfaceFactory creation. SurfaceFactory is tied to an OutputSurface. Thus, we
cannot register a namespace hierarchy relationship until after we create an
OutputSurface for the ui::Compositor. However, we create a new OutputSurface
every time the GPU process is restarted. Thus, we need a mechanism to store
these relationships between clients across GPU restarts.

This is particularly important if the display compositor lives in the same
process GPU and these relationships would be lost. This CL stores the
relationship between DelegatedFrameHost and ui::Compositor in ui::Compositor.

Note that depending on the final shape of the display compositor host API,
we may wish to centralize all hierarchy relationship registration including
OOPIF.

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

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

[Fady Samuel, 2016-07-25 17:27:14]

Description was changed from

==========
cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

In the future, we'd like to merge Surface client ID registration and
SurfaceFactory creation. SurfaceFactory is tied to an OutputSurface. Thus, we
cannot register a namespace hierarchy relationship until after we create an
OutputSurface for the ui::Compositor. However, we create a new OutputSurface
every time the GPU process is restarted. Thus, we need a mechanism to store
these relationships between clients across GPU restarts.

This is particularly important if the display compositor lives in the same
process GPU and these relationships would be lost. This CL stores the
relationship between DelegatedFrameHost and ui::Compositor in ui::Compositor.

Note that depending on the final shape of the display compositor host API,
we may wish to centralize all hierarchy relationship registration including
OOPIF.

BUG=629940
==========

to

==========
cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

In the future, we'd like to merge Surface client ID registration and
SurfaceFactory creation. SurfaceFactory is tied to an OutputSurface. Thus, we
cannot register a namespace hierarchy relationship until after we create an
OutputSurface for the ui::Compositor. However, we create a new OutputSurface
every time the GPU process is restarted. Thus, we need a mechanism to store
these relationships between clients across GPU restarts.

This is particularly important if the display compositor lives in the same
process GPU and these relationships would be lost. This CL stores the
relationship between DelegatedFrameHost and ui::Compositor in ui::Compositor.

Note that depending on the final shape of the display compositor host API,
we may wish to centralize all hierarchy relationship registration including
OOPIF.

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

[Fady Samuel, 2016-07-25 17:27:56]

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

[Fady Samuel, 2016-07-25 17:27:57]

Hi Antoine,

Is this what you had in mind?

Thanks,

Fady

[piman, 2016-07-25 20:13:39]

I think that's the idea overall. A couple of details. Do you think it's possible
to add a unit test?

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:249: surface_id_allocator_->client_id());
Do we need to unregister the surface_clients_ here, or is it implicit through
the InvalidateSurfaceClientId?

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:258: parent_client_id =
surface_id_allocator_->client_id();
Do we need to RegisterSurfaceNamespaceHierarchy at this point?

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:263: void Compositor::RemoveSurfaceClient(uint32_t
client_id) {
And Unregister here, if we have an output surface?

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:275: // If a client already has a parent, then we
unregister the existing parent.
Can we skip if client.second == surface_id_allocator_->client_id() ?

This would be the case (today) if we're just recreating the output surface
because we lost the context, right?

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:394: host_->ReleaseOutputSurface();
You probably want to unregister each of the surface_clients_ here.

[Fady Samuel, 2016-07-26 00:42:14]

PTAL Antoine! Thanks!

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:249: surface_id_allocator_->client_id());
On 2016/07/25 20:13:39, piman - slow reviews until 8-8 wrote:
> Do we need to unregister the surface_clients_ here, or is it implicit through
> the InvalidateSurfaceClientId?

Done.

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:258: parent_client_id =
surface_id_allocator_->client_id();
On 2016/07/25 20:13:39, piman - slow reviews until 8-8 wrote:
> Do we need to RegisterSurfaceNamespaceHierarchy at this point?

Yes.

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:263: void Compositor::RemoveSurfaceClient(uint32_t
client_id) {
On 2016/07/25 20:13:39, piman - slow reviews until 8-8 wrote:
> And Unregister here, if we have an output surface?

Done.

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:275: // If a client already has a parent, then we
unregister the existing parent.
On 2016/07/25 20:13:39, piman - slow reviews until 8-8 wrote:
> Can we skip if client.second == surface_id_allocator_->client_id() ?
> 
> This would be the case (today) if we're just recreating the output surface
> because we lost the context, right?

Yes, we can skip for now. I want to move the SurfaceIdAllocator to
SurfaceDisplayOutputSurface in a subsequent CL. In that case, that will no
longer be true.

https://codereview.chromium.org/2175293003/diff/1/ui/compositor/compositor.cc...
ui/compositor/compositor.cc:394: host_->ReleaseOutputSurface();
On 2016/07/25 20:13:39, piman - slow reviews until 8-8 wrote:
> You probably want to unregister each of the surface_clients_ here.

Done.

[Fady Samuel, 2016-07-26 00:42:26]

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

[commit-bot: I haz the power, 2016-07-26 00:42:52]

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-26 01:31:07]

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

[commit-bot: I haz the power, 2016-07-26 01:31:08]

Dry run: This issue passed the CQ dry run.

[piman, 2016-07-26 06:41:17]

lgtm

[Fady Samuel, 2016-07-26 11:44:53]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-07-26 11:45:12]

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-26 11:48:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-26 11:49:53]

Description was changed from

==========
cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

In the future, we'd like to merge Surface client ID registration and
SurfaceFactory creation. SurfaceFactory is tied to an OutputSurface. Thus, we
cannot register a namespace hierarchy relationship until after we create an
OutputSurface for the ui::Compositor. However, we create a new OutputSurface
every time the GPU process is restarted. Thus, we need a mechanism to store
these relationships between clients across GPU restarts.

This is particularly important if the display compositor lives in the same
process GPU and these relationships would be lost. This CL stores the
relationship between DelegatedFrameHost and ui::Compositor in ui::Compositor.

Note that depending on the final shape of the display compositor host API,
we may wish to centralize all hierarchy relationship registration including
OOPIF.

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

to

==========
cc: Register namespace hierarchy after ui::Compositor's SurfaceFactory available

In the future, we'd like to merge Surface client ID registration and
SurfaceFactory creation. SurfaceFactory is tied to an OutputSurface. Thus, we
cannot register a namespace hierarchy relationship until after we create an
OutputSurface for the ui::Compositor. However, we create a new OutputSurface
every time the GPU process is restarted. Thus, we need a mechanism to store
these relationships between clients across GPU restarts.

This is particularly important if the display compositor lives in the same
process GPU and these relationships would be lost. This CL stores the
relationship between DelegatedFrameHost and ui::Compositor in ui::Compositor.

Note that depending on the final shape of the display compositor host API,
we may wish to centralize all hierarchy relationship registration including
OOPIF.

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

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

[commit-bot: I haz the power, 2016-07-26 11:49:54]

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