cc: Use correct message loop proxy in BlockingTaskRunner

cc: Use correct message loop proxy in BlockingTaskRunner

The compositor makes it possible to customize the message loop proxy
both on the main and impl threads. However BlockingTaskRunner just uses
base::MessageLoopProxy::current() directly and thus overrides this
choice. Using the correct message loop proxy is important because
clients of BlockingTaskRunner have an implicit dependency on the
ordering of tasks posted to BlockingTaskRunner vs. all other tasks
posted to the task runner for that same thread.

This patch fixes the problem by removing the thread-local
BlockingTaskRunner::current() instance in favor of an explicitly
constructed instance which is given a specific SingleThreadTaskRunner
to use. The resource provider is changed to pass a Proxy-owned
blocking task runner to the clients which need one.

BUG=391005
TEST=1. Apply https://codereview.chromium.org/363383002
     2. out/Debug/chrome --disable-impl-side-painting --ignore-gpu-blacklist tools/perf/page_sets/tough_scheduling_cases/raf_canvas.html

Committed: https://crrev.com/3976a3f8509145051ff2865ccba0fffe79fe32e6
Cr-Commit-Position: refs/heads/master@{#293353}

[Sami, 2014-08-26 15:15:59]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org

[Sami, 2014-08-26 15:20:35]

PTAL Dana. I considered changing all the clients of BlockingTaskRunner to pass
in an explicit runner instead, but they're very spread out (via TextureLayer)
and now I kinda like this way better because it ties in nicely to the Proxy's
lifetime.

[danakj, 2014-08-26 15:59:00]

https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
cc/trees/blocking_task_runner.h:75: // Forward the tasks to a specific task
runner instead of the
WDYT of passing this to current() and DCHECKing that it matches the existing
BlockingTaskRunner if one already exists?

Would the changes to Proxy no longer be needed?

[Sami, 2014-08-26 16:19:17]

https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
cc/trees/blocking_task_runner.h:75: // Forward the tasks to a specific task
runner instead of the
On 2014/08/26 15:59:00, danakj wrote:
> WDYT of passing this to current() and DCHECKing that it matches the existing
> BlockingTaskRunner if one already exists?
> 
> Would the changes to Proxy no longer be needed?

The places[1] that call BlockingTaskRunner::current() don't have an easy way to
access the main_task_runner_ from Proxy :( Or did you mean something like the
runner only needs to be passed in the first time this is called and after that
it sticks?

[1] DelegatedFrameResourceCollection, DelegatedRendererLayer and TextureLayer

[danakj, 2014-08-26 16:22:40]

On 2014/08/26 16:19:17, Sami wrote:
>
https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
> File cc/trees/blocking_task_runner.h (right):
> 
>
https://codereview.chromium.org/485043003/diff/40001/cc/trees/blocking_task_r...
> cc/trees/blocking_task_runner.h:75: // Forward the tasks to a specific task
> runner instead of the
> On 2014/08/26 15:59:00, danakj wrote:
> > WDYT of passing this to current() and DCHECKing that it matches the existing
> > BlockingTaskRunner if one already exists?
> > 
> > Would the changes to Proxy no longer be needed?
> 
> The places[1] that call BlockingTaskRunner::current() don't have an easy way
to
> access the main_task_runner_ from Proxy :( Or did you mean something like the
> runner only needs to be passed in the first time this is called and after that
> it sticks?
> 
> [1] DelegatedFrameResourceCollection, DelegatedRendererLayer and TextureLayer

Hm.. In a given process there may be more than 1 compositor ie more than 1
proxy. They may have different message loops.

So it seems like we need to stop using BlockingTaskRunner::current() the same
way we stopped using MessageLoopProxy::current()

[danakj, 2014-08-26 16:31:54]

danakj@chromium.org changed reviewers:
+ jamesr@chromium.org, piman@chromium.org

[danakj, 2014-08-26 16:31:54]

As seen on IRC:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/c...

That code uses DelegatedResourceCollection but has only a WebLayer API to access
compositing. It has no access to a LTH (or Proxy) to get the compositor's main
message loop from there.

But it will need to, to use the right message loop.. so.. somehow that has to go
through blink. +jamesr for thoughts.

[danakj, 2014-08-26 16:59:50]

After more discussion on IRC skyostil and I came up with the following:

1. Change ResourceProvider to hold a BlockingTaskRunner obtained from the Proxy.
2. When ResourceProvider deletes a resource and calls the release callback, it
passes along the BlockingTaskRunner as "a way to get back to the main thread".
3. So, add a new type ReleaseCallbackImpl that is like ReleaseCallback but also
has BlockingTaskRunner*.
4. TextureMailboxHolder and DelegatedResourceCollection no longer grab a
TaskRunner on the main thread and store it as a member, instead the callback
they give to the ResourceProvider takes the TaskRunner* argument.

[Sami, 2014-08-28 15:01:44]

Thanks for helping figure out the right way to do this Dana. Now did I turn that
plan into code correctly?-) Sorry for
the huge patch but this ended up touching many of the tests.

[danakj, 2014-08-28 17:10:47]

*does the no more current() dance*

https://codereview.chromium.org/485043003/diff/160001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (left):

https://codereview.chromium.org/485043003/diff/160001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:323: DCHECK(message_loop_->BelongsToCurrentThread());
can we use a ThreadChecker to verify this is on the main thread?

https://codereview.chromium.org/485043003/diff/160001/cc/resources/release_ca...
File cc/resources/release_callback_impl.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/resources/release_ca...
cc/resources/release_callback_impl.h:15: scoped_refptr<BlockingTaskRunner>
can you make this a raw pointer instead of a refptr?

https://codereview.chromium.org/485043003/diff/160001/cc/resources/single_rel...
File cc/resources/single_release_callback_impl.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/resources/single_rel...
cc/resources/single_release_callback_impl.h:25:
scoped_refptr<BlockingTaskRunner> main_thread_task_runner);
raw ptr here too

https://codereview.chromium.org/485043003/diff/160001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:99: aggregator_->Aggregate(current_surface_id_);
can you add a TODO here that "We should hold a
BlockingTaskRunner::CapturePostTasks while Aggregate is called to immediately
run release callbacks afterward."

+jamesr FYI

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.cc:31: return task_runner_ ?
task_runner_->BelongsToCurrentThread() : true;
The LayerTreeHostNoMessageLoopTest test passes a NULL task runner which is why
this was using thread_id_ instead of the task runner to do this check. Does this
not crash for that test? If not, can you explain why it wouldn't crash
elsewhere?

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:28: // BlockingTaskRunner::Create() with the
SingleThreadTaskRunner for the thread
i think this sentence is awkward. period after Create() and describe what STTR
you want to pass as its own sentence.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:50: scoped_refptr<base::SingleThreadTaskRunner>
task_runner);
I'm starting to wonder if this class needs to be ref counted at all anymore. Is
there any place that needs to hold a ref now? Can this return a scoped_ptr
instead?

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:58: scoped_refptr<BlockingTaskRunner>
blocking_runner);
make this a raw pointer

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:62: scoped_refptr<BlockingTaskRunner>
blocking_runner_;
and this

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.cc
File cc/trees/proxy.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.cc#newcode9
cc/trees/proxy.cc:9: #include "cc/trees/blocking_task_runner.h"
this is already in the .h. can that one be a forward declaration? if not then
remove this.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.h#newcod...
cc/trees/proxy.h:107: scoped_refptr<BlockingTaskRunner>
blocking_main_thread_task_runner() const {
could this return a raw pointer?

[Sami, 2014-08-28 18:21:17]

https://codereview.chromium.org/485043003/diff/160001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (left):

https://codereview.chromium.org/485043003/diff/160001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:323: DCHECK(message_loop_->BelongsToCurrentThread());
On 2014/08/28 17:10:46, danakj wrote:
> can we use a ThreadChecker to verify this is on the main thread?

Oo, good idea.

https://codereview.chromium.org/485043003/diff/160001/cc/resources/release_ca...
File cc/resources/release_callback_impl.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/resources/release_ca...
cc/resources/release_callback_impl.h:15: scoped_refptr<BlockingTaskRunner>
On 2014/08/28 17:10:46, danakj wrote:
> can you make this a raw pointer instead of a refptr?

Done.

https://codereview.chromium.org/485043003/diff/160001/cc/resources/single_rel...
File cc/resources/single_release_callback_impl.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/resources/single_rel...
cc/resources/single_release_callback_impl.h:25:
scoped_refptr<BlockingTaskRunner> main_thread_task_runner);
On 2014/08/28 17:10:46, danakj wrote:
> raw ptr here too

