Refactor how surfaces are acquired for GPU compositing.

Refactor how surfaces are acquired for GPU compositing.

Rather than have a couple of nasty #if defined blocks, push the code down into the platform specific RenderWidgetHostView layer.  Added support for releasing surfaces:

- if renderer crashes, RenderWidgetHostView releases the surfaces (no change from previous code)

- if gpu process crashes, the GpuProcessHostUIShim releases the surfaces (introduced in this patch)

- if the channel between the renderer and gpu process falls, the gpu process asks the GpuProcessHostUIShim to release the surface

BUG=none
TEST=by hand 3D css and WebGL on win, max, linux, and linux views

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75387

[jonathan.backer, 2011-02-09 17:29:15]

apatrick: I don't know if this is how we want to handle resources that the
browser allocates. If so, we'll need something similar for the SHM.

kbr, nduca: Just keeping you in the loop about how the code path changed on
MacOSX and Windows.

anicolao: This should make compositing sort of work on Views (there's a nasty
offset). AcceleratedCompositingActivated is still NOTIMPLEMENTED. Because of the
log warning produced by the NOTIMPLEMENTED, I'm OK with a buggy implementation,
but I can just stub out AcquireCompositingSurface if you'd prefer. I think that
we'll need Baklava to get a proper implementation.

[apatrick_chromium, 2011-02-10 19:11:17]

I think this should be hand tested on all three platforms.

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:215: scoped_refptr<ViewSurface>
view_surface(new ViewSurface(view_id));
I can't see where reference counting is needed. Can this be linked_ptr?

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:221: acquired_surfaces_[view_id] =
view_surface;
Is it safe to assume that there will be only one request for a view command
buffer for a particular view? What if the renderer is compromised? Maybe this
function should fail or reuse the existing view surface in this case.

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:471: host =
static_cast<RenderWidgetHost*>(
If the browser process does not trust the renderer process, the render_view_id
might not correspond to a RenderWidgetHost and this static cast could be invalid
and lead to a browser process crash.

[jonathan.backer, 2011-02-11 19:42:03]

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
File chrome/browser/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:215: scoped_refptr<ViewSurface>
view_surface(new ViewSurface(view_id));
On 2011/02/10 19:11:19, apatrick_chromium wrote:
> I can't see where reference counting is needed. Can this be linked_ptr?

Done. I ref count to add to the map.

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:221: acquired_surfaces_[view_id] =
view_surface;
On 2011/02/10 19:11:19, apatrick_chromium wrote:
> Is it safe to assume that there will be only one request for a view command
> buffer for a particular view? What if the renderer is compromised? Maybe this
> function should fail or reuse the existing view surface in this case.

To my knowledge, only the compositor creates a View command buffer. So we can
safely assume only one.  I'm gonna consider a second request an error condition
and fail to create one.

http://codereview.chromium.org/6462034/diff/2001/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:471: host =
static_cast<RenderWidgetHost*>(
On 2011/02/10 19:11:19, apatrick_chromium wrote:
> If the browser process does not trust the renderer process, the render_view_id
> might not correspond to a RenderWidgetHost and this static cast could be
invalid
> and lead to a browser process crash.

I'm not disagreeing with you, but this is done elsewhere in the codebase.

This code is a derivative of the code in RenderViewHost::FromID. An initial
version called RenderViewHost::FromID, but this caused a regression with
Antoine's Pepper 3D work, so Antoine changed it to this
(http://codereview.chromium.org/6258023/).

[apatrick_chromium, 2011-02-17 19:52:32]

LGTM