Don't call renderView methods in threaded mode

Don't call renderView methods in threaded mode


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

[nduca, 2011-11-06 23:36:06]

This prevents us from calling renderview->onSwapXXX from the compositor thread,
which is of course bad.

[Ken Russell (switch to Gerrit), 2011-11-09 03:09:42]

LGTM as long as these are the desired semantics for the threaded compositor.

[jamesr, 2011-11-09 04:51:57]

I don't think this is correct

http://codereview.chromium.org/8490001/diff/1/content/renderer/gpu/webgraphic...
File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right):

http://codereview.chromium.org/8490001/diff/1/content/renderer/gpu/webgraphic...
content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:280: web_view_
? RenderViewImpl::FromWebView(web_view_) : NULL;
hold up, this lookup is not threadsafe either.

I thought the idea is that web_view_ will always be NULL in threaded mode and
that was the way we would guard threaded calls.

[nduca, 2011-11-10 04:04:14]

Erm, disregard the coredump. :s

[Ken Russell (switch to Gerrit), 2011-11-10 21:44:42]

lgtm

[jamesr, 2011-11-10 22:23:24]

Sorry but I still don't buy it - I'm almost 100% sure that the WebView*
parameter to initialize is NULL in the threaded case already.  If it's not, then
we should fix that. This should not require any special handling in
MaybeInitializeGL

[jamesr, 2011-11-10 22:23:46]

What's the problem we're trying to solve here - have we been making bad calls,
or is this just a safety thing?

[nduca, 2011-12-13 20:19:11]

I think we should land this. It is causing problems downstream and this isn't
that bad of a change.

[no sievers, 2011-12-13 20:30:09]

web_view_ currently gets set even if the context is running on the compositor
thread from WebGraphicsContext3DCommandBufferImpl::initialize() which is called
on the main thread.

It seems like it would always be bad to have this set for the thread, looking at
the places where it's used.

On 2011/11/10 22:23:24, jamesr wrote:
> Sorry but I still don't buy it - I'm almost 100% sure that the WebView*
> parameter to initialize is NULL in the threaded case already.  If it's not,
then
> we should fix that. This should not require any special handling in
> MaybeInitializeGL

[jamesr, 2011-12-14 00:20:29]

This seems like a reasonable way to get
WebGraphicsContext3DCommandBufferImpl::web_view_ to be NULL in the threaded
case, but I think we can do better - at the callsite to
WebGraphicsContext3D::initialize() we know whether the context will be used from
this thread or not:
http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit...

I think it'd be even better if we passed this state on and
WebGraphicsContext3DCommandBufferImpl::initialize could use that information to
know whether to set the web_view_ pointer or not.

Modulo that, though (which would require rolls, etc) this LGTM.

[commit-bot: I haz the power, 2011-12-15 00:32:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/8490001/15001

[commit-bot: I haz the power, 2011-12-15 02:31:19]

Change committed as 114582