Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation

Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation


BUG=685426
Review-Url: https://codereview.chromium.org/2656893003
Cr-Commit-Position: refs/heads/master@{#446720}
Committed: https://chromium.googlesource.com/chromium/src/+/9351829f20da199ef48bc3aaaa70f2172baab179

[xlai (Olivia), 2017-01-25 23:19:12]

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

[xlai (Olivia), 2017-01-25 23:19:27]

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

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

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

[Fady Samuel, 2017-01-25 23:23:48]

https://codereview.chromium.org/2656893003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/2656893003/diff/20001/content/browser/rendere...
content/browser/renderer_host/browser_compositor_view_mac.h:54: cc::FrameSinkId
frame_sink_id);
const cc::FrameSinkId&

https://codereview.chromium.org/2656893003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/20001/content/browser/rendere...
content/browser/renderer_host/browser_compositor_view_mac.mm:172:
cc::FrameSinkId frame_sink_id)
const cc::FrameSinkId&

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
ui/android/delegated_frame_host_android.cc:58: cc::FrameSinkId frame_sink_id)
const cc::FrameSinkId&

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
File ui/android/delegated_frame_host_android.h (right):

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
ui/android/delegated_frame_host_android.h:45: cc::FrameSinkId frame_sink_id);
const cc::FrameSinkId&

[Fady Samuel, 2017-01-25 23:24:01]

lgtm + nits

[xlai (Olivia), 2017-01-25 23:32:44]

Description was changed from

==========
Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation


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

to

==========
Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation


BUG=685426
==========

[xlai (Olivia), 2017-01-25 23:43:25]

xlai@chromium.org changed reviewers:
+ boliu@chromium.org, jam@chromium.org

[xlai (Olivia), 2017-01-25 23:43:26]

This is a fix which is similar to the code in render widget host view in Aura
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...).
Basically I applied the same code to other platforms (Android and Mac).

boliu@: Please review ui/android/.
jam@: Please review content/browser/.

Thanks!

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2656893003/diff/20001/ui/android/delegated_fr...
ui/android/delegated_frame_host_android.cc:58: cc::FrameSinkId frame_sink_id)
On 2017/01/25 23:23:48, Fady Samuel wrote:
> const cc::FrameSinkId&

Done here and elsewhere

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

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
these numbers are ok as the "client id" and "sink id"? maybe should document
what these IDs are somewhere and why this is ok?

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:471: // GuestViews
have two RenderWidgetHostViews and so we need to make sure
can you explain a bit how this works? will both RWHVs have is_guest_view_hack_
set to true?

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

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
On 2017/01/26 00:14:39, boliu wrote:
> these numbers are ok as the "client id" and "sink id"? maybe should document
> what these IDs are somewhere and why this is ok?

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.

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:471: // GuestViews
have two RenderWidgetHostViews and so we need to make sure
On 2017/01/26 00:14:39, boliu wrote:
> can you explain a bit how this works? will both RWHVs have is_guest_view_hack_
> set to true?

I copied this comment and the same piece of code from
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid....
is_guest_view_hack is usually false.

The code here is just make Mac platform to behave as same as non-Mac platform.
Otherwise the use of frame sink id in my another patch
https://codereview.chromium.org/2644653003/ is crashing on Mac platform only.

From what I know it is part of the project in MUS team to update frame sink id
for two different scenarios: when the view acts as platform view hack for a
RenderWidgetHostViewGuest, or when it is
not. fsamuel@: Do you mind explaining in more details to boliu@ on why this is
necessary?

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

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> On 2017/01/26 00:14:39, boliu wrote:
> > these numbers are ok as the "client id" and "sink id"? maybe should document
> > what these IDs are somewhere and why this is ok?
> 
> 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.

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.

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:471: // GuestViews
have two RenderWidgetHostViews and so we need to make sure
On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> On 2017/01/26 00:14:39, boliu wrote:
> > can you explain a bit how this works? will both RWHVs have
is_guest_view_hack_
> > set to true?
> 
> I copied this comment and the same piece of code from
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid....
> is_guest_view_hack is usually false.
> 
> The code here is just make Mac platform to behave as same as non-Mac platform.
> Otherwise the use of frame sink id in my another patch
> https://codereview.chromium.org/2644653003/ is crashing on Mac platform only.
> 
> From what I know it is part of the project in MUS team to update frame sink id
> for two different scenarios: when the view acts as platform view hack for a
> RenderWidgetHostViewGuest, or when it is
> not. fsamuel@: Do you mind explaining in more details to boliu@ on why this is
> necessary? 
> 
> 

