Refactor how surfaces are acquired for GPU compositing.

chrome/browser/gpu_process_host_ui_shim.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/gpu_process_host_ui_shim.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/gpu_blacklist.h"
@@ skipping 193 matching lines @@
   }
 }
 
 void GpuProcessHostUIShim::CreateViewCommandBuffer(
     int32 render_view_id,
     int32 renderer_id,
     const GPUCreateCommandBufferConfig& init_params,
     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);
   }
 }
 
 void GpuProcessHostUIShim::CollectGraphicsInfoAsynchronously(
     GPUInfo::Level level) {
   DCHECK(CalledOnValidThread());
   Send(new GpuMsg_CollectGraphicsInfo(level));
 }
@@ skipping 100 matching lines @@
     if (route_id == MSG_ROUTING_NONE)
       CreateCommandBufferError(callback.release(), route_id);
     else
       callback->Run(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) {
   gpu_info_ = gpu_info;
   child_process_logging::SetGpuInfo(gpu_info);
 
   // Used only in testing.
   if (gpu_info_collected_callback_.get())
     gpu_info_collected_callback_->Run();
 }
@@ skipping 81 matching lines @@
           IDR_GPU_BLACKLIST));
   gpu_blacklist_.reset(new GpuBlacklist());
   const CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
   if (browser_command_line.HasSwitch(switches::kIgnoreGpuBlacklist) ||
       gpu_blacklist_->LoadGpuBlacklist(gpu_blacklist_json.as_string(), true)) {
     return true;
   }
   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;
+}