Update Surface ID Terminology

Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/27866427f8e663f7b43e698006bb495b97726bfa
Cr-Commit-Position: refs/heads/master@{#404998}

[Fady Samuel, 2016-07-11 23:16:51]

Description was changed from

==========
[WIP] Update Surface ID to support client-side allocation

BUG=
==========

to

==========
[WIP] Update Surface ID to support client-side allocation

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-11 23:16:52]

Description was changed from

==========
[WIP] Update Surface ID to support client-side allocation

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[WIP] Update Surface ID to support client-side allocation

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

[Fady Samuel, 2016-07-12 18:41:31]

Description was changed from

==========
[WIP] Update Surface ID to support client-side allocation

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

to

==========
Update Surface ID to support client-side allocation

This CL is a pre-requisite to supporting client-side Surface 
ID allocation. Both in Mus and in Chrome, returned resources
should not be reclaimed if the surface ID is stale. This can
happen if an OutputSurface is recreated after the Gpu process
is restarted.

This CL introduces a gpu_id in SurfaceId that will be 
allocated in EstablishGpuChannel in a subsequent CL.

This CL also renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

Finally this CL passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

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

[Fady Samuel, 2016-07-12 18:42:48]

fsamuel@chromium.org changed reviewers:
+ boliu@chromium.org, msw@chromium.org, piman@chromium.org, sadrul@chromium.org

[Fady Samuel, 2016-07-12 18:42:50]

+piman@ for overall (content + cc)
+sadrul@ for ui
+msw@ for services/ui
+boliu@ for android_webview

[Fady Samuel, 2016-07-12 18:43:47]

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

[commit-bot: I haz the power, 2016-07-12 18:44:59]

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

[Fady Samuel, 2016-07-12 19:02:08]

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

[Fady Samuel, 2016-07-12 19:04:54]

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

[Fady Samuel, 2016-07-12 19:04:55]

+dcheng@ for ipc

[commit-bot: I haz the power, 2016-07-12 19:05:53]

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

[msw, 2016-07-12 19:14:09]

services/ui rubber stamp lgtm

[commit-bot: I haz the power, 2016-07-12 19:30:22]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 19:30:23]

Dry run: 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/8...)

[Fady Samuel, 2016-07-12 20:09:41]

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

[commit-bot: I haz the power, 2016-07-12 20:12:42]

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

[piman, 2016-07-12 20:57:19]

