cc: Route offscreen context creation for compositor to the browser.

cc: Route offscreen context creation for compositor to the browser.

Currently the compositor asks WebKit for the SharedGraphicsContext. For the
browser compositor, we instead route requests for an offscreen context to
ui/compositor, where we are able to create the context.

This patch only addresses offscreen contexts for the browser compositor. The
renderer compositor still gets its contexts from WebKit, but now via the
LayerTreeHostClient interface instead of directly going to the
WebSharedGraphicsContext3D.

Tested by the lost context unit tests. They now test that on context loss, the 
shared context is also recreated, and that if it fails to be recreated that we 
retry context creation.

BUG=169373, 175383
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184165

[danakj, 2013-02-05 07:07:57]

This is what I think we can make context creation look like for the browser
compositor.

The renderer compositor would go through LTHClient, and WebKit through
PlatforSupport, to RenderWidget (or RenderWidgetCompositor), which would simply
use ui::ContextFactory as well.

[danakj, 2013-02-05 07:09:57]

One thing I don't get is why we have a DefaultContextFactory right now. When
IsTestCompositorEnabled = true, then ui::Compositor doesn't use the
ContextFactory to make an output surface at all.

[piman, 2013-02-05 07:39:13]

On Mon, Feb 4, 2013 at 11:09 PM, <danakj@chromium.org> wrote:

> One thing I don't get is why we have a DefaultContextFactory right now.
> When
> IsTestCompositorEnabled = true, then ui::Compositor doesn't use the
> ContextFactory to make an output surface at all.
>

Some tests (aura tests, views tests, aura_bench, some ash tests iirc) don't
link in content (i.e. no GPU process infrastructure), but want a real
compositor. DefaultContextFactory provides an in-process implementation.


>
>
https://codereview.chromium.**org/12212007/<https://codereview.chromium.org/1...
>

[danakj, 2013-02-05 18:17:56]

On Tue, Feb 5, 2013 at 2:38 AM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Feb 4, 2013 at 11:09 PM, <danakj@chromium.org> wrote:
>
>> One thing I don't get is why we have a DefaultContextFactory right now.
>> When
>> IsTestCompositorEnabled = true, then ui::Compositor doesn't use the
>> ContextFactory to make an output surface at all.
>>
>
> Some tests (aura tests, views tests, aura_bench, some ash tests iirc)
> don't link in content (i.e. no GPU process infrastructure), but want a real
> compositor. DefaultContextFactory provides an in-process implementation.
>

I see, thanks.

I'll keep working on cleaning this up and skipping the WGC3D for the IPC
callbacks with the gpu mem mgr. But would not mind feedback if I'm on the
right track or barking up a wrong tree.

[piman, 2013-02-05 22:42:48]

Overall I like the approach.

About the renderer, what's the plan exactly? I don't think it's kosher to use
ui/compositor (that defines ContextFactory) in there.

https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h#new...
cc/resource_provider.h:68: WebKit::WebGraphicsContext3D*
offscreenGraphicsContext3d() { return m_offscreenContext3d; }
I don't think it belongs here, since it has nothing to do with resources. Could
we put it on the GLRenderer?

