Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 2ee46f4f425a642b7e6f5a7263b594481261f35d..b874a1d8e0cd394fe69a40994de45fa59a1f2ec7 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -449,8 +449,11 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
   // layer is managed by the DelegatedFrameHost.
   view_.SetLayer(cc::Layer::Create());
   if (using_browser_compositor_) {
+    cc::FrameSinkId frame_sink_id = cc::FrameSinkId(
+        base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),

[boliu, 2017/01/26 00:14:39]

these numbers are ok as the "client id" and "sink id"? maybe should document
what these IDs are somewhere and why this is ok?

[xlai (Olivia), 2017/01/26 15:56:11]

Yes, clientId+SinkId is the frame sink id. Again this is copied from the same
piece of code as non-Mac non-Android platform so as ensure platform consistency.
But here I drop the tertiary operator because is_guest_view_hack_ is never true
in Android.

[Fady Samuel, 2017/01/26 16:06:36]

The FrameSinkId uniquely identifies a CompositorFrameSink (and one day a
ContentFrameSink). FrameSinkId is the persistent (across display compositor
restarts) component of a SurfaceId. A FrameSinkId allows the window server or
browser to know which client submitted a CompositorFrame in response to
OnSurfaceCreated message from the display compositor.

In the Mus window server, each FrameSinkId uniquely identifies a Window (an
embeddable client). In the browser, that is also the case, more or less.
DelegatedFrameHostAndroid is the content area "window" (embeddable client).

In Chrome today, we use the process ID and routing ID combination to uniquely
identify a client. Thus It makes sense for FrameSinkIds to hold the process ID /
routing ID pair. We already ran this by security (tsepez@ in particular).

The interesting this about this pair is FrameSinkId consists of a component
picked by the privileged process (client ID) and a component that can be picked
by an unprivileged process (the sink ID). This enables the client to allocate a
unique ID for embeddable children as is the case with offscreen canvas.

[boliu, 2017/01/26 19:24:45]

I was mostly asking for correctness.

So for renderers, FrameSinkId uses process/routing id. But right now the browser
compositor still uses
ui::ContextProviderFactory::GetInstance()->AllocateFrameSinkId, and it's not
clear to me at this high level that a renderer won't collide with the browser,
since their IDs come from completely independent sources?

Or is there still something I'm misunderstanding?

[Fady Samuel, 2017/01/26 20:15:22]

Good catch on Android, we don't AllocateFrameSinkIds in the browser correctly:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/context_pr...

In GpuProcessTransportFactory, we leave the client ID component 0 (renderers
have a RenderProcessHost ID > 0) and so there are no collisions.

https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...

Olivia, could you please fix this? Thanks!

[xlai (Olivia), 2017/01/26 20:50:21]

I just made a fix to ContextProviderFactory in Android so that the generation of
FrameSinkId will be exactly same as in non-Android platform. Basically, browser
process will have the 0 as its client ID; renderer processes get their client ID
starting from 1 so they will never collide.

+        base::checked_cast<uint32_t>(host_->GetRoutingID()));
     delegated_frame_host_.reset(new ui::DelegatedFrameHostAndroid(
-        &view_, cached_background_color_, this));
+        &view_, cached_background_color_, this, frame_sink_id));
   }
 
   host_->SetView(this);