guest_view_hack a flag used for the BrowserPlugin implementation of guestview.
Fortunately that code going away soon (within a quarter I guess) as the
GuestView team is transitioning to the new OOPIF architecture.

Android doesn't have <webview> in WebUI or Chrome Apps so this is a non-issue on
that platform.

[jam, 2017-01-26 19:04:26]

lgtm

[boliu, 2017-01-26 19:24:46]

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
On 2017/01/26 16:06:36, Fady Samuel wrote:
> On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> > On 2017/01/26 00:14:39, boliu wrote:
> > > these numbers are ok as the "client id" and "sink id"? maybe should
document
> > > what these IDs are somewhere and why this is ok?
> > 
> > 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.
> 
> 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.
> 

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?

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:471: // GuestViews
have two RenderWidgetHostViews and so we need to make sure
On 2017/01/26 16:06:36, Fady Samuel wrote:
> On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> > On 2017/01/26 00:14:39, boliu wrote:
> > > can you explain a bit how this works? will both RWHVs have
> is_guest_view_hack_
> > > set to true?
> > 
> > I copied this comment and the same piece of code from
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid....
> > is_guest_view_hack is usually false.
> > 
> > The code here is just make Mac platform to behave as same as non-Mac
platform.
> > Otherwise the use of frame sink id in my another patch
> > https://codereview.chromium.org/2644653003/ is crashing on Mac platform
only.
> > 
> > From what I know it is part of the project in MUS team to update frame sink
id
> > for two different scenarios: when the view acts as platform view hack for a
> > RenderWidgetHostViewGuest, or when it is
> > not. fsamuel@: Do you mind explaining in more details to boliu@ on why this
is
> > necessary? 
> > 
> > 
> 
> guest_view_hack a flag used for the BrowserPlugin implementation of guestview.
> Fortunately that code going away soon (within a quarter I guess) as the
> GuestView team is transitioning to the new OOPIF architecture.
> 
> Android doesn't have <webview> in WebUI or Chrome Apps so this is a non-issue
on
> that platform.

Same question here, how do we know AllocateFrameSinkId won't collide with
process/routing id?

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

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
On 2017/01/26 19:24:45, boliu wrote:
> On 2017/01/26 16:06:36, Fady Samuel wrote:
> > On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> > > On 2017/01/26 00:14:39, boliu wrote:
> > > > these numbers are ok as the "client id" and "sink id"? maybe should
> document
> > > > what these IDs are somewhere and why this is ok?
> > > 
> > > 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.
> > 
> > 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.
> > 
> 
> 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?

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:35:17]

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

[commit-bot: I haz the power, 2017-01-26 20:35:42]

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

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

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:453:
base::checked_cast<uint32_t>(host_->GetProcess()->GetID()),
On 2017/01/26 20:15:22, Fady Samuel wrote:
> On 2017/01/26 19:24:45, boliu wrote:
> > On 2017/01/26 16:06:36, Fady Samuel wrote:
> > > On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> > > > On 2017/01/26 00:14:39, boliu wrote:
> > > > > these numbers are ok as the "client id" and "sink id"? maybe should
> > document
> > > > > what these IDs are somewhere and why this is ok?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > > 
> > 
> > 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?
> 
> 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!

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.

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2656893003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:471: // GuestViews
have two RenderWidgetHostViews and so we need to make sure
On 2017/01/26 19:24:46, boliu wrote:
> On 2017/01/26 16:06:36, Fady Samuel wrote:
> > On 2017/01/26 15:56:11, xlai (Olivia) wrote:
> > > On 2017/01/26 00:14:39, boliu wrote:
> > > > can you explain a bit how this works? will both RWHVs have
> > is_guest_view_hack_
> > > > set to true?
> > > 
> > > I copied this comment and the same piece of code from
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid....
> > > is_guest_view_hack is usually false.
> > > 
> > > The code here is just make Mac platform to behave as same as non-Mac
> platform.
> > > Otherwise the use of frame sink id in my another patch
> > > https://codereview.chromium.org/2644653003/ is crashing on Mac platform
> only.
> > > 
> > > From what I know it is part of the project in MUS team to update frame
sink
> id
> > > for two different scenarios: when the view acts as platform view hack for
a
> > > RenderWidgetHostViewGuest, or when it is
> > > not. fsamuel@: Do you mind explaining in more details to boliu@ on why
this
> is
> > > necessary? 
> > > 
> > > 
> > 
> > guest_view_hack a flag used for the BrowserPlugin implementation of
guestview.
> > Fortunately that code going away soon (within a quarter I guess) as the
> > GuestView team is transitioning to the new OOPIF architecture.
> > 
> > Android doesn't have <webview> in WebUI or Chrome Apps so this is a
non-issue
> on
> > that platform.
> 
> Same question here, how do we know AllocateFrameSinkId won't collide with
> process/routing id?

