Add ContextProvider overrides to SynchronousCompositorFactory

Add ContextProvider overrides to SynchronousCompositorFactory

SynchronousCompositorFactory does not create the main thread
one since we have not figured out how to get its clients
(video/webgl/canvas) to work in this architecture. This will
cause dynamic but graceful failures when trying to load these
type of contents.

For compositor thread one, create ContextProviderInProcess.

BUG=230197, 239760
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204941

[joth, 2013-06-02 19:08:55]

Starting to make me wonder if we would be better with a synchronous compositor
specific RenderWidgetCompoistor subclass?
This would mean we can easily redirect any cc::LayerTreeHostClient method to
have sync compositor behavior - e.g. CreateOutputSurface() would benefit from
that too. But does turn it into a more complex multi-level inheritance hierarchy
[well, can avoid that with a wrap-and-delegate aggregate LTHC]
Probably best we keep going with the pattern you have here for now and review if
we should refector it once the main functionality hookup is done and has
revealed all the customization points needed.

https://codereview.chromium.org/16235005/diff/1/content/renderer/android/sync...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/android/sync...
content/renderer/android/synchronous_compositor_factory.h:41:
OffscreenContextProviderForCompositorThread() = 0;
Prefix both with Get

https://codereview.chromium.org/16235005/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:128:
parent_context_provider_ = static_cast<ContextProviderCommandBuffer*>(
could we change parent_context_provider_  to be a base-class pointer rather than
cast here? AFAICT the public APIs are the same, so it's just a matter of who
else does this get passed to that also expects subclass pointer...

https://codereview.chromium.org/16235005/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:1008: #if defined(OS_ANDROID)
(hmm at some point we may want to move SynchronousCompositorFactory out of
android folder and let all platforms see the null pointer for it, to avoid
proliferation of ifdef...)

https://codereview.chromium.org/16235005/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:1035: #endif
this means shared_contexts_compositor_thread_ (and shared_contexts_main_thread_)
never get assigned. That seems odd? Is it important this keeps a Ref on the
context, and/or returns the same instance on subsequent calls?

we'll never call set_leak_on_destroy() (which we can live with as we'll never be
running Shutdown on android.)
- however could it call set_leak_on_destroy() here (or even pass to c'tor) and
then store the instance as a base-class pointer again?

[boliu, 2013-06-03 18:51:24]

https://codereview.chromium.org/16235005/diff/1/content/renderer/android/sync...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/android/sync...
content/renderer/android/synchronous_compositor_factory.h:41:
OffscreenContextProviderForCompositorThread() = 0;
On 2013/06/02 19:08:55, joth wrote:
> Prefix both with Get

Done.

https://codereview.chromium.org/16235005/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:128:
parent_context_provider_ = static_cast<ContextProviderCommandBuffer*>(
On 2013/06/02 19:08:55, joth wrote:
> could we change parent_context_provider_  to be a base-class pointer rather
than
> cast here? AFAICT the public APIs are the same, so it's just a matter of who
> else does this get passed to that also expects subclass pointer...

ContextProvider api is the almost same, just the CB provider returns a CB
context. But the public api on the context is not the same: GetImplementation
and GetCommandBufferProxy are not on the in-process context.

I think GetCommandBufferProxy in particular implies cross-process context, and
this has its own command_buffer_ of the same class. So unless we rewrite this
code to not hardcode this class, a cast is needed in this file somewhere.

No idea how hard this rewrite actually is.

https://codereview.chromium.org/16235005/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/16235005/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:1035: #endif
On 2013/06/02 19:08:55, joth wrote:
> this means shared_contexts_compositor_thread_ (and
shared_contexts_main_thread_)
> never get assigned. That seems odd? Is it important this keeps a Ref on the
> context, and/or returns the same instance on subsequent calls?
> 

Yes, it needs to be the same one.

> we'll never call set_leak_on_destroy() (which we can live with as we'll never
be
> running Shutdown on android.)
> - however could it call set_leak_on_destroy() here (or even pass to c'tor) and
> then store the instance as a base-class pointer again?
> 

This was the reason why I'm holding the ref in the factory rather than here,
because the type is different.

Setting set_leak_on_destroy at contruction time is wrong because it would be
leaking things when the context is lost, which can happen multiple times before
shutdown.

For webview we don't have to worry about it because we never clean have clean
shutdowns.

[boliu, 2013-06-07 17:17:09]

James, could you take a look at this? The cast in
pepper_platform_context_3d_impl.cc is no longer needed since that code has been
removed in https://chromiumcodereview.appspot.com/16290005. And returning
cc::ContextProvider is probably a good thing to avoid adding usage of methods
only in the subclass.

[jamesr, 2013-06-07 17:20:14]

lgtm

[joth, 2013-06-07 17:41:17]

lgtm

https://codereview.chromium.org/16235005/diff/9001/content/browser/android/in...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/16235005/diff/9001/content/browser/android/in...
content/browser/android/in_process/synchronous_compositor_impl.cc:56: //
Synchronous compositor does not support main thread context yet.
NOTIMPLEMENTED() ?

https://codereview.chromium.org/16235005/diff/9001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/16235005/diff/9001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_factory.h:17: class
ContextProvider;
alpha order

[commit-bot: I haz the power, 2013-06-07 21:47:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/16235005/15002

[commit-bot: I haz the power, 2013-06-07 21:48:50]

Change committed as 204941