https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:150: {
nit: { on previous line

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:660: (*context3d) =
make_scoped_ptr(CreateOffscreenContext());
nit: context3d->reset(CreateOffscreenContext()) is shorter...

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:676:
reinterpret_cast<GrPlatform3DContext>(interface.get())));
/cry on the reinterpret_cast

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:679:
*gr_context_memory_manager = make_scoped_ptr(new GrContextMemoryManager(
nit: can also use reset(...) here

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:723:
scoped_ptr<WebKit::WebGraphicsContext3D> offscreen_skia_context_main_thread;
Could we reuse the shared_context_ for the main thread? They're costly, it's
worth saving.

[danakj, 2013-02-05 22:56:00]

Thanks! For the renderer, the LTHClient will become RenderWidget (or maybe
RenderWidgetCompositor?) which jamesr@ is working on.

Then that thing can call ui::ContextFactory and own a Context3d/GrContext pair.
And it will provide them to WebKit via PlatformSupport.

https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h#new...
cc/resource_provider.h:68: WebKit::WebGraphicsContext3D*
offscreenGraphicsContext3d() { return m_offscreenContext3d; }
On 2013/02/05 22:42:48, piman wrote:
> I don't think it belongs here, since it has nothing to do with resources.
Could
> we put it on the GLRenderer?

Oh, ya ok I'll give that a shot. The one problem I was trying to dodge is that
it has to go thru the Renderer class API which includes software mode. So then
passing a WGC3D thru there is weird.. ideas about that?

https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:150: {
On 2013/02/05 22:42:48, piman wrote:
> nit: { on previous line

Oh gosh this file has become a frankenstyle. :X

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:660: (*context3d) =
make_scoped_ptr(CreateOffscreenContext());
On 2013/02/05 22:42:48, piman wrote:
> nit: context3d->reset(CreateOffscreenContext()) is shorter...

Done.

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:676:
reinterpret_cast<GrPlatform3DContext>(interface.get())));
On 2013/02/05 22:42:48, piman wrote:
> /cry on the reinterpret_cast

Ya... more incoming as well :X

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:679:
*gr_context_memory_manager = make_scoped_ptr(new GrContextMemoryManager(
On 2013/02/05 22:42:48, piman wrote:
> nit: can also use reset(...) here

Done.

https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:723:
scoped_ptr<WebKit::WebGraphicsContext3D> offscreen_skia_context_main_thread;
On 2013/02/05 22:42:48, piman wrote:
> Could we reuse the shared_context_ for the main thread? They're costly, it's
> worth saving.

I don't know, it looks like something that is created once per compositor, not
once per process? I didn't understand what it's purpose was. Can you enlighten?

[piman, 2013-02-05 23:19:49]

On Tue, Feb 5, 2013 at 2:56 PM, <danakj@chromium.org> wrote:

> Thanks! For the renderer, the LTHClient will become RenderWidget (or maybe
> RenderWidgetCompositor?) which jamesr@ is working on.
>
> Then that thing can call ui::ContextFactory and own a Context3d/GrContext
> pair.
> And it will provide them to WebKit via PlatformSupport.


I still insist, there's no ui::ContextFactory in the renderer :)

You probably want to go to the RenderThreadImpl which is the singleton
class for the renderer.


> https://codereview.chromium.**org/12212007/diff/1001/cc/**
>
resource_provider.h<https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h>
> File cc/resource_provider.h (right):
>
> https://codereview.chromium.**org/12212007/diff/1001/cc/**
>
resource_provider.h#newcode68<https://codereview.chromium.org/12212007/diff/1001/cc/resource_provider.h#newcode68>
> cc/resource_provider.h:68: WebKit::WebGraphicsContext3D*
> offscreenGraphicsContext3d() { return m_offscreenContext3d; }
> On 2013/02/05 22:42:48, piman wrote:
>
>> I don't think it belongs here, since it has nothing to do with
>>
> resources. Could
>
>> we put it on the GLRenderer?
>>
>
> Oh, ya ok I'll give that a shot. The one problem I was trying to dodge
> is that it has to go thru the Renderer class API which includes software
> mode. So then passing a WGC3D thru there is weird.. ideas about that?


I don't have a great idea. There is a bit of awkwardness in that we will
try to create the filter context even in software or delegated mode, and
pass them through the system (proxies, LTH*) - not new to this patch. So
adding it to the Renderer API is not much worse compared to that.
To fix that I think we'd need to abstract the renderer type on the main
thread. I don't know how much work that would be...


 https://codereview.chromium.**org/12212007/diff/1001/cc/**
>
single_thread_proxy.cc<https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc>

 File cc/single_thread_proxy.cc (right):
>
> https://codereview.chromium.**org/12212007/diff/1001/cc/**
>
single_thread_proxy.cc#**newcode150<https://codereview.chromium.org/12212007/diff/1001/cc/single_thread_proxy.cc#newcode150>
> cc/single_thread_proxy.cc:150: {
> On 2013/02/05 22:42:48, piman wrote:
>
>> nit: { on previous line
>>
>
> Oh gosh this file has become a frankenstyle. :X
>
>
> https://codereview.chromium.**org/12212007/diff/1001/**
>
content/browser/renderer_host/**image_transport_factory.cc<https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_host/image_transport_factory.cc>
> File content/browser/renderer_host/**image_transport_factory.cc (right):
>
> https://codereview.chromium.**org/12212007/diff/1001/**
>
content/browser/renderer_host/**image_transport_factory.cc#**newcode660<https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_host/image_transport_factory.cc#newcode660>
> content/browser/renderer_host/**image_transport_factory.cc:**660:
> (*context3d) = make_scoped_ptr(**CreateOffscreenContext());
> On 2013/02/05 22:42:48, piman wrote:
>
>> nit: context3d->reset(**CreateOffscreenContext()) is shorter...
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12212007/diff/1001/**
>
content/browser/renderer_host/**image_transport_factory.cc#**newcode676<https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_host/image_transport_factory.cc#newcode676>
> content/browser/renderer_host/**image_transport_factory.cc:**676:
> reinterpret_cast<**GrPlatform3DContext>(**interface.get())));
> On 2013/02/05 22:42:48, piman wrote:
>
>> /cry on the reinterpret_cast
>>
>
> Ya... more incoming as well :X
>
>
> https://codereview.chromium.**org/12212007/diff/1001/**
>
content/browser/renderer_host/**image_transport_factory.cc#**newcode679<https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_host/image_transport_factory.cc#newcode679>
> content/browser/renderer_host/**image_transport_factory.cc:**679:
> *gr_context_memory_manager = make_scoped_ptr(new GrContextMemoryManager(
> On 2013/02/05 22:42:48, piman wrote:
>
>> nit: can also use reset(...) here
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12212007/diff/1001/**
>
content/browser/renderer_host/**image_transport_factory.cc#**newcode723<https://codereview.chromium.org/12212007/diff/1001/content/browser/renderer_host/image_transport_factory.cc#newcode723>
> content/browser/renderer_host/**image_transport_factory.cc:**723:
> scoped_ptr<WebKit::**WebGraphicsContext3D>
> offscreen_skia_context_main_**thread;
> On 2013/02/05 22:42:48, piman wrote:
>
>> Could we reuse the shared_context_ for the main thread? They're
>>
> costly, it's
>
>> worth saving.
>>
>
> I don't know, it looks like something that is created once per
> compositor, not once per process?


It's created once per process (otherwise it'd be in
the PerCompositorDataMap).


> I didn't understand what it's purpose
> was. Can you enlighten?
>

It's used for main thread things, like creating the backing textures for
the renderer, and used by the GLHelper to do the readback for the
thumbnailer.


>
>
https://codereview.chromium.**org/12212007/<https://codereview.chromium.org/1...
>

[danakj, 2013-02-05 23:26:16]

On Tue, Feb 5, 2013 at 6:19 PM, Antoine Labour <piman@chromium.org> wrote:

> On 2013/02/05 22:42:48, piman wrote:
>
>>  Could we reuse the shared_context_ for the main thread? They're
>>>
>> costly, it's
>>
>>> worth saving.
>>>
>>
>> I don't know, it looks like something that is created once per
>> compositor, not once per process?
>
>
> It's created once per process (otherwise it'd be in
> the PerCompositorDataMap).
>
>
>> I didn't understand what it's purpose
>> was. Can you enlighten?
>>
>
> It's used for main thread things, like creating the backing textures for
> the renderer, and used by the GLHelper to do the readback for the
> thumbnailer.
>

Ok, the only thing I'm worried about is that Ganesh will set various state
on the context you give it, and I hope that doesn't interfere with the
browser. I'll assume it's okay since you suggested it. :)

[piman, 2013-02-05 23:59:18]

On Tue, Feb 5, 2013 at 3:25 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Feb 5, 2013 at 6:19 PM, Antoine Labour <piman@chromium.org> wrote:
>
>> On 2013/02/05 22:42:48, piman wrote:
>>
>>>  Could we reuse the shared_context_ for the main thread? They're
>>>>
>>> costly, it's
>>>
>>>> worth saving.
>>>>
>>>
>>> I don't know, it looks like something that is created once per
>>> compositor, not once per process?
>>
>>
>> It's created once per process (otherwise it'd be in
>> the PerCompositorDataMap).
>>
>>
>>> I didn't understand what it's purpose
>>> was. Can you enlighten?
>>>
>>
>> It's used for main thread things, like creating the backing textures for
>> the renderer, and used by the GLHelper to do the readback for the
>> thumbnailer.
>>
>
> Ok, the only thing I'm worried about is that Ganesh will set various state
> on the context you give it, and I hope that doesn't interfere with the
> browser. I'll assume it's okay since you suggested it. :)
>

The good news is that we're not using Ganesh currently in the browser. We
may have to do some things if we ever try it, but I'm not too worried right
now. You can blame me for it later :)

Antoine

[danakj, 2013-02-06 03:37:52]

New patch. I added ui/compositor/offscreen_context.cc|h to implement
ui::OffscreenContext. This class lazily owns/creates an offscreen WGC3D and a
GrContext bound to it.

The DefaultContextFactory an GpuProcessTransportFactory each own
ui::OffscreenContext instances.

There's kinda a circular situation I realized though, as I write this.

The ContextFactory provides the offscreen contexts to the ui::Compositor, by
using a ui::OffscreenContext.
The ui::OffscreenContext creates its WGC3D via the ContextFactory.

This works because it's making use of a method that doesn't need the offscreen
context, but I wonder what do you think of it? It's a bit complicated since
GpuProcessTransportFactory owns the OffscreenContexts but is a ContextFactory.

[danakj, 2013-02-06 03:39:50]

(I didn't address the comments for cc/ yet, but I did get rid of the dual
shared_contexts in the browser.)

[danakj, 2013-02-06 04:58:19]

ResourceUpdateController::updateTexture also makes use of the offscreen
context3d. Now I recall that was what led me to think the resource provider was
a good place to hold the pointers.

Would you feel differently if I moved the GrContext* texture creation (and maybe
the picture drawing?) stuff into ResourceProvider? The offscreen contexts are
used for resources in two ways now:
- Drawing SkPicture to accelerated surface in ResourceUpdateController.
- Creating GrTextures for drawing compositor filters.

[danakj, 2013-02-07 22:00:31]

New patch adds cc::GaneshResourceProvider that cc::ResourceProvider owns. The
new class holds the offscreen WGContext3D and GrContext, and provides scoped
access to the contexts with correct flushing.  Bonus points for hopefully fixing
the flaky filter layout tests, as this improves the flush symantics on filters
to match those used in the resource update controller.

This should be feature-complete for offscreen browser contexts.

I need to still figure out how to allocate and provide the renderer compositor's
offscreen contexts in content/ without using WebSharedGraphicsContext, and I
should probably make sure it's possible to do so building on this CL first, but
I'd like to make that a followup regardless.

[Stephen White, 2013-02-07 22:07:52]

I'll look at the context creation stuff later, but just a quick question about
the context flushing.

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc#oldcode435
cc/gl_renderer.cc:435: canvas.flush();
I hate to be a pain, but couldn't this simply be fixed by changing this
canvas.flush() to grContext->flush()?  It just seems like the guard class is a
bit overkill, in this case.  (I assume that GrContext::flush() subsumes the
functionality of canvas.flush(), but Brian can correct me if I'm wrong).

[danakj, 2013-02-07 22:11:14]

Thanks :)

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc#oldcode435
cc/gl_renderer.cc:435: canvas.flush();
On 2013/02/07 22:07:52, Stephen White wrote:
> I hate to be a pain, but couldn't this simply be fixed by changing this
> canvas.flush() to grContext->flush()?  It just seems like the guard class is a
> bit overkill, in this case.  (I assume that GrContext::flush() subsumes the
> functionality of canvas.flush(), but Brian can correct me if I'm wrong).

I like the guard class because it makes it impossible to miss the flush. We have
3 code paths that use these contexts and they each do different flushing. I'd
like to prevent that from happening in the future.

if we can drop the canvas.flush() here that'd be cool.

[danakj, 2013-02-08 00:39:11]

WindowAnimationsTest.HideShowBrightnessGrayscaleAnimation is failing on the bot,
but it passes for me locally, not sure what is wrong there.

Similar for the two webgl layout tests.

Any advice/insight would be helpful.

[danakj, 2013-02-08 01:25:02]

On 2013/02/08 00:39:11, danakj wrote:
> WindowAnimationsTest.HideShowBrightnessGrayscaleAnimation is failing on the
bot,
> but it passes for me locally, not sure what is wrong there.
> 
> Similar for the two webgl layout tests.

The WebGL failures seem unrelated, they are failing on other cls. That leaves
the ash tests tho.

[danakj, 2013-02-08 01:37:58]

On 2013/02/08 01:25:02, danakj wrote:
> On 2013/02/08 00:39:11, danakj wrote:
> > WindowAnimationsTest.HideShowBrightnessGrayscaleAnimation is failing on the
> bot,
> > but it passes for me locally, not sure what is wrong there.

So I realized that ui::Compositor does not call ContextFactory::GetInstance()
when g_test_compositor_enabled is true. That seems like it will fix the test
failures.

I'm really confused by the g_test_compositor_enabled logic though. Why won't GL
initialize correctly in this case but we can still construct GL contexts clearly
by avoiding the GLSurface::InitializeOneOff()?

What's the point of having DefaultContextFactory do anything at all if GLSurface
won't work?

[bsalomon_chromium, 2013-02-08 01:55:56]

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12212007/diff/49/cc/gl_renderer.cc#oldcode435
cc/gl_renderer.cc:435: canvas.flush();
On 2013/02/07 22:11:14, danakj wrote:
> On 2013/02/07 22:07:52, Stephen White wrote:
> > I hate to be a pain, but couldn't this simply be fixed by changing this
> > canvas.flush() to grContext->flush()?  It just seems like the guard class is
a
> > bit overkill, in this case.  (I assume that GrContext::flush() subsumes the
> > functionality of canvas.flush(), but Brian can correct me if I'm wrong).
> 
> I like the guard class because it makes it impossible to miss the flush. We
have
> 3 code paths that use these contexts and they each do different flushing. I'd
> like to prevent that from happening in the future.
> 
> if we can drop the canvas.flush() here that'd be cool.

Calling GrContext::flush() is sufficient. You don't need to also call
SkCanvas::flush().

[danakj, 2013-02-08 01:57:58]

On Thu, Feb 7, 2013 at 8:55 PM, <bsalomon@chromium.org> wrote:

> Calling GrContext::flush() is sufficient. You don't need to also call
> SkCanvas::flush().
>
>
Thanks!

[danakj, 2013-02-09 02:36:45]

I moved the OffscreenContext class stuff into content/common/gpu/client.

With that, I have a CL that makes RenderWidgetCompositor allocate the shared
contexts via RenderThreadImpl, which itself owns the OffscreenContext objects
for the main and compositor threads.

So, I'll call this my first real proposal CL now. Please feel free to rip it
apart. I'll put up separate CLs for the renderer compositor contexts.

[jamesr, 2013-02-09 02:48:11]

https://codereview.chromium.org/12212007/diff/2132/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/2132/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:291: // TODO(danakj): This
should go through WebWidget
what would this have to do with WebWidget?

https://codereview.chromium.org/12212007/diff/20108/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/20108/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:303: return
WebKit::WebSharedGraphicsContext3D::compositorThreadContext();
this will go to the new content/common/gpu/offscreen_context class, right?

[danakj, 2013-02-09 02:50:51]

https://codereview.chromium.org/12212007/diff/2132/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/2132/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:291: // TODO(danakj): This
should go through WebWidget
On 2013/02/09 02:48:11, jamesr wrote:
> what would this have to do with WebWidget?

I copied this from the other methods, I assumed you meant that WebWidget will be
the LTHClient instead of this class. The contents of the methods would just move
there.

https://codereview.chromium.org/12212007/diff/20108/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/20108/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:303: return
WebKit::WebSharedGraphicsContext3D::compositorThreadContext();
On 2013/02/09 02:48:12, jamesr wrote:
> this will go to the new content/common/gpu/offscreen_context class, right?

Ya this calls
RenderThreadImpl::current()->OffscreenContext3dForCompositorThread().

RenderThreadImpl owns two OffscreenContextCommandBuffer classes (one for each
thread).

[piman, 2013-02-11 18:45:32]

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
File cc/ganesh_resource_provider.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.cc:37: if (gr_context()) gr_context()->flush();
nit: statement goes onto next line.

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.cc:40: if (context3d()) context3d()->flush();
nit: statement goes onto next line.

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provider.h
File cc/ganesh_resource_provider.h (right):

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.h:17: class CC_EXPORT GaneshResourceProvider {
nit: GaneshContextProvider?

https://codereview.chromium.org/12212007/diff/9135/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/gl_renderer.cc#newcode405
cc/gl_renderer.cc:405: skia::RefPtr<GrTexture> texture =
skia::AdoptRef(ganeshContexts.gr_context()->createPlatformTexture(backendTextureDescription));
Is the s/wrapBackendTexture/createPlatformTexture/ intentional?

https://codereview.chromium.org/12212007/diff/9135/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:146: offscreenContext3d =
m_layerTreeHost->client()->OffscreenContext3dForMainThread();
ForCompositorThread, no?

https://codereview.chromium.org/12212007/diff/9135/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12212007/diff/9135/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:607: void
OnLostMainThreadSharedContext() {
Mmh, there was code here to keep alive the context and the GLHelper while we
call the observers, but resetting them so that if the OnLostResources callback
tries to access it (I'm pretty sure it does), it would recreate it.
I'm not sure how that behavior is kept, at the very least, I don't see what
resets the GLHelper.

https://codereview.chromium.org/12212007/diff/9135/content/common/gpu/client/...
File content/common/gpu/client/offscreen_context.h (right):

https://codereview.chromium.org/12212007/diff/9135/content/common/gpu/client/...
content/common/gpu/client/offscreen_context.h:36: WebKit::WebGraphicsContext3D*
Context3d();
nit: Context3D for consistency?

[danakj, 2013-02-11 18:58:12]

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
File cc/ganesh_resource_provider.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.cc:37: if (gr_context()) gr_context()->flush();
On 2013/02/11 18:45:33, piman wrote:
> nit: statement goes onto next line.

Done.

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.cc:40: if (context3d()) context3d()->flush();
On 2013/02/11 18:45:33, piman wrote:
> nit: statement goes onto next line.

Done.

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provider.h
File cc/ganesh_resource_provider.h (right):

https://codereview.chromium.org/12212007/diff/9135/cc/ganesh_resource_provide...
cc/ganesh_resource_provider.h:17: class CC_EXPORT GaneshResourceProvider {
On 2013/02/11 18:45:33, piman wrote:
> nit: GaneshContextProvider?

Done.

https://codereview.chromium.org/12212007/diff/9135/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/gl_renderer.cc#newcode405
cc/gl_renderer.cc:405: skia::RefPtr<GrTexture> texture =
skia::AdoptRef(ganeshContexts.gr_context()->createPlatformTexture(backendTextureDescription));
On 2013/02/11 18:45:33, piman wrote:
> Is the s/wrapBackendTexture/createPlatformTexture/ intentional?

Done. This code changed and unchanged a lot, I think this is introduced noise..
thanks.

https://codereview.chromium.org/12212007/diff/9135/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

https://codereview.chromium.org/12212007/diff/9135/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:146: offscreenContext3d =
m_layerTreeHost->client()->OffscreenContext3dForMainThread();
On 2013/02/11 18:45:33, piman wrote:
> ForCompositorThread, no?

No, this is single thread proxy, so it wants the main thread context.

https://codereview.chromium.org/12212007/diff/9135/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12212007/diff/9135/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:607: void
OnLostMainThreadSharedContext() {
On 2013/02/11 18:45:33, piman wrote:
> Mmh, there was code here to keep alive the context and the GLHelper while we
> call the observers, but resetting them so that if the OnLostResources callback
> tries to access it (I'm pretty sure it does), it would recreate it.
> I'm not sure how that behavior is kept, at the very least, I don't see what
> resets the GLHelper.

Hm, ya I lost the GLHelper thing. I kept the resource-holding logic but it moved
to the OffscreenContext class, and since this is called via a post task, then it
doesn't do the same thing anymore. So, questions:

1) Why do we hold onto the WGC3D here? Tests don't see to depend on it.
2) Why do we call the observers on a post-task instead of in the lost-context
callback directly?

https://codereview.chromium.org/12212007/diff/9135/content/common/gpu/client/...
File content/common/gpu/client/offscreen_context.h (right):

https://codereview.chromium.org/12212007/diff/9135/content/common/gpu/client/...
content/common/gpu/client/offscreen_context.h:36: WebKit::WebGraphicsContext3D*
Context3d();
On 2013/02/11 18:45:33, piman wrote:
> nit: Context3D for consistency?

Hm, I've been using Context3d for function names and context3d for variables
everywhere including in cc/.

[Stephen White, 2013-02-11 19:48:23]

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#oldcode455
cc/gl_renderer.cc:455: context3d->flush();
I still don't think it's a good idea to make this flushing the responsibility of
a guard class.  For example, someone could add some code after the
accessBitmap() but before the guard class flushed the drawing commands, and the
contents of the texture would be wrong, and the guard class hides the fact that
this flushing is taking place.  I think it's better to be explicit here.

(BTW, I would've sworn that we used to flush the GrContext every time the
GraphicsContext3D was flushed, somewhere in the bowels of
[Web]GraphicsContext3D[Private][Impl] but I can't seem to find it now.)

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode376
cc/gl_renderer.cc:376: if
(!renderer->resourceProvider()->ganeshContextProvider()->has_contexts())
Nit:  Skia doesn't actually refer to "Ganesh" anywhere in the API or code.  I'd
prefer we don't start that here.  Maybe grContextProvider(), if that makes
sense?

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode401
cc/gl_renderer.cc:401: backendTextureDescription.fHeight =
sourceResource->size().height();
Nit:  Is this sourceResource not always a texture?  I realize it's a
ScopedResource by type, but if it's always a texture, I'd prefer that the
parameter continues to reflect that.

[danakj, 2013-02-11 21:06:13]

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode376
cc/gl_renderer.cc:376: if
(!renderer->resourceProvider()->ganeshContextProvider()->has_contexts())
On 2013/02/11 19:48:23, Stephen White wrote:
> Nit:  Skia doesn't actually refer to "Ganesh" anywhere in the API or code. 
I'd
> prefer we don't start that here.  Maybe grContextProvider(), if that makes
> sense?

Ya ok, I think that sounds fine.

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode401
cc/gl_renderer.cc:401: backendTextureDescription.fHeight =
sourceResource->size().height();
On 2013/02/11 19:48:23, Stephen White wrote:
> Nit:  Is this sourceResource not always a texture?  I realize it's a
> ScopedResource by type, but if it's always a texture, I'd prefer that the
> parameter continues to reflect that.

Ya it will be a texture, I was confused because it was called texture but the
actual type is a ScopedResource. How about sourceTextureResource?

[Stephen White, 2013-02-11 21:12:05]

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode376
cc/gl_renderer.cc:376: if
(!renderer->resourceProvider()->ganeshContextProvider()->has_contexts())
On 2013/02/11 19:48:23, Stephen White wrote:
> Nit:  Skia doesn't actually refer to "Ganesh" anywhere in the API or code. 
I'd
> prefer we don't start that here.  Maybe grContextProvider(), if that makes
> sense?

Actually, maybe if it provides both the GrContext and the GC3D, it should just
be contextProvider()?  Not sure...

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode401
cc/gl_renderer.cc:401: backendTextureDescription.fHeight =
sourceResource->size().height();
On 2013/02/11 21:06:13, danakj wrote:
> On 2013/02/11 19:48:23, Stephen White wrote:
> > Nit:  Is this sourceResource not always a texture?  I realize it's a
> > ScopedResource by type, but if it's always a texture, I'd prefer that the
> > parameter continues to reflect that.
> 
> Ya it will be a texture, I was confused because it was called texture but the
> actual type is a ScopedResource. How about sourceTextureResource?

Sure, that's fine.

[danakj, 2013-02-11 21:14:21]

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode376
cc/gl_renderer.cc:376: if
(!renderer->resourceProvider()->ganeshContextProvider()->has_contexts())
On 2013/02/11 21:12:06, Stephen White wrote:
> On 2013/02/11 19:48:23, Stephen White wrote:
> > Nit:  Skia doesn't actually refer to "Ganesh" anywhere in the API or code. 
> I'd
> > prefer we don't start that here.  Maybe grContextProvider(), if that makes
> > sense?
> 
> Actually, maybe if it provides both the GrContext and the GC3D, it should just
> be contextProvider()?  Not sure...

Well, it's the SharedOffscreenContextProvider. I could just name it that.. my
intention had been to express that the WGC3D is being used by Ganesh so it
should only really be used for that, to contrast it with the compositor context.

[Stephen White, 2013-02-11 21:17:48]

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/2136/cc/gl_renderer.cc#newcode376
cc/gl_renderer.cc:376: if
(!renderer->resourceProvider()->ganeshContextProvider()->has_contexts())
On 2013/02/11 21:14:21, danakj wrote:
> On 2013/02/11 21:12:06, Stephen White wrote:
> > On 2013/02/11 19:48:23, Stephen White wrote:
> > > Nit:  Skia doesn't actually refer to "Ganesh" anywhere in the API or code.

> > I'd
> > > prefer we don't start that here.  Maybe grContextProvider(), if that makes
> > > sense?
> > 
> > Actually, maybe if it provides both the GrContext and the GC3D, it should
just
> > be contextProvider()?  Not sure...
> 
> Well, it's the SharedOffscreenContextProvider. I could just name it that.. my
> intention had been to express that the WGC3D is being used by Ganesh so it
> should only really be used for that, to contrast it with the compositor
context.

Hmm.. just contextProvider() doesn't cut it, I guess?  skiaContextProvider()? 
sharedContextProvider()?  (shared contexts are always used for Ganesh, but I
guess that's just historical rather than explanatory).  I got nothin'.

[danakj, 2013-02-12 07:34:04]

OK rebooted the patch (again).

The browser-side changes are a lot less complicated now (yay). GrContextProvider
creates and owns the GrContext inside cc/.

I did some testing with losing the GPU process to make sure things work. The sad
news is that we can't really test lost context interaction with GrContext/Ganesh
until we make FakeWGC3D able to mimic enough "real" behaviour to make ganesh use
it. The good news is that we can lose a context and not crash with a filter in
use.

I didn't look at unscoping the flushing yet.. must sleep first.

One question for where I should be going below:

https://codereview.chromium.org/12212007/diff/22040/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/12212007/diff/22040/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:271: return
WebSharedGraphicsContext3D::mainThreadContext();
Should this be going right to the webkit/ version of WebKitPlatformSupport in
the future?

[danakj, 2013-02-13 05:49:26]

This trip has certainly been enlightening. PTAL.

1) embedder owns the GrContext. this is a requirement so that we only have 1
GrContext per WGC3D.
2) dropped the scoped flushing for explicit flushing as requested.
3) named the offscreen context provider class type as GrContextProvider.
4) added a DestroyOffscreenContext3DForCompositorThread() to the client API so
that we can tell the embedder when the context is gone (it can't easily watch
for it since the context lives on the compositor thread).

[Stephen White, 2013-02-13 15:26:05]

https://codereview.chromium.org/12212007/diff/5179/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/5179/cc/gl_renderer.cc#newcode408
cc/gl_renderer.cc:408: offscreenContexts->BeginUsingContexts();
This is still somewhat mysterious to someone who doesn't know this code.  What
do Begin and End do?  Could we just call Begin FlushCompositorContext() and End
FlushSharedContext(), or something (not sold on the last one's name), so it's
clear what those calls do at the call site?

[piman, 2013-02-13 19:13:30]

https://codereview.chromium.org/12212007/diff/5179/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12212007/diff/5179/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.cc:628:
memory_callback_proxy_compositor_thread_.reset(new MemoryCallbackProxy(
So, this is called on the main thread, but the MemoryCallbackProxy will do
context calls in the constructor/destructor (without making the context
current). That's racy for several reasons (among others, the
setMemoryAllocationChangedCallbackCHROMIUM may end up as a noop)...

I'm not sure how to solve that well, what a mess!

In the current WebKit implementation, the create() happens on the main thread,
but the get(), which lazily creates the gr context, happens on the compositor
thread - hence why it works there (FTM the pattern in
getOrCreateContextForImplThread isn't thread safe, but who's counting.).


The closest would be to have a separate Create*ForCompositorThread (on LTHC) and
a Get*ForCompositorThread called from the impl thread. Where that call belongs
isn't clear... It could be LTHC with big red letters saying "called on impl
thread", but that feels wrong. It could be OutputSurface, I don't like the scope
creep, but short of introducing another class (LTHIClient) I don't have anything
better.

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:359: return
WebKit::WebSharedGraphicsContext3D::compositorThreadContext();
WebSharedGraphicsContext3D::compositorThreadContext is meant to be called from
the impl thread - I don't think it asserts but I think things will go wrong (see
comments in image_transport_factory)

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:369: return
WebKit::WebSharedGraphicsContext3D::compositorThreadGrContext();
Same here, I think this will break if called on the main thread.

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.c...
ui/compositor/compositor.cc:173: scoped_ptr<ui::TestWebGraphicsContext3D>
test_context(
Mmh, we should only be creating TestWGC3D-type contexts if
g_test_compositor_enabled is set. Otherwise in the general case, we'll be trying
to share fake texture/programs/... with a real context and bad things will
happen.

I'd rather have this always create a real context (through
CreateOffscreenContext for example), so that it's consistent, and have
Compositor decide which type of context based on g_test_compositor_enabled, like
it does for createOutputSurface.

https://codereview.chromium.org/12212007/diff/5179/webkit/gpu/grcontext_for_w...
File webkit/gpu/grcontext_for_webgraphicscontext3d.h (right):

https://codereview.chromium.org/12212007/diff/5179/webkit/gpu/grcontext_for_w...
webkit/gpu/grcontext_for_webgraphicscontext3d.h:17: // This class binds an
offscreen GrContext to an offscreen context3d.
nit: it'd be useful to remind the user that the context3d must outlive this
class.

[danakj, 2013-02-13 20:47:09]

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.c...
ui/compositor/compositor.cc:173: scoped_ptr<ui::TestWebGraphicsContext3D>
test_context(
On 2013/02/13 19:13:31, piman wrote:
> Mmh, we should only be creating TestWGC3D-type contexts if
> g_test_compositor_enabled is set. Otherwise in the general case, we'll be
trying
> to share fake texture/programs/... with a real context and bad things will
> happen.
> 
> I'd rather have this always create a real context (through
> CreateOffscreenContext for example), so that it's consistent, and have
> Compositor decide which type of context based on g_test_compositor_enabled,
like
> it does for createOutputSurface.

The downside to this is that then we need storage in the compositor (or statics)
to hold the context3d because they need to persist across multiple calls to the
function.

I'll make them statics I guess since they shouldn't be per-compositor.

https://codereview.chromium.org/12212007/diff/5179/webkit/gpu/grcontext_for_w...
File webkit/gpu/grcontext_for_webgraphicscontext3d.h (right):

https://codereview.chromium.org/12212007/diff/5179/webkit/gpu/grcontext_for_w...
webkit/gpu/grcontext_for_webgraphicscontext3d.h:17: // This class binds an
offscreen GrContext to an offscreen context3d.
On 2013/02/13 19:13:31, piman wrote:
> nit: it'd be useful to remind the user that the context3d must outlive this
> class.

Done.

[danakj, 2013-02-13 20:57:23]

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:359: return
WebKit::WebSharedGraphicsContext3D::compositorThreadContext();
On 2013/02/13 19:13:31, piman wrote:
> WebSharedGraphicsContext3D::compositorThreadContext is meant to be called from
> the impl thread - I don't think it asserts but I think things will go wrong
(see
> comments in image_transport_factory)

I think this one is okay, it just does this:
    PassRefPtr<GraphicsContext3D> getContext()
    {
        return m_context;
    }

https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:369: return
WebKit::WebSharedGraphicsContext3D::compositorThreadGrContext();
On 2013/02/13 19:13:31, piman wrote:
> Same here, I think this will break if called on the main thread.

Right.. ok I'll have to make the gr context creation on impl.

[piman, 2013-02-14 00:16:07]

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.c...
ui/compositor/compositor.cc:173: scoped_ptr<ui::TestWebGraphicsContext3D>
test_context(
On 2013/02/13 20:47:10, danakj wrote:
> On 2013/02/13 19:13:31, piman wrote:
> > Mmh, we should only be creating TestWGC3D-type contexts if
> > g_test_compositor_enabled is set. Otherwise in the general case, we'll be
> trying
> > to share fake texture/programs/... with a real context and bad things will
> > happen.
> > 
> > I'd rather have this always create a real context (through
> > CreateOffscreenContext for example), so that it's consistent, and have
> > Compositor decide which type of context based on g_test_compositor_enabled,
> like
> > it does for createOutputSurface.
> 
> The downside to this is that then we need storage in the compositor (or
statics)
> to hold the context3d because they need to persist across multiple calls to
the
> function.
> 
> I'll make them statics I guess since they shouldn't be per-compositor.

Why couldn't they still be fields in DefaultContextFactory?

[piman, 2013-02-14 00:20:42]

On Wed, Feb 13, 2013 at 12:57 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/12212007/diff/5179/**
>
content/renderer/gpu/render_**widget_compositor.cc<https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/render_widget_compositor.cc>
> File content/renderer/gpu/render_**widget_compositor.cc (right):
>
> https://codereview.chromium.**org/12212007/diff/5179/**
>
content/renderer/gpu/render_**widget_compositor.cc#**newcode359<https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/render_widget_compositor.cc#newcode359>
> content/renderer/gpu/render_**widget_compositor.cc:359: return
> WebKit::**WebSharedGraphicsContext3D::**compositorThreadContext();
> On 2013/02/13 19:13:31, piman wrote:
>
>> WebSharedGraphicsContext3D::**compositorThreadContext is meant to be
>>
> called from
>
>> the impl thread - I don't think it asserts but I think things will go
>>
> wrong (see
>
>> comments in image_transport_factory)
>>
>
> I think this one is okay, it just does this:
>     PassRefPtr<GraphicsContext3D> getContext()
>     {
>         return m_context;
>
>     }
>

That's fair - I think it breaks the intention, but the pattern is actually
better in this case - since in the current world, the static is set in one
thread but read in another one, with no synchronization, and this would fix
that.
As long as James is ok with this, that's fine by me.


> https://codereview.chromium.**org/12212007/diff/5179/**
>
content/renderer/gpu/render_**widget_compositor.cc#**newcode369<https://codereview.chromium.org/12212007/diff/5179/content/renderer/gpu/render_widget_compositor.cc#newcode369>
> content/renderer/gpu/render_**widget_compositor.cc:369: return
> WebKit::**WebSharedGraphicsContext3D::**compositorThreadGrContext();
> On 2013/02/13 19:13:31, piman wrote:
>
>> Same here, I think this will break if called on the main thread.
>>
>
> Right.. ok I'll have to make the gr context creation on impl.
>

This is the killer, yes...
OTOH, since this context isn't actually used outside of cc, maybe it could
make sense to make this one internal to cc, if we can figure out a good way
to do that (you're probably hating me at this point :/).


>
https://codereview.chromium.**org/12212007/<https://codereview.chromium.org/1...
>

[danakj, 2013-02-14 00:36:39]

On 2013/02/14 00:16:07, piman wrote:
> https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.c...
> ui/compositor/compositor.cc:173: scoped_ptr<ui::TestWebGraphicsContext3D>
> test_context(
> On 2013/02/13 20:47:10, danakj wrote:
> > On 2013/02/13 19:13:31, piman wrote:
> > > Mmh, we should only be creating TestWGC3D-type contexts if
> > > g_test_compositor_enabled is set. Otherwise in the general case, we'll be
> > trying
> > > to share fake texture/programs/... with a real context and bad things will
> > > happen.
> > > 
> > > I'd rather have this always create a real context (through
> > > CreateOffscreenContext for example), so that it's consistent, and have
> > > Compositor decide which type of context based on
g_test_compositor_enabled,
> > like
> > > it does for createOutputSurface.
> > 
> > The downside to this is that then we need storage in the compositor (or
> statics)
> > to hold the context3d because they need to persist across multiple calls to
> the
> > function.
> > 
> > I'll make them statics I guess since they shouldn't be per-compositor.
> 
> Why couldn't they still be fields in DefaultContextFactory?

Well, I thought you were suggesting the ui::Compositor make a test context
instead of using the DefaultContextFactory with g_test_compositor_enabled is
true, so that DefaultContextFactory can always make a real context.

Maybe I misunderstood some part of that?

[piman, 2013-02-14 00:59:51]

On Wed, Feb 13, 2013 at 4:36 PM, <danakj@chromium.org> wrote:

> On 2013/02/14 00:16:07, piman wrote:
>
>> https://codereview.chromium.**org/12212007/diff/5179/ui/**
>>
compositor/compositor.cc<https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc>
>> File ui/compositor/compositor.cc (right):
>>
>
>
> https://codereview.chromium.**org/12212007/diff/5179/ui/**
>
compositor/compositor.cc#**newcode173<https://codereview.chromium.org/12212007/diff/5179/ui/compositor/compositor.cc#newcode173>
>
>> ui/compositor/compositor.cc:**173: scoped_ptr<ui::**
>> TestWebGraphicsContext3D>
>> test_context(
>> On 2013/02/13 20:47:10, danakj wrote:
>> > On 2013/02/13 19:13:31, piman wrote:
>> > > Mmh, we should only be creating TestWGC3D-type contexts if
>> > > g_test_compositor_enabled is set. Otherwise in the general case,
>> we'll be
>> > trying
>> > > to share fake texture/programs/... with a real context and bad things
>> will
>> > > happen.
>> > >
>> > > I'd rather have this always create a real context (through
>> > > CreateOffscreenContext for example), so that it's consistent, and have
>> > > Compositor decide which type of context based on
>>
> g_test_compositor_enabled,
>
>> > like
>> > > it does for createOutputSurface.
>> >
>> > The downside to this is that then we need storage in the compositor (or
>> statics)
>> > to hold the context3d because they need to persist across multiple
>> calls to
>> the
>> > function.
>> >
>> > I'll make them statics I guess since they shouldn't be per-compositor.
>>
>
>  Why couldn't they still be fields in DefaultContextFactory?
>>
>
> Well, I thought you were suggesting the ui::Compositor make a test context
> instead of using the DefaultContextFactory with g_test_compositor_enabled
> is
> true, so that DefaultContextFactory can always make a real context.
>
> Maybe I misunderstood some part of that?
>

Oh, I see we were talking about different things. Sorry, you're probably
right, that one would need to be static, or you'd need to do it in the
factory.

Sorry, I should really clean up the interaction between the ContextFactory
and the ImageTransportFactory to make this less painful.


>
https://codereview.chromium.**org/12212007/<https://codereview.chromium.org/1...
>

[danakj, 2013-02-21 06:20:53]

PTAL

When all else fails, punt! No more cc::GrContextProvider, so I left all the
explicit GrContext flushes, but made them consistent across the 3 sites.

Here's the plan:

LTHClient has an API to return a ui::ContextProvider. It always must return some
provider, and this provider will own a WGC3D.

The WGC3D is created on the main thread by initializing the provider there. It
is bound to the thread to be used on later.

The provider can watch for lost context events. When that happens, it does *not*
delete its context. Instead it invalidates itself. Then, the next time someone
asks for a provider, the LTHClient implementation can see its current provider
is invalidated, and make a new one. This way the WGC3D is kept alive after loss
until no one is left holding onto the provider that created it.

This should be more threadsafe friendly than the existing code is.

I made an implementation of ui::ContextProvider that uses a
WGC3DCommandBufferImpl in content/common/gpu/client/ which is the provider
handed out by ImageTransportFactory through ui::Compositor to cc/.

There's a few other implementations of ui::ContextProvider as well:
- DefaultContextProvider in ui::Compositor that just creates and returns valid
contexts without watching for context loss or anything fancy.
- NullContextProvider in ui::Compositor for the test compositor. This could
instead return TestWGC3D in the future.
- NullContextProvider in content/browser/r_h/compositor_impl_android. Android's
browser compositor doesn't need offscreen contexts as they don't intend to use
ganesh filters that would require one.

I will add a ContextProviderInProcessImpl later for the in-process WebGL when I
move WebKit over to using this stuff.

[danakj, 2013-02-21 06:41:15]

Oh, and also:

- RenderWidgetCompositor has its own ContextProviders that just wrap the
WebSharedGraphicsContext3D class. These will move to RenderThreadImpl and act
more like GpuProcessTransportFactory later on.
- WebLayerTreeViewImplForTesting and WebLayerTreeViewImpl have similar
ContextProviders. These will have to create an inprocess context similar to the
DefaultContextFactory in ui. Maybe we can unite that somehow.

[piman, 2013-02-21 22:49:47]

Note: a couple of small thread safety worries/problems, otherwise mostly nits.

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc#newcode392
cc/gl_renderer.cc:392: // Make sure ganesh uses the correct GL context.
nit: s/ganesh/skia/

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc#newcode403
cc/gl_renderer.cc:403: offscreenContexts->Context3d()->flush();
Should we also make the compositor context current again? Is it done somewhere
else? Or has this been broken all along?
(note: it doesn't matter for command-buffer contexts but it does for in-process
ones).

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:352: return
null_offscreen_context_provider_;
I assume that Android UI doesn't use filters right now?

https://codereview.chromium.org/12212007/diff/37039/content/common/gpu/client...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/common/gpu/client...
content/common/gpu/client/context_provider_command_buffer.cc:92: destroyed_ =
true;
The pattern is a bit iffy because you set this on the impl thread but test this
on the main thread, without locks (meaning technically the behavior is
undefined). Just adding a lock should do it I think.

The same is true of most of the ContextProvider implementations in this CL.

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:472: destroyed_ = !Context3d()
|| Context3d()->isContextLost();
same here wrt thread safety

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:475: protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:477: private:
nit: blank line before 'private:'

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:82: protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:86: static scoped_refptr<NullContextProvider>
You can't have non-POD as globals.
Use Singletons?

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:171: destroyed_ = true;
same here wrt thread safety

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h
File ui/gl/context_provider.h (right):

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
ui/gl/context_provider.h:11: namespace WebKit { class WebGraphicsContext3D; }
nit: It's a bit weird to have this file in ui/gl, and have ui/gl depend on
WebKit.
How about moving this file to webkit/gpu?

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
ui/gl/context_provider.h:15: class ContextProvider : public
base::RefCounted<ContextProvider> {
RefCountedThreadSafe

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:301: protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:332: destroyed_ = true;
same here wrt thread safety

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:335: protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:337: private:
nit: blank line before 'private:'

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:241:
protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:273:
destroyed_ = true;
same here wrt thread safety

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:276:
protected:
nit: blank line before 'protected:'

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:278: private:
nit: blank line before 'private:'

[danakj, 2013-02-22 01:56:31]

Thanks! All addressed. PTAL

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc#newcode392
cc/gl_renderer.cc:392: // Make sure ganesh uses the correct GL context.
On 2013/02/21 22:49:48, piman wrote:
> nit: s/ganesh/skia/

Done.

https://codereview.chromium.org/12212007/diff/37039/cc/gl_renderer.cc#newcode403
cc/gl_renderer.cc:403: offscreenContexts->Context3d()->flush();
On 2013/02/21 22:49:48, piman wrote:
> Should we also make the compositor context current again? Is it done somewhere
> else? Or has this been broken all along?
> (note: it doesn't matter for command-buffer contexts but it does for
in-process
> ones).

I copied this code from resource update controller as it was the most verbose
explaining its steps. I'll make them all make current back to the compositor
context. Nice catch!

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:352: return
null_offscreen_context_provider_;
On 2013/02/21 22:49:48, piman wrote:
> I assume that Android UI doesn't use filters right now?

Right. And don't intend to use filters that will require this context. I
confirmed this with @aelias.

https://codereview.chromium.org/12212007/diff/37039/content/common/gpu/client...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/common/gpu/client...
content/common/gpu/client/context_provider_command_buffer.cc:92: destroyed_ =
true;
On 2013/02/21 22:49:48, piman wrote:
> The pattern is a bit iffy because you set this on the impl thread but test
this
> on the main thread, without locks (meaning technically the behavior is
> undefined). Just adding a lock should do it I think.
> 
> The same is true of most of the ContextProvider implementations in this CL.

Done, and thanks for the explanations!

Renamed all the destroyed() functions to DestroyedOnMainThread() to make this
all more obvious too.

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:472: destroyed_ = !Context3d()
|| Context3d()->isContextLost();
On 2013/02/21 22:49:48, piman wrote:
> same here wrt thread safety

Done.

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:475: protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:477: private:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'private:'

Done.

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:82: protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:86: static scoped_refptr<NullContextProvider>
On 2013/02/21 22:49:48, piman wrote:
> You can't have non-POD as globals.
> Use Singletons?

Oh right, POD only.. I used a Singleton class that wraps a scoped_refptr, and
one for each thread.

https://codereview.chromium.org/12212007/diff/37039/ui/compositor/compositor....
ui/compositor/compositor.cc:171: destroyed_ = true;
On 2013/02/21 22:49:48, piman wrote:
> same here wrt thread safety

Done.

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h
File ui/gl/context_provider.h (right):

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
ui/gl/context_provider.h:11: namespace WebKit { class WebGraphicsContext3D; }
On 2013/02/21 22:49:48, piman wrote:
> nit: It's a bit weird to have this file in ui/gl, and have ui/gl depend on
> WebKit.
> How about moving this file to webkit/gpu?

Oh, somehow I thought cc can't see webkit/gpu, but it can.. I'll try moving it
and see if anything blows up!

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
ui/gl/context_provider.h:15: class ContextProvider : public
base::RefCounted<ContextProvider> {
On 2013/02/21 22:49:48, piman wrote:
> RefCountedThreadSafe

Aahh ok!

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:301: protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:332: destroyed_ = true;
On 2013/02/21 22:49:48, piman wrote:
> same here wrt thread safety

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:335: protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:337: private:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'private:'

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:241:
protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:273:
destroyed_ = true;
On 2013/02/21 22:49:48, piman wrote:
> same here wrt thread safety

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:276:
protected:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'protected:'

Done.

https://codereview.chromium.org/12212007/diff/37039/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc:278: private:
On 2013/02/21 22:49:48, piman wrote:
> nit: blank line before 'private:'

Done.

[danakj, 2013-02-22 01:59:28]

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h
File ui/gl/context_provider.h (right):

https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
ui/gl/context_provider.h:11: namespace WebKit { class WebGraphicsContext3D; }
On 2013/02/22 01:56:31, danakj wrote:
> On 2013/02/21 22:49:48, piman wrote:
> > nit: It's a bit weird to have this file in ui/gl, and have ui/gl depend on
> > WebKit.
> > How about moving this file to webkit/gpu?
> 
> Oh, somehow I thought cc can't see webkit/gpu, but it can.. I'll try moving it
> and see if anything blows up!

Or actually there's a TODO in our DEPS, which I guess was added with webkit/gpu
since I last looked.

Where else can this class could live then since it will outlive WGC3D?

[danakj, 2013-02-22 02:06:38]

On 2013/02/22 01:59:28, danakj wrote:
> https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h
> File ui/gl/context_provider.h (right):
> 
>
https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
> ui/gl/context_provider.h:11: namespace WebKit { class WebGraphicsContext3D; }
> On 2013/02/22 01:56:31, danakj wrote:
> > On 2013/02/21 22:49:48, piman wrote:
> > > nit: It's a bit weird to have this file in ui/gl, and have ui/gl depend on
> > > WebKit.
> > > How about moving this file to webkit/gpu?
> > 
> > Oh, somehow I thought cc can't see webkit/gpu, but it can.. I'll try moving
it
> > and see if anything blows up!
> 
> Or actually there's a TODO in our DEPS, which I guess was added with
webkit/gpu
> since I last looked.
> 
> Where else can this class could live then since it will outlive WGC3D?

James points out it seems like I can just stick this in cc/ now. I'll try that.

[danakj, 2013-02-22 02:33:27]

On 2013/02/22 02:06:38, danakj wrote:
> On 2013/02/22 01:59:28, danakj wrote:
> > https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h
> > File ui/gl/context_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/12212007/diff/37039/ui/gl/context_provider.h#...
> > ui/gl/context_provider.h:11: namespace WebKit { class WebGraphicsContext3D;
}
> > On 2013/02/22 01:56:31, danakj wrote:
> > > On 2013/02/21 22:49:48, piman wrote:
> > > > nit: It's a bit weird to have this file in ui/gl, and have ui/gl depend
on
> > > > WebKit.
> > > > How about moving this file to webkit/gpu?
> > > 
> > > Oh, somehow I thought cc can't see webkit/gpu, but it can.. I'll try
moving
> it
> > > and see if anything blows up!
> > 
> > Or actually there's a TODO in our DEPS, which I guess was added with
> webkit/gpu
> > since I last looked.
> > 
> > Where else can this class could live then since it will outlive WGC3D?
> 
> James points out it seems like I can just stick this in cc/ now. I'll try
that.

Done that. PTAL

[piman, 2013-02-22 02:54:07]

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:352: return
null_offscreen_context_provider_;
On 2013/02/22 01:56:31, danakj wrote:
> On 2013/02/21 22:49:48, piman wrote:
> > I assume that Android UI doesn't use filters right now?
> 
> Right. And don't intend to use filters that will require this context. I
> confirmed this with @aelias.

Worth a comment? "If you're trying to use filters and the world blows up,
implement this..."

https://codereview.chromium.org/12212007/diff/36108/cc/test/fake_context_prov...
File cc/test/fake_context_provider.h (right):

https://codereview.chromium.org/12212007/diff/36108/cc/test/fake_context_prov...
cc/test/fake_context_provider.h:37: base::Lock destroyed_lock_;
nit: in this case it's pretty explicit, but it's useful to declare which member
it protects. Often we actually declare the lock before the member(s) it
protects, which gives a nice symmetry with the use sites.

class Blah {
  base::Lock lock_;  // protects locked_member_
  Type locked_member_;
};

void ThreadSafeFoo() {
  base::AutoLock lock(lock_);
  locked_member_.Access();
}

Small nits, up to you.

https://codereview.chromium.org/12212007/diff/36108/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/36108/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:32: #include
"webkit/gpu/context_provider.h"
You probably meant to fix this and some other instances below.

[danakj, 2013-02-22 03:04:49]

Done. Also I was tweaking with the unit tests and inspired a small change in the
proxies. When renderer initialization fails, it does a VerifyContexts() on the
provider so that when we try again, the provider can be replaced with a valid
one if the offscreen context was lost. This is now verified by the unit tests
and let me get rid of the extra "set destroyed = true if makeContextCurrent
fails" in the FakeContextProvider that the real ones don't do.

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/37039/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:352: return
null_offscreen_context_provider_;
On 2013/02/22 02:54:07, piman wrote:
> On 2013/02/22 01:56:31, danakj wrote:
> > On 2013/02/21 22:49:48, piman wrote:
> > > I assume that Android UI doesn't use filters right now?
> > 
> > Right. And don't intend to use filters that will require this context. I
> > confirmed this with @aelias.
> 
> Worth a comment? "If you're trying to use filters and the world blows up,
> implement this..."

ya, sure! the world won't blow cuz we nullcheck them, but it'll do nothing.

https://codereview.chromium.org/12212007/diff/36108/cc/test/fake_context_prov...
File cc/test/fake_context_provider.h (right):

https://codereview.chromium.org/12212007/diff/36108/cc/test/fake_context_prov...
cc/test/fake_context_provider.h:37: base::Lock destroyed_lock_;
On 2013/02/22 02:54:07, piman wrote:
> nit: in this case it's pretty explicit, but it's useful to declare which
member
> it protects. Often we actually declare the lock before the member(s) it
> protects, which gives a nice symmetry with the use sites.
> 
> class Blah {
>   base::Lock lock_;  // protects locked_member_
>   Type locked_member_;
> };
> 
> void ThreadSafeFoo() {
>   base::AutoLock lock(lock_);
>   locked_member_.Access();
> }
> 
> Small nits, up to you.

Ah I had started this way and then later moved all the locks above already,
except here. But it's a good point and I'll add whitespace to clarify that in
the command buffer implementation too.

https://codereview.chromium.org/12212007/diff/36108/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/36108/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:32: #include
"webkit/gpu/context_provider.h"
On 2013/02/22 02:54:07, piman wrote:
> You probably meant to fix this and some other instances below.

oops missed that, thanks.

[piman, 2013-02-22 03:16:39]

LGTM, yay!

https://codereview.chromium.org/12212007/diff/51008/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/51008/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:343: // would require
them in this compositor instance. If they are needed,]
nit: typo ']'

https://codereview.chromium.org/12212007/diff/51008/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:354: // would require
them in this compositor instance. If they are needed,]
nit: typo ']'

[danakj, 2013-02-22 03:23:08]

thanks!

https://codereview.chromium.org/12212007/diff/51008/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212007/diff/51008/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:343: // would require
them in this compositor instance. If they are needed,]
On 2013/02/22 03:16:39, piman wrote:
> nit: typo ']'

*^_^*

[jamesr, 2013-02-22 19:11:40]

webkit/ lgtm

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:453: return
WebKit::WebSharedGraphicsContext3D::mainThreadContext();
We also have WebKit::WebView::sharedGraphicsContext3D() that exposes the same
thing and that this class could get access to fairly easily.  We should remove
one of these or the other, someday.

[danakj, 2013-02-22 19:17:52]

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:453: return
WebKit::WebSharedGraphicsContext3D::mainThreadContext();
On 2013/02/22 19:11:40, jamesr wrote:
> We also have WebKit::WebView::sharedGraphicsContext3D() that exposes the same
> thing and that this class could get access to fairly easily.  We should remove
> one of these or the other, someday.

Ohh, okay. Thanks for the heads up.

[danakj, 2013-02-22 19:17:53]

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12212007/diff/45069/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:453: return
WebKit::WebSharedGraphicsContext3D::mainThreadContext();
On 2013/02/22 19:11:40, jamesr wrote:
> We also have WebKit::WebView::sharedGraphicsContext3D() that exposes the same
> thing and that this class could get access to fairly easily.  We should remove
> one of these or the other, someday.

Ohh, okay. Thanks for the heads up.

[commit-bot: I haz the power, 2013-02-22 19:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12212007/45069

[commit-bot: I haz the power, 2013-02-22 20:32:07]

Change committed as 184165