Remove static thread pointers from CC

Remove static thread pointers from CC, attempt 3

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

[aelias_OOO_until_Jul13, 2012-10-22 21:49:07]

Hi James, this is an early, still-broken version of this patch so you can check
if the general approach is what we want.  Basically I introduce a struct held by
WebCompositorSupportImpl with a weak pointer passed to CCProxy base class, and
the rest of the compositor talks to CCProxy.  No WebKit-side changes will be
needed (except for deleting WebCompositor class later).

[jamesr, 2012-10-22 22:00:35]

I like making LayerTreeHost / LayerTreeHostImpl have explicit CCProxy pointers.
I don't like adding CCProxy pointers to classes that only care about "am I in
threaded mode?" and/or only want to do DCHECK()s.  It would be much better to
pass a bool down to these classes if they just care about if they are in
threaded mode.  You only need a proxy pointer if you are going to actually post
a task.  By that reasoning, I think you should kill the CCProxy*s from the
Resource classes but you will probably have to leave it on RateLimiter. 
Alternately, move the task posting logic on RateLimiter to go through its
client.

https://codereview.chromium.org/11232051/diff/1/cc/compositor_support_state.h
File cc/compositor_support_state.h (right):

https://codereview.chromium.org/11232051/diff/1/cc/compositor_support_state.h...
cc/compositor_support_state.h:12: struct CompositorSupportState {
Not sure you really need this struct - most users seem to only care about one
thread or the other (nearly always just the implThread).  Since our
initialization paths are always on the mainThread, passing that down through the
callstack typically isn't necessary either - we can just pick up the pointer in
whatever step needs it (CCProxy initialization typically).  What about just
passing an implThread pointer down?

https://codereview.chromium.org/11232051/diff/1/cc/direct_renderer.h
File cc/direct_renderer.h (right):

https://codereview.chromium.org/11232051/diff/1/cc/direct_renderer.h#newcode98
cc/direct_renderer.h:98: CCProxy* m_proxy;
why are you adding a proxy pointer to this class? it seems unused (and it really
doesn't make sense in the class hierarchy)

https://codereview.chromium.org/11232051/diff/1/cc/font_atlas.cc
File cc/font_atlas.cc (right):

https://codereview.chromium.org/11232051/diff/1/cc/font_atlas.cc#newcode34
cc/font_atlas.cc:34: //    DCHECK(CCProxy::isImplThread());
just nuking these dchecks would be OK, IMO

https://codereview.chromium.org/11232051/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11232051/diff/1/cc/layer_tree_host.cc#newcode121
cc/layer_tree_host.cc:121: if (compositorThread)
i can't tell where this bool is coming from

https://codereview.chromium.org/11232051/diff/1/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11232051/diff/1/cc/layer_tree_host.h#newcode26
cc/layer_tree_host.h:26: #include <public/WebThread.h>
this isn't good - we should be removing WebKit dependencies from cc/, not adding
more. can this class the cc::CCThread type instead?

https://codereview.chromium.org/11232051/diff/1/cc/proxy.h
File cc/proxy.h (right):

https://codereview.chromium.org/11232051/diff/1/cc/proxy.h#newcode12
cc/proxy.h:12: #include <public/WebThread.h>
again, not great :( cc::CCThread?

[aelias_OOO_until_Jul13, 2012-10-22 22:09:58]

Thanks for the quick feedback.  OK, I'll pass around booleans and use CCThread
where possible.

https://codereview.chromium.org/11232051/diff/1/cc/compositor_support_state.h
> File cc/compositor_support_state.h (right):
> 
>
https://codereview.chromium.org/11232051/diff/1/cc/compositor_support_state.h...
> cc/compositor_support_state.h:12: struct CompositorSupportState {
> Not sure you really need this struct - most users seem to only care about one
> thread or the other (nearly always just the implThread).  Since our
> initialization paths are always on the mainThread, passing that down through
the
> callstack typically isn't necessary either - we can just pick up the pointer
in
> whatever step needs it (CCProxy initialization typically).  What about just
> passing an implThread pointer down?

I figured we can later move the settings into this struct as well.  That would
make it possible to have different settings for UI and renderer compositors in
single-process mode.  Seem reasonable?

[jamesr, 2012-10-22 22:33:12]

On 2012/10/22 22:09:58, aelias wrote:
> I figured we can later move the settings into this struct as well.  That would
> make it possible to have different settings for UI and renderer compositors in
> single-process mode.  Seem reasonable?

We have a settings struct in CC already (two of them, really).  I'd prefer to
start without a struct and add it if/when it makes sense.

[aelias_OOO_until_Jul13, 2012-10-25 04:06:43]

PTAL, this is getting into a landable state.  cc_unittests now pass in a debug
build.

[danakj, 2012-10-25 05:06:06]

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/gl_renderer.cc#n...
cc/gl_renderer.cc:1133: onMemoryAllocationChangedOnImplThread(allocation);
Is there a reason to keep the OnImplThread function then?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/gl_renderer_unit...
File cc/gl_renderer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/gl_renderer_unit...
cc/gl_renderer_unittest.cc:47: //        DebugScopedSetMainThread main;  //
TODO: eviction
Left a comment in here by mistake?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/layer_tree_host....
cc/layer_tree_host.h:93: static scoped_ptr<LayerTreeHost>
create(LayerTreeHostClient*, const LayerTreeSettings&, Thread*);
add "implThread" name here? The Thread* isn't super obvious.

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/prioritized_text...
File cc/prioritized_texture.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/prioritized_text...
cc/prioritized_texture.cc:180: DCHECK(m_threadChecker.CalledOnValidThread());
The main thread is blocked part here is kinda important, I'm not sure it makes
sense to remove that.

How about adding a for-debug method on PTM and have this call that? Since PTM
has a Proxy*

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/prioritized_text...
cc/prioritized_texture.cc:192: DCHECK(m_threadChecker.CalledOnValidThread());
ditto

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/proxy.cc
File cc/proxy.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/proxy.cc#newcode105
cc/proxy.cc:105: {
DCHECK(m_mainThread != m_implThread) ?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provide...
cc/resource_provider.h:8: #include "IntRect.h"
?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/single_thread_pr...
File cc/single_thread_proxy.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/single_thread_pr...
cc/single_thread_proxy.h:126: m_proxy->setCurrentThreadIsImplThread(true);
set to m_previousValue?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/test/layer_tree_...
File cc/test/layer_tree_test_common.cc (left):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/test/layer_tree_...
cc/test/layer_tree_test_common.cc:393: DCHECK(Proxy::isMainThread());
Why not call proxy()->isMainThread() etc for all these checks in this file?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tiled_layer_unit...
File cc/tiled_layer_unittest.cc (left):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tiled_layer_unit...
cc/tiled_layer_unittest.cc:861: settings.maxPartialTextureUpdates = 4;
Don't really like this being set for the whole test suite instead of inside this
test.. it makes it super less clear what this test is testing for.

Can you make this test use a subclass of TiledLayerTest that sets this partial
texture update limit?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tree_synchronize...
File cc/tree_synchronizer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tree_synchronize...
cc/tree_synchronizer_unittest.cc:145: DebugScopedSetImplThread impl(&proxy);
Why do we need to be impl thread for these tests?

LTHI::create() is called from LTH on the main thread. TreeSynchronizer isn't
asserting about thread the thread, nor layer?

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/video_layer_impl.h
File cc/video_layer_impl.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/video_layer_impl...
cc/video_layer_impl.h:10: #include "base/threading/thread_checker.h"
unused header?

[aelias_OOO_until_Jul13, 2012-10-25 06:11:06]

OK, all done except for comments below.

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provide...
cc/resource_provider.h:8: #include "IntRect.h"
On 2012/10/25 05:06:06, danakj wrote:
> ?

There's a mistaken forward-declaration of IntRect inside cc namespace below
which caused a compile error for me.  This is a drive-by fix.

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tree_synchronize...
File cc/tree_synchronizer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/tree_synchronize...
cc/tree_synchronizer_unittest.cc:145: DebugScopedSetImplThread impl(&proxy);
On 2012/10/25 05:06:06, danakj wrote:
> Why do we need to be impl thread for these tests?
> 
> LTHI::create() is called from LTH on the main thread. TreeSynchronizer isn't
> asserting about thread the thread, nor layer?

LTHI constructor asserts that it's on the impl thread.

[aelias_OOO_until_Jul13, 2012-10-25 06:30:08]

Also, I replaced WebThread with CCThreadImpl but now I get a DEPS error that CC
cannot depend on webkit/compositor_bindings (in order to create the main-thread
CCThreadImpl).  Any suggestions?

[danakj, 2012-10-25 14:59:50]

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/7001/cc/resource_provide...
cc/resource_provider.h:8: #include "IntRect.h"
On 2012/10/25 06:11:06, aelias wrote:
> On 2012/10/25 05:06:06, danakj wrote:
> > ?
> 
> There's a mistaken forward-declaration of IntRect inside cc namespace below
> which caused a compile error for me.  This is a drive-by fix.

Hm, well FWIW, this file also includes cc::IntRect from cc/stubs/

[danakj, 2012-10-25 15:03:02]

Thanks for the changes, I like that we have removed fewer dchecks now. lgtm if
jamesr likes it.

Last thought, do you think that PTM::proxyForDebug(), and the DCHECKS that use
it, should be inside NDEBUG guards? Typically we're doing that when adding
functions/members just for dchecks.

[enne (OOO), 2012-10-25 17:28:24]

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/gl_renderer.h#n...
cc/gl_renderer.h:36: static scoped_ptr<GLRenderer> create(RendererClient*,
ResourceProvider*, bool hasImplThread);
Could you put hasImplThread in the RendererClient interface instead? That seems
like a better place to put it.

[jamesr, 2012-10-25 19:42:39]

Let's not add a webkit/compositor_bindings/ dependency for this.  If you follow
the chain all the way through, cc::Thread is implemented on top of
WebKit::CCThreadImpl which is implemented on top of WebThread which is
implemented on top of base::MessageLoopProxy.  If you cut out all the middle
layers, I believe you can implement everything you need really easily on top of
base::MessageLoopProxy and not need any of the other layers.

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/proxy.cc
File cc/proxy.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/proxy.cc#newcode9
cc/proxy.cc:9: #include "webkit/compositor_bindings/ccthread_impl.h"
this doesn't seem right at all - everything you need should be on the cc::Thread
interface, you shouldn't need to care about the implementation

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/proxy.cc#newcode99
cc/proxy.cc:99: : m_mainThread(WebKit::CCThreadImpl::createForCurrentThread())
If this is the only call you need from CCThreadImpl I think you should either
figure out a way to implement the call just from cc:: and base:: or route the
main thread pointer down from webkit/compositor_bindings

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/test/layer_tree...
File cc/test/layer_tree_test_common.cc (right):

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/test/layer_tree...
cc/test/layer_tree_test_common.cc:29: #include
"webkit/compositor_bindings/ccthread_impl.h"
this is also not good

https://chromiumcodereview.appspot.com/11232051/diff/15001/cc/test/layer_tree...
cc/test/layer_tree_test_common.cc:551: m_implThread =
CCThreadImpl::createForDifferentThread(m_webImplThread.get()).release();
instead of relying on WebKit::Platform to get a WebThread and then wrapping it,
you do this all from base

[jamesr, 2012-10-30 23:02:49]

If you rebase on top of
http://src.chromium.org/viewvc/chrome?view=rev&revision=165050 some of this
should get easier (in particular dealing with cc::Threads)

[jamesr, 2012-10-30 23:32:16]

On 2012/10/30 23:02:49, jamesr wrote:
> If you rebase on top of
> http://src.chromium.org/viewvc/chrome?view=rev&revision=165050 some of this
> should get easier (in particular dealing with cc::Threads)

Sorry, make that '060.  Got reverted for a silly reason

[aelias_OOO_until_Jul13, 2012-10-31 03:51:24]

Rebased on the cc::ThreadImpl change.


On 2012/10/25 15:03:02, danakj wrote:
> Last thought, do you think that PTM::proxyForDebug(), and the DCHECKS that use
> it, should be inside NDEBUG guards? Typically we're doing that when adding
> functions/members just for dchecks.

I'd rather not add more NDEBUG guards.  I don't like how they can cause only one
build type but not the other to fail.

[jamesr, 2012-10-31 04:42:12]

Looking close!  Try to use NULL instead of 0 for null pointers.

I think there are some cc::Thread ownership issues in this patch but I think
they are fairly easy to fix.

https://codereview.chromium.org/11232051/diff/23001/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/gl_renderer.h#newcode184
cc/gl_renderer.h:184: bool m_hasImplThread;
it looks like the code in gl_renderer.cc is calling
RendererClient::hasImplThread(), do ya need this bool member still?

https://codereview.chromium.org/11232051/diff/23001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11232051/diff/23001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:135: DCHECK(m_proxy.get());
I think DCHECK(m_proxy) works too

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.cc
File cc/proxy.cc (right):

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.cc#newcode26
cc/proxy.cc:26: return !!m_implThread.get();
return m_implThread; will do the right thing

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.h
File cc/proxy.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.h#newcode37
cc/proxy.h:37: void setMainThread(Thread*);
hmm, why do we need a setter for the main thread? cc::Proxy's constructor can
just create one around the current thread

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.h#newcode38
cc/proxy.h:38: void setImplThread(Thread*);
since these setters transfer ownership, they should take base::scoped_ptr<>s

instead of having setters, could we make these pointers construction parameters
of SingleThreadProxy and ThreadProxy?

https://codereview.chromium.org/11232051/diff/23001/cc/rate_limiter.cc
File cc/rate_limiter.cc (right):

https://codereview.chromium.org/11232051/diff/23001/cc/rate_limiter.cc#newcode41
cc/rate_limiter.cc:41:
m_proxy->mainThread()->postTask(base::Bind(&RateLimiter::rateLimitContext,
this));
could we just give RateLimit a cc::Thread* instead of a proxy?

https://codereview.chromium.org/11232051/diff/23001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/resource_provider.h#ne...
cc/resource_provider.h:249: ResourceProvider(GraphicsContext*);
think we still want explicit

https://codereview.chromium.org/11232051/diff/23001/cc/single_thread_proxy.h
File cc/single_thread_proxy.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/single_thread_proxy.h#...
cc/single_thread_proxy.h:94: DebugScopedSetImplThread(Proxy* proxy)
explicit

https://codereview.chromium.org/11232051/diff/23001/cc/single_thread_proxy.h#...
cc/single_thread_proxy.h:117: DebugScopedSetMainThread(Proxy* proxy)
explicit

https://codereview.chromium.org/11232051/diff/23001/cc/single_thread_proxy.h#...
cc/single_thread_proxy.h:141:
DebugScopedSetImplThreadAndMainThreadBlocked(Proxy* proxy)
explicit

https://codereview.chromium.org/11232051/diff/23001/cc/test/fake_proxy.h
File cc/test/fake_proxy.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/test/fake_proxy.h#newc...
cc/test/fake_proxy.h:18: FakeProxy(Thread* implThread) : Proxy(implThread) { }
explicit

https://codereview.chromium.org/11232051/diff/23001/cc/test/layer_tree_test_c...
File cc/test/layer_tree_test_common.cc (right):

https://codereview.chromium.org/11232051/diff/23001/cc/test/layer_tree_test_c...
cc/test/layer_tree_test_common.cc:354: if (m_implThread.get())
if (m_implThread)

https://codereview.chromium.org/11232051/diff/23001/cc/test/run_all_unittests.cc
File cc/test/run_all_unittests.cc (left):

https://codereview.chromium.org/11232051/diff/23001/cc/test/run_all_unittests...
cc/test/run_all_unittests.cc:15: scoped_ptr<cc::Thread> mainCCThread =
cc::ThreadImpl::createForCurrentThread();
drop the cc #includes too.  if base/test/run_all_unittests.cc spun up a message
loop we could just use that - any idea if it does?

https://codereview.chromium.org/11232051/diff/23001/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/11232051/diff/23001/cc/tiled_layer_unittest.c...
cc/tiled_layer_unittest.cc:1522: {
{ goes on the previous line in chromium and WebKit style

https://codereview.chromium.org/11232051/diff/23001/webkit/compositor_binding...
File webkit/compositor_bindings/web_compositor_support_impl.cc (right):

https://codereview.chromium.org/11232051/diff/23001/webkit/compositor_binding...
webkit/compositor_bindings/web_compositor_support_impl.cc:94: if
(!layerTreeViewImpl->initialize(settings, impl_thread_))
this passes ownership of impl_thread_ to the cc::LayerTreeHost, since
cc::LayerTreeHost has a scoped_ptr<cc::Proxy> and cc::Proxy has a
scoped_ptr<cc::Thread>

I think you need to keep something retained on the WebCompositorSupportImpl and
then construct instances of cc::Thread to pass to each cc::LayerTreeView. 
probably extracting + keeping a scoped_refptr<MessageLoopProxy> in
::initialize() and calling cc::ThreadImpl::createFor...() here

[aelias_OOO_until_Jul13, 2012-10-31 06:15:30]

OK, all feedback applied.

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.h
File cc/proxy.h (right):

https://codereview.chromium.org/11232051/diff/23001/cc/proxy.h#newcode38
cc/proxy.h:38: void setImplThread(Thread*);
On 2012/10/31 04:42:13, jamesr wrote:
> since these setters transfer ownership, they should take base::scoped_ptr<>s
> 
> instead of having setters, could we make these pointers construction
parameters
> of SingleThreadProxy and ThreadProxy?

The setters were unused residue.  I deleted both of them.

https://codereview.chromium.org/11232051/diff/23001/cc/test/run_all_unittests.cc
File cc/test/run_all_unittests.cc (left):

https://codereview.chromium.org/11232051/diff/23001/cc/test/run_all_unittests...
cc/test/run_all_unittests.cc:15: scoped_ptr<cc::Thread> mainCCThread =
cc::ThreadImpl::createForCurrentThread();
On 2012/10/31 04:42:13, jamesr wrote:
> drop the cc #includes too.  if base/test/run_all_unittests.cc spun up a
message
> loop we could just use that - any idea if it does?

Nope, it doesn't.

[jamesr, 2012-10-31 06:38:19]

I think you should use scoped_ptr<cc::Thread> as the parameter type for all the
functions that transfer ownership through the stack.  I think the full path for
this looks something like: WebCompositorSupportImpl -> WebLayerTreeViewImpl ->
LayerTreeHost -> ThreadProxy -> Proxy.  You'll have to add .Pass() in various
places, but having the ownership transfer in the type system makes this a lot
easier to understand IMO and avoids the class of problems patch set 6 had
completely.  The only place we should be using raw pointers is for non-ownership
transfer functions (like getters) or when going through WebKit API, since it
doesn't understand scoped_ptr

Other than that lgtm

https://codereview.chromium.org/11232051/diff/32002/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11232051/diff/32002/cc/layer_tree_host.h#newc...
cc/layer_tree_host.h:95: static scoped_ptr<LayerTreeHost>
create(LayerTreeHostClient*, const LayerTreeSettings&, Thread* implThread);
scoped_ptr<Thread> would be better since it takes ownership

https://codereview.chromium.org/11232051/diff/32002/webkit/compositor_binding...
File webkit/compositor_bindings/web_compositor_support_impl.cc (right):

https://codereview.chromium.org/11232051/diff/32002/webkit/compositor_binding...
webkit/compositor_bindings/web_compositor_support_impl.cc:61:
is_threading_enabled_ = true;
i think the is_threading_enabled_ bool is redundant with
impl_thread_message_loop_proxy_. if the latter is null, threading isn't enabled.
it's a caller error to call any functions on WebCompositorSupportImpl before
initialize()

https://codereview.chromium.org/11232051/diff/32002/webkit/compositor_binding...
webkit/compositor_bindings/web_compositor_support_impl.cc:64:
message_loop()->message_loop_proxy();
indent this line another 4 spaces since it's an additional continuation. might
be worth having a temp var here or two just to cut down on the epicness of the
line

https://codereview.chromium.org/11232051/diff/32002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11232051/diff/32002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_tree_view_impl.h:27: bool initialize(const
Settings&, cc::Thread*);
second parameter should be type scoped_ptr<cc::Thread> since it takes ownership
of the value

[aelias_OOO_until_Jul13, 2012-10-31 20:20:04]

Ownership issues fixed, I'll make the trybots happy now and try to land.

[commit-bot: I haz the power, 2012-10-31 20:36:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/23009

[commit-bot: I haz the power, 2012-10-31 22:32:03]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-31 22:42:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/23009

[commit-bot: I haz the power, 2012-10-31 22:42:48]

Failed to apply patch for cc/prioritized_texture_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/prioritized_texture_manager.cc
  Hunk #16 FAILED at 390.
  1 out of 19 hunks FAILED -- saving rejects to file
cc/prioritized_texture_manager.cc.rej

Patch:       cc/prioritized_texture_manager.cc
Index: cc/prioritized_texture_manager.cc
diff --git a/cc/prioritized_texture_manager.cc
b/cc/prioritized_texture_manager.cc
index
79ecbc549a93d5b3d1957a1377ad5f34081282b1..ee8b43a786b9abfe989d70f639b7048d8d077d40
100644
--- a/cc/prioritized_texture_manager.cc
+++ b/cc/prioritized_texture_manager.cc
@@ -17,8 +17,9 @@ using namespace std;
 
 namespace cc {
 
-PrioritizedTextureManager::PrioritizedTextureManager(size_t
maxMemoryLimitBytes, int, int pool)
-    : m_maxMemoryLimitBytes(maxMemoryLimitBytes)
+PrioritizedTextureManager::PrioritizedTextureManager(size_t
maxMemoryLimitBytes, int, int pool, const Proxy* proxy)
+    : m_proxy(proxy)
+    , m_maxMemoryLimitBytes(maxMemoryLimitBytes)
     , m_externalPriorityCutoff(PriorityCalculator::allowEverythingCutoff())
     , m_memoryUseBytes(0)
     , m_memoryAboveCutoffBytes(0)
@@ -46,20 +47,20 @@ PrioritizedTextureManager::~PrioritizedTextureManager()
 
 size_t PrioritizedTextureManager::memoryVisibleBytes() const
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     return m_memoryVisibleLastPushedBytes;
 }
 
 size_t PrioritizedTextureManager::memoryVisibleAndNearbyBytes() const
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     return m_memoryVisibleAndNearbyLastPushedBytes;
 }
 
 void PrioritizedTextureManager::prioritizeTextures()
 {
     TRACE_EVENT0("cc", "PrioritizedTextureManager::prioritizeTextures");
-    DCHECK(Proxy::isMainThread());
+    DCHECK(m_proxy->isMainThread());
 
     // Sorting textures in this function could be replaced by a slightly
     // modified O(n) quick-select to partition textures rather than
@@ -135,7 +136,7 @@ void PrioritizedTextureManager::prioritizeTextures()
 void PrioritizedTextureManager::pushTexturePrioritiesToBackings()
 {
     TRACE_EVENT0("cc",
"PrioritizedTextureManager::pushTexturePrioritiesToBackings");
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
 
     assertInvariants();
     for (BackingList::iterator it = m_backings.begin(); it != m_backings.end();
++it)
@@ -151,7 +152,7 @@ void
PrioritizedTextureManager::pushTexturePrioritiesToBackings()
 void PrioritizedTextureManager::updateBackingsInDrawingImplTree()
 {
     TRACE_EVENT0("cc",
"PrioritizedTextureManager::updateBackingsInDrawingImplTree");
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
 
     assertInvariants();
     for (BackingList::iterator it = m_backings.begin(); it != m_backings.end();
++it) {
@@ -165,7 +166,7 @@ void
PrioritizedTextureManager::updateBackingsInDrawingImplTree()
 void PrioritizedTextureManager::sortBackings()
 {
     TRACE_EVENT0("cc", "PrioritizedTextureManager::sortBackings");
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
 
     // Put backings in eviction/recycling order.
     m_backings.sort(compareBackings);
@@ -174,7 +175,7 @@ void PrioritizedTextureManager::sortBackings()
 
 void PrioritizedTextureManager::clearPriorities()
 {
-    DCHECK(Proxy::isMainThread());
+    DCHECK(m_proxy->isMainThread());
     for (TextureSet::iterator it = m_textures.begin(); it != m_textures.end();
++it) {
         // FIXME: We should remove this and just set all priorities to
         //        PriorityCalculator::lowestPriority() once we have priorities
@@ -186,7 +187,7 @@ void PrioritizedTextureManager::clearPriorities()
 
 bool PrioritizedTextureManager::requestLate(PrioritizedTexture* texture)
 {
-    DCHECK(Proxy::isMainThread());
+    DCHECK(m_proxy->isMainThread());
 
     // This is already above cutoff, so don't double count it's memory below.
     if (texture->isAbovePriorityCutoff())
@@ -211,7 +212,7 @@ bool
PrioritizedTextureManager::requestLate(PrioritizedTexture* texture)
 
 void
PrioritizedTextureManager::acquireBackingTextureIfNeeded(PrioritizedTexture*
texture, ResourceProvider* resourceProvider)
 {
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
     DCHECK(!texture->isSelfManaged());
     DCHECK(texture->isAbovePriorityCutoff());
     if (texture->backing() || !texture->isAbovePriorityCutoff())
@@ -251,7 +252,7 @@ void
PrioritizedTextureManager::acquireBackingTextureIfNeeded(PrioritizedTexture
 
 bool PrioritizedTextureManager::evictBackingsToReduceMemory(size_t limitBytes,
int priorityCutoff, EvictionPolicy evictionPolicy, ResourceProvider*
resourceProvider)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     if (memoryUseBytes() <= limitBytes &&
PriorityCalculator::allowEverythingCutoff() == priorityCutoff)
         return false;
 
@@ -271,7 +272,7 @@ bool
PrioritizedTextureManager::evictBackingsToReduceMemory(size_t limitBytes, i
 
 void PrioritizedTextureManager::reduceMemory(ResourceProvider*
resourceProvider)
 {
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
 
     // Note that it will not always be the case that memoryUseBytes() <=
maxMemoryLimitBytes(),
     // because we are not at liberty to delete textures that are referenced by
the impl tree to 
@@ -306,14 +307,14 @@ void
PrioritizedTextureManager::reduceMemory(ResourceProvider* resourceProvider)
 
 void PrioritizedTextureManager::clearAllMemory(ResourceProvider*
resourceProvider)
 {
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
     DCHECK(resourceProvider);
     evictBackingsToReduceMemory(0, PriorityCalculator::allowEverythingCutoff(),
EvictAnything, resourceProvider);
 }
 
 bool PrioritizedTextureManager::reduceMemoryOnImplThread(size_t limitBytes, int
priorityCutoff, ResourceProvider* resourceProvider)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     DCHECK(resourceProvider);
     // If we are in the process of uploading a new frame then the backings at
the very end of
     // the list are not sorted by priority. Sort them before doing the
eviction.
@@ -324,14 +325,14 @@ bool
PrioritizedTextureManager::reduceMemoryOnImplThread(size_t limitBytes, int
 
 void PrioritizedTextureManager::getEvictedBackings(BackingList&
evictedBackings)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     evictedBackings.clear();
     evictedBackings.insert(evictedBackings.begin(), m_evictedBackings.begin(),
m_evictedBackings.end());
 }
 
 void PrioritizedTextureManager::unlinkEvictedBackings(const BackingList&
evictedBackings)
 {
-    DCHECK(Proxy::isMainThread());
+    DCHECK(m_proxy->isMainThread());
     for (BackingList::const_iterator it = evictedBackings.begin(); it !=
evictedBackings.end(); ++it) {
         PrioritizedTexture::Backing* backing = (*it);
         if (backing->owner())
@@ -341,7 +342,7 @@ void PrioritizedTextureManager::unlinkEvictedBackings(const
BackingList& evicted
 
 void PrioritizedTextureManager::deleteUnlinkedEvictedBackings()
 {
-    DCHECK(Proxy::isMainThread() || (Proxy::isImplThread() &&
Proxy::isMainThreadBlocked()));
+    DCHECK(m_proxy->isMainThread() || (m_proxy->isImplThread() &&
m_proxy->isMainThreadBlocked()));
     BackingList newEvictedBackings;
     for (BackingList::const_iterator it = m_evictedBackings.begin(); it !=
m_evictedBackings.end(); ++it) {
         PrioritizedTexture::Backing* backing = (*it);
@@ -364,7 +365,7 @@ bool PrioritizedTextureManager::linkedEvictedBackingsExist()
const
 
 void PrioritizedTextureManager::registerTexture(PrioritizedTexture* texture)
 {
-    DCHECK(Proxy::isMainThread());
+    DCHECK(m_proxy->isMainThread());
     DCHECK(texture);
     DCHECK(!texture->textureManager());
     DCHECK(!texture->backing());
@@ -377,7 +378,7 @@ void
PrioritizedTextureManager::registerTexture(PrioritizedTexture* texture)
 
 void PrioritizedTextureManager::unregisterTexture(PrioritizedTexture* texture)
 {
-    DCHECK(Proxy::isMainThread() || (Proxy::isImplThread() &&
Proxy::isMainThreadBlocked()));
+    DCHECK(m_proxy->isMainThread() || (m_proxy->isImplThread() &&
m_proxy->isMainThreadBlocked()));
     DCHECK(texture);
     DCHECK(ContainsKey(m_textures, texture));
 
@@ -389,14 +390,14 @@ void
PrioritizedTextureManager::unregisterTexture(PrioritizedTexture* texture)
 
 void PrioritizedTextureManager::returnBackingTexture(PrioritizedTexture*
texture)
 {
-    DCHECK(Proxy::isMainThread() || (Proxy::isImplThread() &&
Proxy::isMainThreadBlocked()));
+    DCHECK(m_proxy->isMainThread() || (m_proxy->isImplThread() &&
m_proxy->isMainThreadBlocked()));
     if (texture->backing())
         texture->unlink();
 }
 
 PrioritizedTexture::Backing* PrioritizedTextureManager::createBacking(IntSize
size, GLenum format, ResourceProvider* resourceProvider)
 {
-    DCHECK(Proxy::isImplThread() && Proxy::isMainThreadBlocked());
+    DCHECK(m_proxy->isImplThread() && m_proxy->isMainThreadBlocked());
     DCHECK(resourceProvider);
     ResourceProvider::ResourceId resourceId =
resourceProvider->createResource(m_pool, size, format,
ResourceProvider::TextureUsageAny);
     PrioritizedTexture::Backing* backing = new
PrioritizedTexture::Backing(resourceId, resourceProvider, size, format);
@@ -406,7 +407,7 @@ PrioritizedTexture::Backing*
PrioritizedTextureManager::createBacking(IntSize si
 
 void PrioritizedTextureManager::evictFirstBackingResource(ResourceProvider*
resourceProvider)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_proxy->isImplThread());
     D…
(message too large)

[commit-bot: I haz the power, 2012-10-31 22:51:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/27005

[commit-bot: I haz the power, 2012-11-01 00:57:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 04:47:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/28030

[commit-bot: I haz the power, 2012-11-01 08:58:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 17:18:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/9208

[commit-bot: I haz the power, 2012-11-01 18:44:58]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-01 19:17:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/35079

[egraether, 2012-11-01 21:38:17]

https://codereview.chromium.org/11232051/diff/35079/cc/frame_rate_counter.cc
File cc/frame_rate_counter.cc (right):

https://codereview.chromium.org/11232051/diff/35079/cc/frame_rate_counter.cc#...
cc/frame_rate_counter.cc:74: bool schedulerAllowsDoubleFrames = m_hasImplThread;
The '!' is missing in the new patch.

[aelias_OOO_until_Jul13, 2012-11-01 23:03:45]

Good catch, thanks, uploaded a version that puts it back.

I can't repro the layout test flakiness on my Linux machine.  Do we believe this
was the cause?

[aelias_OOO_until_Jul13, 2012-11-03 00:11:19]

OK, I have a fix for the webkit_unit_tests hang.  The cause was that an old
pointer to impl_thread_message_loop_proxy_ was getting carried over on the
global compositorSupport object between different unit tests.  I fixed it by
setting it explicitly to NULL in WebCompositorSupportImpl::initialize().

[jamesr, 2012-11-03 00:13:13]

lgtm

https://codereview.chromium.org/11232051/diff/36079/webkit/compositor_binding...
File webkit/compositor_bindings/web_compositor_support_impl.cc (right):

https://codereview.chromium.org/11232051/diff/36079/webkit/compositor_binding...
webkit/compositor_bindings/web_compositor_support_impl.cc:64:
impl_thread_message_loop_proxy_ = NULL;
could you do this in ::shutdown() ?

[aelias_OOO_until_Jul13, 2012-11-03 00:20:45]

OK, moved to shutdown() -- that works as well.  Relanding.

[commit-bot: I haz the power, 2012-11-03 00:21:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/35133

[commit-bot: I haz the power, 2012-11-03 00:44:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/36081

[commit-bot: I haz the power, 2012-11-03 03:37:51]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-03 05:05:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/36081

[commit-bot: I haz the power, 2012-11-03 05:13:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/28248

[commit-bot: I haz the power, 2012-11-03 07:10:57]

Failed to apply patch for cc/resource_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resource_provider.cc
  Hunk #1 succeeded at 14 with fuzz 2.
  Hunk #2 succeeded at 130 (offset -7 lines).
  Hunk #3 succeeded at 159 (offset -7 lines).
  Hunk #4 succeeded at 185 (offset -7 lines).
  Hunk #5 succeeded at 197 (offset -7 lines).
  Hunk #6 succeeded at 208 (offset -7 lines).
  Hunk #7 succeeded at 240 (offset -7 lines).
  Hunk #8 succeeded at 260 (offset -7 lines).
  Hunk #9 succeeded at 323 (offset -7 lines).
  Hunk #10 succeeded at 331 (offset -7 lines).
  Hunk #11 succeeded at 342 (offset -7 lines).
  Hunk #12 succeeded at 370 (offset -7 lines).
  Hunk #13 succeeded at 381 (offset -7 lines).
  Hunk #14 succeeded at 395 (offset -7 lines).
  Hunk #15 succeeded at 478 (offset -7 lines).
  Hunk #16 succeeded at 514 (offset -7 lines).
  Hunk #17 succeeded at 524 (offset -7 lines).
  Hunk #18 succeeded at 534 (offset -7 lines).
  Hunk #19 FAILED at 549.
  Hunk #20 FAILED at 571.
  Hunk #21 succeeded at 589 (offset -9 lines).
  Hunk #22 succeeded at 621 (offset -9 lines).
  Hunk #23 succeeded at 645 (offset -9 lines).
  2 out of 23 hunks FAILED -- saving rejects to file cc/resource_provider.cc.rej

Patch:       cc/resource_provider.cc
Index: cc/resource_provider.cc
diff --git a/cc/resource_provider.cc b/cc/resource_provider.cc
index
eb17f188fef9edb5cc7465dd94ab1e451a96192d..9f22193ee242d060185765b79475ba5b2545dd3a
100644
--- a/cc/resource_provider.cc
+++ b/cc/resource_provider.cc
@@ -14,7 +14,6 @@
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "cc/gl_renderer.h" // For the GLC() macro.
-#include "cc/proxy.h"
 #include "cc/texture_uploader.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
@@ -138,13 +137,13 @@ ResourceProvider::~ResourceProvider()
 
 WebGraphicsContext3D* ResourceProvider::graphicsContext3D()
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     return m_context->context3D();
 }
 
 bool ResourceProvider::inUseByConsumer(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -167,7 +166,7 @@ ResourceProvider::ResourceId
ResourceProvider::createResource(int pool, const gf
 
 ResourceProvider::ResourceId ResourceProvider::createGLTexture(int pool, const
gfx::Size& size, GLenum format, TextureUsageHint hint)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     unsigned textureId = 0;
     WebGraphicsContext3D* context3d = m_context->context3D();
     DCHECK(context3d);
@@ -193,7 +192,7 @@ ResourceProvider::ResourceId
ResourceProvider::createGLTexture(int pool, const g
 
 ResourceProvider::ResourceId ResourceProvider::createBitmap(int pool, const
gfx::Size& size)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
 
     uint8_t* pixels = new uint8_t[size.width() * size.height() * 4];
 
@@ -205,7 +204,7 @@ ResourceProvider::ResourceId
ResourceProvider::createBitmap(int pool, const gfx:
 
 ResourceProvider::ResourceId
ResourceProvider::createResourceFromExternalTexture(unsigned textureId)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     DCHECK(m_context->context3D());
     ResourceId id = m_nextId++;
     Resource resource(textureId, 0, gfx::Size(), 0);
@@ -216,7 +215,7 @@ ResourceProvider::ResourceId
ResourceProvider::createResourceFromExternalTexture
 
 void ResourceProvider::deleteResource(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -248,7 +247,7 @@ void
ResourceProvider::deleteResourceInternal(ResourceMap::iterator it)
 
 void ResourceProvider::deleteOwnedResources(int pool)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceIdArray toDelete;
     for (ResourceMap::iterator it = m_resources.begin(); it !=
m_resources.end(); ++it) {
         if (it->second.pool == pool && !it->second.external &&
!it->second.markedForDeletion)
@@ -268,7 +267,7 @@ ResourceProvider::ResourceType
ResourceProvider::resourceType(ResourceId id)
 
 void ResourceProvider::setPixels(ResourceId id, const uint8_t* image, const
gfx::Rect& imageRect, const gfx::Rect& sourceRect, const gfx::Vector2d&
destOffset)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -331,7 +330,7 @@ double ResourceProvider::estimatedUploadsPerSecond()
 
 void ResourceProvider::flush()
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     WebGraphicsContext3D* context3d = m_context->context3D();
     if (context3d)
         context3d->flush();
@@ -339,7 +338,7 @@ void ResourceProvider::flush()
 
 bool ResourceProvider::shallowFlushIfSupported()
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     WebGraphicsContext3D* context3d = m_context->context3D();
     if (!context3d || !m_useShallowFlush)
         return false;
@@ -350,7 +349,7 @@ bool ResourceProvider::shallowFlushIfSupported()
 
 const ResourceProvider::Resource* ResourceProvider::lockForRead(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
 
     if (it == m_resources.end()) {
@@ -378,7 +377,7 @@ const ResourceProvider::Resource*
ResourceProvider::lockForRead(ResourceId id)
 
 void ResourceProvider::unlockForRead(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -389,7 +388,7 @@ void ResourceProvider::unlockForRead(ResourceId id)
 
 const ResourceProvider::Resource* ResourceProvider::lockForWrite(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -403,7 +402,7 @@ const ResourceProvider::Resource*
ResourceProvider::lockForWrite(ResourceId id)
 
 void ResourceProvider::unlockForWrite(ResourceId id)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ResourceMap::iterator it = m_resources.find(id);
     CHECK(it != m_resources.end());
     Resource* resource = &it->second;
@@ -486,7 +485,7 @@ ResourceProvider::ResourceProvider(GraphicsContext* context)
 
 bool ResourceProvider::initialize()
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     WebGraphicsContext3D* context3d = m_context->context3D();
     if (!context3d) {
         m_maxTextureSize = INT_MAX / 2;
@@ -522,7 +521,7 @@ bool ResourceProvider::initialize()
 
 int ResourceProvider::createChild(int pool)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     Child childInfo;
     childInfo.pool = pool;
     int child = m_nextChild++;
@@ -532,7 +531,7 @@ int ResourceProvider::createChild(int pool)
 
 void ResourceProvider::destroyChild(int child)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ChildMap::iterator it = m_children.find(child);
     DCHECK(it != m_children.end());
     deleteOwnedResources(it->second.pool);
@@ -542,7 +541,7 @@ void ResourceProvider::destroyChild(int child)
 
 const ResourceProvider::ResourceIdMap&
ResourceProvider::getChildToParentMap(int child) const
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     ChildMap::const_iterator it = m_children.find(child);
     DCHECK(it != m_children.end());
     return it->second.childToParentMap;
@@ -550,7 +549,7 @@ const ResourceProvider::ResourceIdMap&
ResourceProvider::getChildToParentMap(int
 
 ResourceProvider::TransferableResourceList
ResourceProvider::prepareSendToParent(const ResourceIdArray& resources)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     TransferableResourceList list;
     list.syncPoint = 0;
     WebGraphicsContext3D* context3d = m_context->context3D();
@@ -572,7 +571,7 @@ ResourceProvider::TransferableResourceList
ResourceProvider::prepareSendToParent
 
 ResourceProvider::TransferableResourceList
ResourceProvider::prepareSendToChild(int child, const ResourceIdArray&
resources)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     TransferableResourceList list;
     list.syncPoint = 0;
     WebGraphicsContext3D* context3d = m_context->context3D();
@@ -599,7 +598,7 @@ ResourceProvider::TransferableResourceList
ResourceProvider::prepareSendToChild(
 
 void ResourceProvider::receiveFromChild(int child, const
TransferableResourceList& resources)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread());
     WebGraphicsContext3D* context3d = m_context->context3D();
     if (!context3d || !context3d->makeContextCurrent()) {
         // FIXME: Implement this path for software compositing.
@@ -631,7 +630,7 @@ void ResourceProvider::receiveFromChild(int child, const
TransferableResourceLis
 
 void ResourceProvider::receiveFromParent(const TransferableResourceList&
resources)
 {
-    DCHECK(Proxy::isImplThread());
+    DCHECK(m_threadChecker.CalledOnValidThread())…
(message too large)

[commit-bot: I haz the power, 2012-11-03 23:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/36085

[commit-bot: I haz the power, 2012-11-04 00:35:51]

Retried try job too often for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests

[commit-bot: I haz the power, 2012-11-04 00:46:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/9235

[commit-bot: I haz the power, 2012-11-04 03:25:57]

Change committed as 165872

[aelias_OOO_until_Jul13, 2012-11-08 02:55:46]

PTAL.  This version has the following changes:

- WebLayerTreeViewImpl::m_hasImplThread was always false because I was taking
the value after the scoped_ptr::Pass.  This was the cause of the ChromeOS
problem.  I just moved the logic in composite() to LayerTreeHost::composite()
instead, bypassing the need to keep track of the value.

- I put the DebugScopedSetImplThread in
LayerTreeHostImpl::setManagedMemoryPolicy back into an if
(m_proxy->hasImplThread()) clause.  This caused flakiness in debug builds with
threaded compositing enabled, if the main thread hit an assertion isMainThread()
at the same time.

- I deleted WebLayerTreeView::create() and its only caller in web_layer_test.

- I added DCHECKs to WebCompositorSupportImpl to make sure
initialize/createLayerTreeView/shutdown are called in the expected order.

[jamesr, 2012-11-08 03:03:16]

lgtm

http://codereview.chromium.org/11232051/diff/32062/webkit/compositor_bindings...
File webkit/compositor_bindings/test/run_all_unittests.cc (right):

http://codereview.chromium.org/11232051/diff/32062/webkit/compositor_bindings...
webkit/compositor_bindings/test/run_all_unittests.cc:8: #include
"cc/thread_impl.h"
can remove these #includes now too

http://codereview.chromium.org/11232051/diff/32062/webkit/compositor_bindings...
webkit/compositor_bindings/test/run_all_unittests.cc:9: #include <gmock/gmock.h>
since you're here, mind changing this to "testing/gmock/include/gmock.h" ?
that's what it's supposed to be but i didn't know any better

http://codereview.chromium.org/11232051/diff/32062/webkit/compositor_bindings...
File webkit/compositor_bindings/web_compositor_support_impl.cc (right):

http://codereview.chromium.org/11232051/diff/32062/webkit/compositor_bindings...
webkit/compositor_bindings/web_compositor_support_impl.cc:105:
scoped_ptr<cc::Thread> impl_thread(NULL);
nit: you don't need the (NULL) here - the default constructor does the right
thing

[aelias_OOO_until_Jul13, 2012-11-08 03:22:53]

OK, all done.  Crossing fingers and relanding.

[commit-bot: I haz the power, 2012-11-08 03:23:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11232051/27188

[commit-bot: I haz the power, 2012-11-08 07:03:47]

Change committed as 166627