Done.

https://codereview.chromium.org/485043003/diff/160001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:99: aggregator_->Aggregate(current_surface_id_);
On 2014/08/28 17:10:46, danakj wrote:
> can you add a TODO here that "We should hold a
> BlockingTaskRunner::CapturePostTasks while Aggregate is called to immediately
> run release callbacks afterward."
> 
> +jamesr FYI

Done.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.cc:31: return task_runner_ ?
task_runner_->BelongsToCurrentThread() : true;
On 2014/08/28 17:10:46, danakj wrote:
> The LayerTreeHostNoMessageLoopTest test passes a NULL task runner which is why
> this was using thread_id_ instead of the task runner to do this check. Does
this
> not crash for that test? If not, can you explain why it wouldn't crash
> elsewhere?

It doesn't crash because of the NULL check, which I guess is cheating a bit :)
However if a BlockingTaskRunner has no SingleThreadTaskRunner, you could say it
doesn't belong to any specific thread.  I considered keeping the thread_id_
around, but I didn't see why we'd need to restrict which thread
BlockingTaskRunner gets created on so I took it out.

The NULL check is only needed for LayerTreeHostNoMessageLoopTest all others
either have a valid message loop or never end up running uncaptured tasks.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:28: // BlockingTaskRunner::Create() with the
SingleThreadTaskRunner for the thread
On 2014/08/28 17:10:46, danakj wrote:
> i think this sentence is awkward. period after Create() and describe what STTR
> you want to pass as its own sentence.