Mac is using GPUProcessTransportFactory so it's same as Aura: 0 is the browser
ID and renderer processes have non-zero IDs so there will not be conflicts.

[Fady Samuel, 2017-01-26 20:52:20]

https://codereview.chromium.org/2656893003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2656893003/diff/60001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:139:
return cc::FrameSinkId(0 /* client_id */, ++next_sink_id_);
next_sink_id_++ to match other platforms.

This skips 1

[xlai (Olivia), 2017-01-26 20:59:44]

https://codereview.chromium.org/2656893003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2656893003/diff/60001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:139:
return cc::FrameSinkId(0 /* client_id */, ++next_sink_id_);
On 2017/01/26 20:52:20, Fady Samuel wrote:
> next_sink_id_++ to match other platforms.
> 
> This skips 1

Done.

[xlai (Olivia), 2017-01-26 21:00:14]

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

[commit-bot: I haz the power, 2017-01-26 21:00:49]

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

[boliu, 2017-01-26 21:04:48]

What about InProcessContextFactory::AllocateFrameSinkId? Anything uses that? Do
we need to audit all the places FrameSinkIds are created?

I'm concerned about future maintenance of this. There's this hidden property
that allocations need to be unique globally, but allocations are all over the
place. What's preventing someone accidentally changing one of those allocations
in the future and introduce a possibility of collision? That would be a really
hard to catch bug.

Maybe we should move all allocations to a single source file, with comments
explaining why there are no collisions globally?

[Fady Samuel, 2017-01-26 21:09:14]

On 2017/01/26 21:04:48, boliu wrote:
> What about InProcessContextFactory::AllocateFrameSinkId? Anything uses that?
Do
> we need to audit all the places FrameSinkIds are created?
> 
> I'm concerned about future maintenance of this. There's this hidden property
> that allocations need to be unique globally, but allocations are all over the
> place. What's preventing someone accidentally changing one of those
allocations
> in the future and introduce a possibility of collision? That would be a really
> hard to catch bug.
> 
> Maybe we should move all allocations to a single source file, with comments
> explaining why there are no collisions globally?

+1 InProcessContextFactory::AllocateFrameSinkId needs to be updated too. +1 as
well to there being a central AllocateFrameSinkId somewhere.

Note that eventually the Mus window server will be that place, but that will
take a while to make its way onto all platforms so I support this.

[xlai (Olivia), 2017-01-26 21:45:07]

Patchset #6 (id:100001) has been deleted

[xlai (Olivia), 2017-01-26 21:47:12]

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

[commit-bot: I haz the power, 2017-01-26 21:47:42]

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

[xlai (Olivia), 2017-01-26 21:55:16]

I've fixed InProcessContextFactory::AllocateFrameSinkId as well. 

The current allocation of frame_sink_id being spread all the places already
exist and is not introduced by this CL; also, the refactoring effort needs
cautious planning that involve all platforms and is not trivial. In a discussion
offline with boliu@, we agree to create a crbug.com/685748 to keep track the
need to refactor frame sink id allocation to make it centralized in one single
place. I added TODO comment in everywhere that I could
think of, including Aura, Mac, Android platforms.

[boliu, 2017-01-26 22:52:41]

On 2017/01/26 21:09:14, Fady Samuel wrote:
> Note that eventually the Mus window server will be that place, but that will
> take a while to make its way onto all platforms so I support this.

Fady, how far away is that realistically?

