Progress towards fixing 77536

Progress towards fixing 77536

This first thing this CL does is plumbs through the surface that the GPU will eventually render to very early on (with the ViewMsg_New and ViewMsg_CreateNew IPCs). Previously, these surfaces were acquired at the time that GPU got a request for a command buffer. This required mediation by the browser UI thread. With this CL, we pre-acquire the surfaces so that they can be passed with the request for the command buffer. This will allow us to handle the request on the browser IO thread.

The second thing this CL does is change the surface management a bit. When surfaces were acquired and released when command buffers were created and destroyed, the GPU process host was natural manager of surfaces. This pushes the management further down into the RWHV level and GtkNativeViewManager (on Linux). It fixes a minor resource leak.

BUG=first stage of 77536
TEST=By hand WebGL, 3D CSS, Pepper 3D on Linux, Windows, and Mac.

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

[jonathan.backer, 2011-04-14 21:21:44]

I've got a few small TODOs and FIXMEs. I'll clean them up tomorrow morning.
Otherwise, I think this is pretty good. I've already tested by hand on Linux and
Mac. I'll run it by the trybots after a final nit picking session.

jam: Can you make sure that what I did with the IPC and storing of the
compositing_window in the render_widget makes sense?

apatrick: Please take a look at GpuProcessHostUIShim and the changes (pretty
minor) to the RWHVs.

piman: Can you verify that I didn't regress Pepper. I made a few changes to
GtkNativeViewManager (which you have reviewed in the past).

[apatrick_chromium, 2011-04-14 21:58:53]

LGTM for UIShim and RWHVs.

[jam, 2011-04-14 22:42:00]

lgtm with the second comment, and your TODOs in the code taken care of. thanks
for fixing this.

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

http://codereview.chromium.org/6840060/diff/1021/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:119: class
GpuProcessHostUIShim::SurfaceLock {
nit: lock implies that locking is taking place, where this looks like a
reference.  How about SurfaceReference?

http://codereview.chromium.org/6840060/diff/1021/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:130:
DCHECK(manager->AddRefPermanentXID(surface_));
DCHECK is compiled away in release builds, so you'll either want to use CHECK,
or get the rv and DCHECK it

[jonathan.backer, 2011-04-15 19:02:51]

Just waiting for piman@ to try this with pepper flash plugin before I upstream
this. Working on moving IPC handling from UI to IO thread in a separate CL.

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

http://codereview.chromium.org/6840060/diff/1021/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:119: class
GpuProcessHostUIShim::SurfaceLock {
On 2011/04/14 22:42:00, John Abd-El-Malek wrote:
> nit: lock implies that locking is taking place, where this looks like a
> reference.  How about SurfaceReference?

Done.

http://codereview.chromium.org/6840060/diff/1021/chrome/browser/gpu_process_h...
chrome/browser/gpu_process_host_ui_shim.cc:130:
DCHECK(manager->AddRefPermanentXID(surface_));
On 2011/04/14 22:42:00, John Abd-El-Malek wrote:
> DCHECK is compiled away in release builds, so you'll either want to use CHECK,
> or get the rv and DCHECK it

Good catch! Done.

[piman, 2011-04-15 21:53:33]

http://codereview.chromium.org/6840060/diff/7001/content/renderer/render_widg...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/6840060/diff/7001/content/renderer/render_widg...
content/renderer/render_widget_fullscreen_pepper.cc:304: DCHECK(!context_);
This can in theory be called before we have the compositing_surface, in which
case returning early is the right thing to do. The plugin's context creation
will fail but that's expected.

[vsevik, 2011-04-20 12:50:59]

This patch breaks Chrome DevTools.
If you try to press Ctrl-Shift-I several times you can see that sometimes
DevTools fills the screen with gray box.
Trying to drag the border of this box makes it normal though.

[jonathan.backer, 2011-04-20 20:09:44]

On 2011/04/20 12:50:59, vsevik wrote:
> This patch breaks Chrome DevTools.
> If you try to press Ctrl-Shift-I several times you can see that sometimes
> DevTools fills the screen with gray box.
> Trying to drag the border of this box makes it normal though.

Filed a bug: http://crbug.com/80017