New issue
Advanced search Search tips

Issue 843694 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Move parent LocalSurfaceId allocation to RenderWidgetHostImpl::SynchronizeVisualProperties

Project Member Reported by fsam...@chromium.org, May 16 2018

Issue description

In RenderFrameProxy::SynchronizeVisualProperties we allocate a new LocalSurfaceId if one of the visual properties has changed.

In RenderWidgetHostImpl::SynchronizeVisualProperties we grab the visual properties AND the LocalSurfaceId from the RenderWidgetHostView and hope the invariant "Change In Properties => new LocalSurfaceId" holds.

This seems bad. We should just allocate LocalSurfaceIds in RenderWidgetHostImpl::SynchronizeVisualProperties and move the LocalSurfaceId OUT OF VisualProperties struct.

The current blocker for this is Mus: aura::Window likes to do its own LocalSurfaceId allocations in order to propagate to children. Maybe we should move LocalSurfaceIds entirely out of aura and Mus?


 
Components: Internals>Services>Viz
Also seems pretty fragile to me that the id allocator lives far away from RWHI, which manages sending the updated id to the child. It can lead to subtle bugs where each id update doesn't get pushed to the renderer.

Came up here: https://chromium-review.googlesource.com/c/chromium/src/+/1055626/5/ui/android/delegated_frame_host_android.cc#396
Scott: I'd like to move LocalSurfaceIds entirely out of aura::Window, Mus and so on. I think it should be up to clients to manually update LocalSurfaceIds on layers or to update their LocalSurfaceIds from the parent.

The window service doesn't need to know about LocalSurfaceId at all.

Comment 3 by sky@chromium.org, May 17 2018

I'm all for simpler. If this won't break anything, go for it.
Cc: piman@chromium.org danakj@chromium.org
Owner: jonr...@chromium.org
Status: Assigned (was: Untriaged)
Labels: -Pri-3 Proj-Mash-SingleProcess Pri-2
Sadrul pointed out that Exo will need to have LocalSurfaceId allocation code though because it currently relies on Aura to do the work for it.
Fady, is this needed for single-process-mash?
Labels: -Proj-Mash-SingleProcess Proj-Mash-MultiProcess
I'm assuming this isn't needed for single-process-mash. Jon and/or Fady, please add back single-process-mash if it is necessary.

Sign in to add a comment