Refactor how surfaces are acquired for GPU compositing.

chrome/browser/gpu_process_host_ui_shim.cc

Index: chrome/browser/gpu_process_host_ui_shim.cc
diff --git a/chrome/browser/gpu_process_host_ui_shim.cc b/chrome/browser/gpu_process_host_ui_shim.cc
index 04b535c6eb4be328ff595f8ae971aa0ec397f6a7..57ffcc7c4bd6b5169006b722ac51ac575ac3d4aa 100644
--- a/chrome/browser/gpu_process_host_ui_shim.cc
+++ b/chrome/browser/gpu_process_host_ui_shim.cc
@@ -211,44 +211,14 @@ void GpuProcessHostUIShim::CreateViewCommandBuffer(
     CreateCommandBufferCallback* callback) {
   DCHECK(CalledOnValidThread());
   linked_ptr<CreateCommandBufferCallback> wrapped_callback(callback);
+  ViewID view_id(renderer_id, render_view_id);
+  scoped_refptr<ViewSurface> view_surface(new ViewSurface(view_id));

[apatrick_chromium, 2011/02/10 19:11:19]

I can't see where reference counting is needed. Can this be linked_ptr?

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

Done. I ref count to add to the map.

 
-  gfx::PluginWindowHandle window = gfx::kNullPluginWindow;
-  RenderProcessHost* process = RenderProcessHost::FromID(renderer_id);
-  RenderWidgetHost* host = NULL;
-  if (process) {
-    host = static_cast<RenderWidgetHost*>(
-        process->GetListenerByID(render_view_id));
-  }
-
-  RenderWidgetHostView* view = NULL;
-  if (host)
-    view = host->view();
-
-  if (view) {
-#if defined(OS_LINUX)
-    gfx::NativeViewId view_id = NULL;
-    view_id = gfx::IdFromNativeView(view->GetNativeView());
-
-    // Lock the window that we will draw into.
-    GtkNativeViewManager* manager = GtkNativeViewManager::GetInstance();
-    if (!manager->GetPermanentXIDForId(&window, view_id)) {
-      DLOG(ERROR) << "Can't find XID for view id " << view_id;
-    }
-#elif defined(OS_MACOSX)
-    // On Mac OS X we currently pass a (fake) PluginWindowHandle for the
-    // window that we draw to.
-    window = view->AllocateFakePluginWindowHandle(
-        /*opaque=*/true, /*root=*/true);
-#elif defined(OS_WIN)
-    // Create a window that we will overlay.
-    window = view->GetCompositorHostWindow();
-#endif
-  }
-
-  if (window != gfx::kNullPluginWindow &&
+  if (view_surface->surface() != gfx::kNullPluginWindow &&
       Send(new GpuMsg_CreateViewCommandBuffer(
-          window, render_view_id, renderer_id, init_params))) {
+          view_surface->surface(), render_view_id, renderer_id, init_params))) {
     create_command_buffer_requests_.push(wrapped_callback);
+    acquired_surfaces_[view_id] = view_surface;

[apatrick_chromium, 2011/02/10 19:11:19]

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.

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

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.

   } else {
     CreateCommandBufferError(wrapped_callback.release(), MSG_ROUTING_NONE);
   }
@@ -369,31 +339,8 @@ void GpuProcessHostUIShim::OnCommandBufferCreated(const int32 route_id) {
 void GpuProcessHostUIShim::OnDestroyCommandBuffer(
     gfx::PluginWindowHandle window, int32 renderer_id,
     int32 render_view_id) {
-  if (!window)
-    return;
-
-#if defined(OS_LINUX)
-  GtkNativeViewManager* manager = GtkNativeViewManager::GetInstance();
-  manager->ReleasePermanentXID(window);
-#elif defined(OS_MACOSX) || defined(OS_WIN)
-  RenderProcessHost* process = RenderProcessHost::FromID(renderer_id);
-  RenderWidgetHost* host = NULL;
-  if (process) {
-    host = static_cast<RenderWidgetHost*>(
-        process->GetListenerByID(render_view_id));
-  }
-  RenderWidgetHostView* view = NULL;
-  if (host)
-    view = host->view();
-
-  if (view) {
-#if defined(OS_MACOSX)
-    view->DestroyFakePluginWindowHandle(window);
-#elif defined(OS_WIN)
-    view->ShowCompositorHostWindow(false);
-#endif
-  }
-#endif  // defined(OS_MACOSX) || defined(OS_WIN)
+  ViewID view_id(renderer_id, render_view_id);
+  acquired_surfaces_.erase(view_id);
 }
 
 void GpuProcessHostUIShim::OnGraphicsInfoCollected(const GPUInfo& gpu_info) {
@@ -495,3 +442,39 @@ bool GpuProcessHostUIShim::LoadGpuBlacklist() {
   gpu_blacklist_.reset(NULL);
   return false;
 }
+
+GpuProcessHostUIShim::ViewSurface::ViewSurface(ViewID view_id)
+    : view_id_(view_id), surface_(gfx::kNullPluginWindow) {
+  RenderWidgetHostView* view = GetRenderWidgetHostView();
+  if (view)
+    surface_ = view->AcquireCompositingSurface();
+}
+
+GpuProcessHostUIShim::ViewSurface::~ViewSurface() {
+  if (!surface_)
+    return;
+
+  RenderWidgetHostView* view = GetRenderWidgetHostView();
+  if (view)
+    view->ReleaseCompositingSurface(surface_);
+}
+
+// We do separate lookups for the RenderWidgetHostView when acquiring
+// and releasing surfaces (rather than caching) because the
+// RenderWidgetHostView could die without warning. In such a case,
+// it's the RenderWidgetHostView's responsibility to cleanup.
+RenderWidgetHostView* GpuProcessHostUIShim::ViewSurface::
+    GetRenderWidgetHostView() {
+  RenderProcessHost* process = RenderProcessHost::FromID(view_id_.first);
+  RenderWidgetHost* host = NULL;
+  if (process) {
+    host = static_cast<RenderWidgetHost*>(

[apatrick_chromium, 2011/02/10 19:11:19]

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]

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

+        process->GetListenerByID(view_id_.second));
+  }
+
+  RenderWidgetHostView* view = NULL;
+  if (host)
+    view = host->view();
+
+  return view;
+}