Yah, not sure what I was going for there. Rewritten.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:50: scoped_refptr<base::SingleThreadTaskRunner>
task_runner);
On 2014/08/28 17:10:46, danakj wrote:
> I'm starting to wonder if this class needs to be ref counted at all anymore.
Is
> there any place that needs to hold a ref now? Can this return a scoped_ptr
> instead?

Great idea, the ownership is much simpler now so a scoped_ptr should do.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:58: scoped_refptr<BlockingTaskRunner>
blocking_runner);
On 2014/08/28 17:10:46, danakj wrote:
> make this a raw pointer

Done.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:62: scoped_refptr<BlockingTaskRunner>
blocking_runner_;
On 2014/08/28 17:10:46, danakj wrote:
> and this

Done.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.cc
File cc/trees/proxy.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.cc#newcode9
cc/trees/proxy.cc:9: #include "cc/trees/blocking_task_runner.h"
On 2014/08/28 17:10:46, danakj wrote:
> this is already in the .h. can that one be a forward declaration? if not then
> remove this.

Oops, fixed.

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/proxy.h#newcod...
cc/trees/proxy.h:107: scoped_refptr<BlockingTaskRunner>
blocking_main_thread_task_runner() const {
On 2014/08/28 17:10:46, danakj wrote:
> could this return a raw pointer?

Yep, done.

[Sami, 2014-08-28 18:24:41]

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/485043003/diff/160001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.cc:31: return task_runner_ ?
task_runner_->BelongsToCurrentThread() : true;
On 2014/08/28 18:21:16, Sami wrote:
> The NULL check is only needed for LayerTreeHostNoMessageLoopTest all others
> either have a valid message loop or never end up running uncaptured tasks.

Whoops, I meant any tasks -- captured or not.

[Sami, 2014-09-03 10:32:57]

Another look Dana? I'll join in on the "no more current" dance as soon as this
lands :)

[danakj, 2014-09-03 16:09:36]

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:288: is_lost_(false) {
can you CalledOnValidThread() here, so we show InternalRelease happens on the
same thread? The checker binds to the first thread CalledOnValidThread happens
on

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
cc/layers/texture_layer_impl.cc:248:
resource_provider->blocking_main_thread_task_runner());
Can you add a getter for this on layer_tree_impl() that gets it from the Proxy
instead of adding an accessor for this on ResourceProvider? (desire to keep RP
interface minimal and focused on resource providing)

https://codereview.chromium.org/485043003/diff/180001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:344: 0, false,
resource_provider->blocking_main_thread_task_runner());
same here use layer_tree_impl to access the task runner

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider.h:377: BlockingTaskRunner*
blocking_main_thread_task_runner() const {
remove this

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:59: BlockingTaskRunner*
main_thread_task_runner) {
Could you add one test for each Gl and Software, where the task runner is not
null and we check that it's passed to the callback? Maybe add another version of
this ReleaseTM function and the ReleaseSMCallback function so you don't have to
change all the users of them? If that helps.

https://codereview.chromium.org/485043003/diff/180001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/surfaces/display.h#n...
cc/surfaces/display.h:8: #include <set>
?

https://codereview.chromium.org/485043003/diff/180001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:28: // BlockingTaskRunner::Create(). The
SingleThreadTaskRunner passed to that
This comment needs updating to reflect that task runner may be null. "The thread
it is created on"?

[Sami, 2014-09-03 17:59:13]

Woo, all addressed.

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:288: is_lost_(false) {
On 2014/09/03 16:09:36, danakj wrote:
> can you CalledOnValidThread() here, so we show InternalRelease happens on the
> same thread? The checker binds to the first thread CalledOnValidThread happens
> on

Looks like the ThreadCheckerImpl constructor locks it to the current thread, no?

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
cc/layers/texture_layer_impl.cc:248:
resource_provider->blocking_main_thread_task_runner());
On 2014/09/03 16:09:36, danakj wrote:
> Can you add a getter for this on layer_tree_impl() that gets it from the Proxy
> instead of adding an accessor for this on ResourceProvider? (desire to keep RP
> interface minimal and focused on resource providing)

Great idea, done.

https://codereview.chromium.org/485043003/diff/180001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:344: 0, false,
resource_provider->blocking_main_thread_task_runner());
On 2014/09/03 16:09:36, danakj wrote:
> same here use layer_tree_impl to access the task runner

