Vend common GL context

Vend common GL context

We can't rely on having a compositor for set-up and tear-down of cross process texture transport. This CL creates a shared offscreen context appropriate for that. It further separates TextureGL objects from CompositorGL objects. In particular
- I've plumbed through the size of the surface backing the Compositor to the Texture
- I've allows the TextureGL to release it's GL resources using the shared context

It's necessary to make the vendor of the offscreen context a LeakySingletonTrait singleton so that we don't race with destruction of GL bindings on process shutdown.

BUG=fixes dereferencing NULL pointers in RWHVVTouch when images come from GPU process
TEST=3D CSS on TOUCH_UI, views_desktop on Windows

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

[sky, 2011-08-24 21:17:08]

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:54: const gfx::Size& surface_size) = 0;
Move this into TextureDrawParams. I think the name surface_size is a bit
ambiguous here. Would be better to name it compositor_size. Alternatively maybe
you should pass in the Compositor (in TextureDRawParams) so that implementations
can query whatever they want from the compositor.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:96: virtual void OnWidgetSizeChanged(const
gfx::Size& size) = 0;
Rename this to WidgetSizeChanged, make it concrete, have it set size_ and invoke
the pure virtual method OnWidgetSizeChanged.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:105: void SetSize(const gfx::Size& size) { size_
= size; }
Is this needed?

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_win.cc (left):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_win.cc:391: return last_size_;
I don't think you wired up changing the size correctly for this class.

[jonathan.backer, 2011-08-24 22:03:02]

+kbr@

[Ken Russell (switch to Gerrit), 2011-08-25 04:42:17]

The refactoring of the shared resources LGTM; one comment.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_gl.cc:203: // We use LeakSingletonTraits so that we
don't race with
Leak -> Leaky

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_gl.cc:217: return true;
If you had threading problems in the initialization of the GL bindings, do you
have them here too?

[jonathan.backer, 2011-08-25 18:57:18]

Tested on views_desktop for linux. Looking for green on linux_views to make sure
that I didn't regress tear down.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:54: const gfx::Size& surface_size) = 0;
On 2011/08/24 21:17:08, sky wrote:
> Move this into TextureDrawParams. I think the name surface_size is a bit
> ambiguous here. Would be better to name it compositor_size. Alternatively
maybe
> you should pass in the Compositor (in TextureDRawParams) so that
implementations
> can query whatever they want from the compositor.

Done.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:96: virtual void OnWidgetSizeChanged(const
gfx::Size& size) = 0;
On 2011/08/24 21:17:08, sky wrote:
> Rename this to WidgetSizeChanged, make it concrete, have it set size_ and
invoke
> the pure virtual method OnWidgetSizeChanged.

Done.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:105: void SetSize(const gfx::Size& size) { size_
= size; }
On 2011/08/24 21:17:08, sky wrote:
> Is this needed?

Done.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_gl.cc:217: return true;
Yup. This is the origin of it, as commented in the other CL. The other
initialization is in GpuInfoCollect.

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_win.cc (left):

http://codereview.chromium.org/7552039/diff/4001/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_win.cc:391: return last_size_;
On 2011/08/24 21:17:08, sky wrote:
> I don't think you wired up changing the size correctly for this class.

Done.

[jonathan.backer, 2011-08-25 20:57:33]

Errrr. On views_desktop for Windows.

[sky, 2011-08-25 21:15:55]

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.h:24: TextureDrawParams() : transform(),
blend(false), compositor_size(1,1) {}
Why the 1x1 here?

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:58: 
remove this, and 60. See the Compositor: section below where we generally don't
have whitespace for overriden methods.

[jonathan.backer, 2011-08-26 11:45:55]

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor.h:24: TextureDrawParams() : transform(),
blend(false), compositor_size(1,1) {}
On 2011/08/25 21:15:55, sky wrote:
> Why the 1x1 here?

I thought that it would be less likely to trigger a crash (e.g. 0,0 could cause
a division by zero). I'm happy to either (a) leave as is, (b) use default
constructor, or (c) make this a class w/ setters and getters and force
compositor_size to be given at instantiation. My preference is (a) and (c) ---
in that order --- mostly because this is a thin class and I'd like to keep it
that way.

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7552039/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:58: 
On 2011/08/25 21:15:55, sky wrote:
> remove this, and 60. See the Compositor: section below where we generally
don't
> have whitespace for overriden methods.

Done.

[jonathan.backer, 2011-08-26 15:43:23]

Updated as per chat with sky@.

[sky, 2011-08-26 15:44:15]

LGTM