It's probably really hard to put all allocations in a central place due to all
the different layering requirements. Maybe comments is good enough

https://codereview.chromium.org/2656893003/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2656893003/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:693: //
TODO(crbug.com/685777): Centralize allocation in one place.
I was expecting something like

Must be unique with RenderWidgetHostViewFoo FrameSinkId allocations.

And then in all the RWHV places, add that it must be unique with this, etc etc.

[commit-bot: I haz the power, 2017-01-26 23:28:12]

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

[commit-bot: I haz the power, 2017-01-26 23:28:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Fady Samuel, 2017-01-27 01:09:55]

On 2017/01/26 22:52:41, boliu wrote:
> On 2017/01/26 21:09:14, Fady Samuel wrote:
> > Note that eventually the Mus window server will be that place, but that will
> > take a while to make its way onto all platforms so I support this.
> 
> Fady, how far away is that realistically?
> 
> It's probably really hard to put all allocations in a central place due to all
> the different layering requirements. Maybe comments is good enough
> 
>
https://codereview.chromium.org/2656893003/diff/120001/content/browser/compos...
> File content/browser/compositor/gpu_process_transport_factory.cc (right):
> 
>
https://codereview.chromium.org/2656893003/diff/120001/content/browser/compos...
> content/browser/compositor/gpu_process_transport_factory.cc:693: //
> TODO(crbug.com/685777): Centralize allocation in one place.
> I was expecting something like
> 
> Must be unique with RenderWidgetHostViewFoo FrameSinkId allocations.
> 
> And then in all the RWHV places, add that it must be unique with this, etc
etc.

The Mus window server is a couple of quarters away at least from completion.
Probably longer for platforms other than CrOS.

[boliu, 2017-01-27 02:24:34]

Please make the comment refer to other code that it needs to be globally unique
with, rather than refer to an unassigned bug. Then I'll approve

[xlai (Olivia), 2017-01-27 16:32:16]

boliu@: I have updated the comments in the context factories where
FrameSinkId is allocated, to refer to its corresponding RenderWidgetHostFoo's
frame sink id generation, in addition to the
TODO issue.

[xlai (Olivia), 2017-01-27 16:46:19]

boliu@: I have updated the comments in the context factories where
FrameSinkId is allocated, to refer to its corresponding RenderWidgetHostFoo's
frame sink id generation. 
Also, I've added back references in comments in all the RenderWidgetHostFoo. 
All these are in addition to the TODO issue.

[xlai (Olivia), 2017-01-27 16:55:40]

Patchset #7 (id:140001) has been deleted

[bo658713, 2017-01-27 16:59:35]

lgtm


On Jan 27, 2017 08:46, <xlai@chromium.org> wrote:

> boliu@: I have updated the comments in the context factories where
> FrameSinkId is allocated, to refer to its corresponding
> RenderWidgetHostFoo's
> frame sink id generation.
> Also, I've added back references in comments in all the
> RenderWidgetHostFoo.
> All these are in addition to the TODO issue.
>
>
>
> https://codereview.chromium.org/2656893003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xlai (Olivia), 2017-01-27 17:09:41]

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

[commit-bot: I haz the power, 2017-01-27 17:09:56]

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

[boliu, 2017-01-27 17:49:46]

lgtm

Er.. sorry about that

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

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

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

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2017-01-27 18:30:18]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-01-27 18:30:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2656893003/#ps160001
(title: "more comments")

[commit-bot: I haz the power, 2017-01-27 18:30:49]

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-27 18:39:18]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485541818031940,
"parent_rev": "2dcdb5ae7b15a7e6aaa6745c2065202982a17220", "commit_rev":
"9351829f20da199ef48bc3aaaa70f2172baab179"}

[commit-bot: I haz the power, 2017-01-27 18:39:54]

Description was changed from

==========
Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation


BUG=685426
==========

to

==========
Let DelegateFrameHost in Mac and Android use correct FrameSinkId during creation


BUG=685426

Review-Url: https://codereview.chromium.org/2656893003
Cr-Commit-Position: refs/heads/master@{#446720}
Committed:
https://chromium.googlesource.com/chromium/src/+/9351829f20da199ef48bc3aaaa70...
==========

[commit-bot: I haz the power, 2017-01-27 18:39:55]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9351829f20da199ef48bc3aaaa70...