Remove WebGraphicsContext3D getter from cc::ContextProvider

Remove WebGraphicsContext3D getter from cc::ContextProvider

The compositor uses GLES2Interface to talk to the OpenGL context now, so
the WebGraphicsContext3D getter isn't applicable at this layer.

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

[jamesr, 2014-01-09 20:57:48]

piman - please review content/
dana - please review cc/ (mostly test changes and clang-format)S
sievers - fyi on some of the android changes

This depends on https://codereview.chromium.org/126093010/

[danakj, 2014-01-09 21:23:34]

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc#n...
cc/test/layer_tree_test.cc:697: return static_cast<TestContextProvider*>(
can you DCHECK we're on the impl side here?

or maybe even better would be to make this method take a LTHI* and have it pull
the context off of there. Then it's obvious where you are. (I'd like the
output_surface() method to do the same and remove the output_surface_ member
here entirely..)

This would also let you get rid of this extra indirection with
CreateFakeOutputSurfaceForTest().

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.h#ne...
cc/test/layer_tree_test.h:193: FakeOutputSurface* output_surface_;
Why public?

https://codereview.chromium.org/132163004/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/132163004/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:36: if
(context_provider_->IsContextLost())
It's kinda weird to have a NULL pointer due to context loss, as that's usually
not what we do.

I see how you're doing this to branch where we used to branch on
makeContextCurrent() because the provider is not exposed only the ContextGL is.

If you're going to do this can you update the comment on the base class to warn
about NULL return values? Maybe it'd be better to just return the
ContextProvider instead of the ContextGL here?

[jamesr, 2014-01-10 00:12:33]

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc#n...
cc/test/layer_tree_test.cc:697: return static_cast<TestContextProvider*>(
On 2014/01/09 21:23:34, danakj wrote:
> can you DCHECK we're on the impl side here?
> 
> or maybe even better would be to make this method take a LTHI* and have it
pull
> the context off of there. Then it's obvious where you are. (I'd like the
> output_surface() method to do the same and remove the output_surface_ member
> here entirely..)

That'd mean downcasting here instead of relying on pointers that are already of
the right type.  I'm a little wary of doing that for tests that might want to
make their own OutputSurface (by overriding CreateOutputSurface directly) and
then trying to downcast to a FakeOutputSurface, which is an issue I hit on some
tests.  By having the CreateFakeOutput...() function be what actually sets
output_surface_ the member is either always NULL or actually of type
FakeOutputSurface.

> 
> This would also let you get rid of this extra indirection with
> CreateFakeOutputSurfaceForTest().

https://codereview.chromium.org/132163004/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/132163004/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:36: if
(context_provider_->IsContextLost())
On 2014/01/09 21:23:34, danakj wrote:
> It's kinda weird to have a NULL pointer due to context loss, as that's usually
> not what we do.
> 
> I see how you're doing this to branch where we used to branch on
> makeContextCurrent() because the provider is not exposed only the ContextGL
is.
> 
> If you're going to do this can you update the comment on the base class to
warn
> about NULL return values? Maybe it'd be better to just return the
> ContextProvider instead of the ContextGL here?

sievers@ - do you know why StreamTextureFactory uses a raw context pointer
instead of a ref to a ContextProvider?  I think using a provider would make a
bit more sense.  I just tried to preserve existing behavior here.

[jamesr, 2014-01-10 00:56:20]

Looking more closely the lost context checks on the StreamTextureFactory's
context all seem bogus - removed in the latest PS and fixed up
StreamTextureFactoryImpl to retain a scoped_refptr<cc::ContextProvider> instead
of a raw context pointer to be safer.

PTAL.

[danakj, 2014-01-13 19:36:42]

LGTM

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/132163004/diff/1/cc/test/layer_tree_test.cc#n...
cc/test/layer_tree_test.cc:697: return static_cast<TestContextProvider*>(
On 2014/01/10 00:12:34, jamesr wrote:
> On 2014/01/09 21:23:34, danakj wrote:
> > can you DCHECK we're on the impl side here?
> > 
> > or maybe even better would be to make this method take a LTHI* and have it
> pull
> > the context off of there. Then it's obvious where you are. (I'd like the
> > output_surface() method to do the same and remove the output_surface_ member
> > here entirely..)
> 
> That'd mean downcasting here instead of relying on pointers that are already
of
> the right type.  I'm a little wary of doing that for tests that might want to
> make their own OutputSurface (by overriding CreateOutputSurface directly) and
> then trying to downcast to a FakeOutputSurface, which is an issue I hit on
some
> tests.  By having the CreateFakeOutput...() function be what actually sets
> output_surface_ the member is either always NULL or actually of type
> FakeOutputSurface.

I see, that's a good point.

[jamesr, 2014-01-14 21:26:04]

piman@ - could you PTAL at the content/ changes?  All of the dependencies of
this patch have landed now.

[piman, 2014-01-14 22:28:45]

lgtm

[commit-bot: I haz the power, 2014-01-14 23:14:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/132163004/380001

[commit-bot: I haz the power, 2014-01-15 02:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/132163004/380001

[commit-bot: I haz the power, 2014-01-15 17:52:16]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-15 18:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/132163004/380001

[commit-bot: I haz the power, 2014-01-16 00:11:13]

Change committed as 245026