https://codereview.chromium.org/2136413002/diff/80001/cc/ipc/surface_id.mojom
File cc/ipc/surface_id.mojom (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/ipc/surface_id.mojom...
cc/ipc/surface_id.mojom:23: // in the same process as the gpu service.
TBH, I'm not sure that's true... This is to prevent races on the client side. It
could very well be that the display_compositor process sends an ack/returns
resources, then crashes. Meanwhile, the client detects the crash, recreates an
output surface, then receives the ack/resource returns. It needs a way to know
that the ack isn't relevant any more.

Or maybe you're saying that there's an out-of-band way to ensure that in the mus
world, similar to the output_surface_id today... if that's the case, is it worth
doing this (somewhat large) patch, just to revert it later?

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:82: uint64_t nonce_;
nit: can we order the fields so that it's more compact - e.g. nonce_ at the
beginning?

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_seq...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_seq...
cc/surfaces/surface_sequence.h:21: SurfaceSequence() : client_id(0u),
sequence(0u) {}
Do we need the gpu_id here too? Or is the assumption that all surfaces that
participate in the dependency graph will have the same gpu_id (if so, is there a
way to check that it's true?)

[Fady Samuel, 2016-07-12 22:41:53]

Description was changed from

==========
Update Surface ID to support client-side allocation

This CL is a pre-requisite to supporting client-side Surface 
ID allocation. Both in Mus and in Chrome, returned resources
should not be reclaimed if the surface ID is stale. This can
happen if an OutputSurface is recreated after the Gpu process
is restarted.

This CL introduces a gpu_id in SurfaceId that will be 
allocated in EstablishGpuChannel in a subsequent CL.

This CL also renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

Finally this CL passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

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

to

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

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

[Fady Samuel, 2016-07-12 22:45:34]

PTAL gpu_id was actually a small change. This CL is mostly renaming and making
things const ref.

https://codereview.chromium.org/2136413002/diff/80001/cc/ipc/surface_id.mojom
File cc/ipc/surface_id.mojom (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/ipc/surface_id.mojom...
cc/ipc/surface_id.mojom:23: // in the same process as the gpu service.
On 2016/07/12 20:57:19, piman wrote:
> TBH, I'm not sure that's true... This is to prevent races on the client side.
It
> could very well be that the display_compositor process sends an ack/returns
> resources, then crashes. Meanwhile, the client detects the crash, recreates an
> output surface, then receives the ack/resource returns. It needs a way to know
> that the ack isn't relevant any more.
> 
> Or maybe you're saying that there's an out-of-band way to ensure that in the
mus
> world, similar to the output_surface_id today... if that's the case, is it
worth
> doing this (somewhat large) patch, just to revert it later?

Well, I think I've confused myself. Really output_surface_id looks like it's
used to establish a "connection". This isn't necessary once we switch to mojo
because each OutputSurface has a different messagepipe and a message from an old
GPU service cannot arrive to a new OutputSurface.

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:82: uint64_t nonce_;
On 2016/07/12 20:57:19, piman wrote:
> nit: can we order the fields so that it's more compact - e.g. nonce_ at the
> beginning?

tsepez@ preferred nonce at the end for readability.

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_seq...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_seq...
cc/surfaces/surface_sequence.h:21: SurfaceSequence() : client_id(0u),
sequence(0u) {}
On 2016/07/12 20:57:19, piman wrote:
> Do we need the gpu_id here too? Or is the assumption that all surfaces that
> participate in the dependency graph will have the same gpu_id (if so, is there
a
> way to check that it's true?)

gpu_id is now gone.

[piman, 2016-07-12 22:47:50]

lgtm

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:82: uint64_t nonce_;
On 2016/07/12 22:45:34, Fady Samuel wrote:
> On 2016/07/12 20:57:19, piman wrote:
> > nit: can we order the fields so that it's more compact - e.g. nonce_ at the
> > beginning?
> 
> tsepez@ preferred nonce at the end for readability.

ok - it doesn't matter any more.

[boliu, 2016-07-12 22:51:20]

rs lgtm

[Fady Samuel, 2016-07-13 00:16:27]

TBR'ing dcheng@ and sadrul@ for a trivial rename as it is now.

[Fady Samuel, 2016-07-13 00:16:45]

Description was changed from

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

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

to

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-07-13 00:16:51]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-13 00:16:53]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2136413002/#ps100001
(title: "Remove gpu_id")

[commit-bot: I haz the power, 2016-07-13 00:19:02]

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

[sadrul, 2016-07-13 00:19:21]

lgtm

[commit-bot: I haz the power, 2016-07-13 00:43:22]

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

[commit-bot: I haz the power, 2016-07-13 00:43:24]

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...)

[dcheng, 2016-07-13 01:57:48]

RS LGTM

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:28: // A SurfaceId consists of three components:
namespace, local Id, and nonce.
Nit: namespace -> client ID?

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:32: // compositor client.
Maybe mention something that local_id is unique within a client: it can be
inferred based on the context, but it doesn't hurt to be explicit about it.

[Fady Samuel, 2016-07-13 02:22:30]

CQ'ing.

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:28: // A SurfaceId consists of three components:
namespace, local Id, and nonce.
On 2016/07/13 01:57:48, dcheng wrote:
> Nit: namespace -> client ID?

Done.

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:32: // compositor client.
On 2016/07/13 01:57:48, dcheng wrote:
> Maybe mention something that local_id is unique within a client: it can be
> inferred based on the context, but it doesn't hurt to be explicit about it.

Done.

[Fady Samuel, 2016-07-13 02:22:31]

CQ'ing.

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:28: // A SurfaceId consists of three components:
namespace, local Id, and nonce.
On 2016/07/13 01:57:48, dcheng wrote:
> Nit: namespace -> client ID?

Done.

https://codereview.chromium.org/2136413002/diff/100001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:32: // compositor client.
On 2016/07/13 01:57:48, dcheng wrote:
> Maybe mention something that local_id is unique within a client: it can be
> inferred based on the context, but it doesn't hurt to be explicit about it.

Done.

[Fady Samuel, 2016-07-13 02:22:39]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-13 02:22:40]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sadrul@chromium.org, boliu@chromium.org, dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2136413002/#ps120001
(title: "Fixed webkit_unit_tests")

[commit-bot: I haz the power, 2016-07-13 02:24:24]

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-13 04:07:12]

Description was changed from

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-13 04:07:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-13 04:07:33]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 04:10:26]

Description was changed from

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Update Surface ID Terminology

This CL renames id_namespace to client_id to capture
the fact that clients may be things other than renderers.

This CL also passes SurfaceId by const reference instead
of value now that SurfaceId is a 128-bit quantity.

BUG=627283
TBR=sadrul@chromium.org, dcheng@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/27866427f8e663f7b43e698006bb495b97726bfa
Cr-Commit-Position: refs/heads/master@{#404998}
==========

[commit-bot: I haz the power, 2016-07-13 04:10:28]

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

[michaelpg, 2016-07-15 18:32:56]

https://codereview.chromium.org/2136413002/diff/120001/cc/proto/layer_tree_ho...
File cc/proto/layer_tree_host.proto (right):

https://codereview.chromium.org/2136413002/diff/120001/cc/proto/layer_tree_ho...
cc/proto/layer_tree_host.proto:63: optional uint32 surface_client_id = 32;
Annoyingly, I had to blow away my entire out directory to build cc after this
change for Simple Chrome (chrome on CrOS). No idea why. Removing the subsets of
the out dir that seemed relevant to this change didn't help.

Just commenting in case someone else bisects to this issue too.