Done.

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider.h:377: BlockingTaskRunner*
blocking_main_thread_task_runner() const {
On 2014/09/03 16:09:36, danakj wrote:
> remove this

Done.

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:59: BlockingTaskRunner*
main_thread_task_runner) {
On 2014/09/03 16:09:36, danakj wrote:
> Could you add one test for each Gl and Software, where the task runner is not
> null and we check that it's passed to the callback? Maybe add another version
of
> this ReleaseTM function and the ReleaseSMCallback function so you don't have
to
> change all the users of them? If that helps.

Yep, would be good to have some coverage here too. I ended up extending
TextureMailbox_{SharedMemory,GLTexture2D} to check the callback runs. I didn't
fork the callback since it wasn't used in that many places.

https://codereview.chromium.org/485043003/diff/180001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/surfaces/display.h#n...
cc/surfaces/display.h:8: #include <set>
On 2014/09/03 16:09:36, danakj wrote:
> ?

Hmm, looks like a rebasing snafu. Earlier my patch removed this but somehow it
morphed into adding it instead. Fixed.

https://codereview.chromium.org/485043003/diff/180001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/180001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:28: // BlockingTaskRunner::Create(). The
SingleThreadTaskRunner passed to that
On 2014/09/03 16:09:36, danakj wrote:
> This comment needs updating to reflect that task runner may be null. "The
thread
> it is created on"?

Wordsmith'd.

[piman, 2014-09-03 18:55:44]

LGTM once Dana is happy. This is a great change.

[danakj, 2014-09-03 19:47:42]

LGTM! w/ couple nits

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/485043003/diff/180001/cc/layers/texture_layer...
cc/layers/texture_layer.cc:288: is_lost_(false) {
On 2014/09/03 17:59:12, Sami wrote:
> On 2014/09/03 16:09:36, danakj wrote:
> > can you CalledOnValidThread() here, so we show InternalRelease happens on
the
> > same thread? The checker binds to the first thread CalledOnValidThread
happens
> > on
> 
> Looks like the ThreadCheckerImpl constructor locks it to the current thread,
no?

Ohh, ya it does!

https://codereview.chromium.org/485043003/diff/220001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/220001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:46: virtual ~BlockingTaskRunner();
is there a reason this is virtual now? I think this is leftover from being
refcounted

also nit: put this just below Create() which acts like the public constructor.

https://codereview.chromium.org/485043003/diff/220001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/485043003/diff/220001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:295: BlockingTaskRunner*
BlockingMainThreadTaskRunner() const;
Move this up to the "Methods called by the layer tree that pass-through or
access LTHI." section near the top of the class.

[Sami, 2014-09-04 09:30:47]

Thanks all!

https://codereview.chromium.org/485043003/diff/220001/cc/trees/blocking_task_...
File cc/trees/blocking_task_runner.h (right):

https://codereview.chromium.org/485043003/diff/220001/cc/trees/blocking_task_...
cc/trees/blocking_task_runner.h:46: virtual ~BlockingTaskRunner();
On 2014/09/03 19:47:41, danakj wrote:
> is there a reason this is virtual now? I think this is leftover from being
> refcounted
> 
> also nit: put this just below Create() which acts like the public constructor.

Right, no need for virtual.

https://codereview.chromium.org/485043003/diff/220001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/485043003/diff/220001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:295: BlockingTaskRunner*
BlockingMainThreadTaskRunner() const;
On 2014/09/03 19:47:42, danakj wrote:
> Move this up to the "Methods called by the layer tree that pass-through or
> access LTHI." section near the top of the class.

Done.

[Sami, 2014-09-04 09:30:57]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-04 09:32:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/485043003/240001

[commit-bot: I haz the power, 2014-09-04 16:27:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 16:27:32]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[Sami, 2014-09-04 16:28:12]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-04 16:29:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/485043003/240001

[commit-bot: I haz the power, 2014-09-04 18:02:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:02:16]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[Sami, 2014-09-04 18:06:41]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:08:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/485043003/240001

[commit-bot: I haz the power, 2014-09-04 19:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 19:36:17]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Sami, 2014-09-04 22:00:50]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:05:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/485043003/240001

[commit-bot: I haz the power, 2014-09-04 22:10:23]

Committed patchset #13 (id:240001) as c2d572fc808398aeee44342007ff77d13e7fa08f

[commit-bot: I haz the power, 2014-09-10 03:33:12]

Patchset 13 (id:??) landed as
https://crrev.com/3976a3f8509145051ff2865ccba0fffe79fe32e6
Cr-Commit-Position: refs/heads/master@{#293353}