cc: Support on demand raster with ganesh

cc: Support on demand raster with gpu rasterization

Normal gpu rasterization happens on compositor thread, so on-demand
rasteration must also be on compositor thread. So if impl-side painting is
off or if gpu rasterization is on, then directly raster on compositor
thread.

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

[boliu, 2014-06-16 17:15:56]

Could you take a first pass please to see if this approach is reasoanble?

[reveman, 2014-06-17 20:13:03]

I'm not sure this is good idea. The use of the task graph runner for on-demand
is really just a workaround for the restriction that usage of SkPictures is not
thread safe without cloning. I don't like that idea that we're adding even more
complexity to cc/ because of this restriction.

If you're trying to use ganesh for on-demand raster then why use a task graph
runner instance at all? It's all running on the same thread so you don't have to
worry about SkPicture thread safety..

[boliu, 2014-06-17 20:25:52]

On 2014/06/17 20:13:03, reveman wrote:
> I'm not sure this is good idea. The use of the task graph runner for on-demand
> is really just a workaround for the restriction that usage of SkPictures is
not
> thread safe without cloning. I don't like that idea that we're adding even
more
> complexity to cc/ because of this restriction.
> 
> If you're trying to use ganesh for on-demand raster then why use a task graph
> runner instance at all? It's all running on the same thread so you don't have
to
> worry about SkPicture thread safety..

Sure that works too. Didn't realize it was a hack.

[boliu, 2014-06-17 22:23:30]

Uploaded PS2 which is much smaller. Haven't wrote any tests yet.

[boliu, 2014-06-18 01:07:40]

Added one small test. PTAL

[reveman, 2014-06-18 01:18:28]

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h#new...
cc/output/renderer.h:43: virtual bool UseTaskGraphRunnerForOnDemandRaster() = 0;
Would it be better to just have this take a resource id and a picture pile and
perform the raster in whatever way is appropriate?

[boliu, 2014-06-18 02:02:36]

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h#new...
cc/output/renderer.h:43: virtual bool UseTaskGraphRunnerForOnDemandRaster() = 0;
On 2014/06/18 01:18:28, reveman wrote:
> Would it be better to just have this take a resource id and a picture pile and
> perform the raster in whatever way is appropriate?

Umm, not familiar with this area so not sure? Two questions.

SoftwareRenderer on-demand raster shouldn't be using any resources and it's part
of webview's requirement. So resource id doesn't make sense to me. I thought
on-demand raster is used (in chrome) during OOM when a tile doesn't get a
resource?

Also I noticed SoftwareRenderer uses RasterDirect but GLRenderer uses
RasterToBitmap. Seems the difference is important, so has to pass a bool/enum
for this?

[reveman, 2014-06-18 04:17:25]

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h#new...
cc/output/renderer.h:43: virtual bool UseTaskGraphRunnerForOnDemandRaster() = 0;
On 2014/06/18 02:02:36, boliu wrote:
> On 2014/06/18 01:18:28, reveman wrote:
> > Would it be better to just have this take a resource id and a picture pile
and
> > perform the raster in whatever way is appropriate?
> 
> Umm, not familiar with this area so not sure? Two questions.
> 
> SoftwareRenderer on-demand raster shouldn't be using any resources and it's
part
> of webview's requirement. So resource id doesn't make sense to me. I thought
> on-demand raster is used (in chrome) during OOM when a tile doesn't get a
> resource?

SoftwareRenderer can use software resources, right? On-demand allow one resource
to be used to draw multiple tiles so the memory usage is constant.

> 
> Also I noticed SoftwareRenderer uses RasterDirect but GLRenderer uses
> RasterToBitmap. Seems the difference is important, so has to pass a bool/enum
> for this?

I'm not sure why that is. Are you sure it makes a difference? If we can
initialize software resources in the tile manager without knowing if it's a
software resource then we should also be able to do that for on-demand raster.

[boliu, 2014-06-18 16:41:11]

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h#new...
cc/output/renderer.h:43: virtual bool UseTaskGraphRunnerForOnDemandRaster() = 0;
On 2014/06/18 04:17:24, reveman wrote:
> On 2014/06/18 02:02:36, boliu wrote:
> > On 2014/06/18 01:18:28, reveman wrote:
> > > Would it be better to just have this take a resource id and a picture pile
> and
> > > perform the raster in whatever way is appropriate?
> > 
> > Umm, not familiar with this area so not sure? Two questions.
> > 
> > SoftwareRenderer on-demand raster shouldn't be using any resources and it's
> part
> > of webview's requirement. So resource id doesn't make sense to me. I thought
> > on-demand raster is used (in chrome) during OOM when a tile doesn't get a
> > resource?
> 
> SoftwareRenderer can use software resources, right? On-demand allow one
resource
> to be used to draw multiple tiles so the memory usage is constant.

Not webview's SoftwareRenderer I think? That one is specifically called
"resourceless software draw"; it just blimps into the SkCanvas that is backed
directly by the output surface software device. (In webview, the software device
is given to us by android view system and could already contains views below the
webview.) We avoid using auxillary bitmaps at all costs here; this draw doesn't
even use any auxillary bitmaps for blending (which is incorrect, but fast)

I think Alex has an internal talk on webview later today that will make this
clearer? Although I just learned about it just now and won't be attending..

> 
> > 
> > Also I noticed SoftwareRenderer uses RasterDirect but GLRenderer uses
> > RasterToBitmap. Seems the difference is important, so has to pass a
bool/enum
> > for this?
> 
> I'm not sure why that is. Are you sure it makes a difference? If we can
> initialize software resources in the tile manager without knowing if it's a
> software resource then we should also be able to do that for on-demand raster.

So for the same reason above, SoftwareRenderer has to use RasterDirect.

GLRenderer could use RasterDirect too if it copies the code that clears the
SkCanvas I guess? But RasterToBitmap is also called tile_manager.cc (not
entirely sure what for) so can't get rid of it after.

[boliu, 2014-06-19 01:08:27]

friendly ping?

[reveman, 2014-06-19 04:23:28]

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/334133002/diff/40001/cc/output/renderer.h#new...
cc/output/renderer.h:43: virtual bool UseTaskGraphRunnerForOnDemandRaster() = 0;
On 2014/06/18 16:41:11, boliu wrote:
> On 2014/06/18 04:17:24, reveman wrote:
> > On 2014/06/18 02:02:36, boliu wrote:
> > > On 2014/06/18 01:18:28, reveman wrote:
> > > > Would it be better to just have this take a resource id and a picture
pile
> > and
> > > > perform the raster in whatever way is appropriate?
> > > 
> > > Umm, not familiar with this area so not sure? Two questions.
> > > 
> > > SoftwareRenderer on-demand raster shouldn't be using any resources and
it's
> > part
> > > of webview's requirement. So resource id doesn't make sense to me. I
thought
> > > on-demand raster is used (in chrome) during OOM when a tile doesn't get a
> > > resource?
> > 
> > SoftwareRenderer can use software resources, right? On-demand allow one
> resource
> > to be used to draw multiple tiles so the memory usage is constant.
> 
> Not webview's SoftwareRenderer I think? That one is specifically called
> "resourceless software draw"; it just blimps into the SkCanvas that is backed
> directly by the output surface software device. (In webview, the software
device
> is given to us by android view system and could already contains views below
the
> webview.) We avoid using auxillary bitmaps at all costs here; this draw
doesn't
> even use any auxillary bitmaps for blending (which is incorrect, but fast)
> 
> I think Alex has an internal talk on webview later today that will make this
> clearer? Although I just learned about it just now and won't be attending..
> 
> > 
> > > 
> > > Also I noticed SoftwareRenderer uses RasterDirect but GLRenderer uses
> > > RasterToBitmap. Seems the difference is important, so has to pass a
> bool/enum
> > > for this?
> > 
> > I'm not sure why that is. Are you sure it makes a difference? If we can
> > initialize software resources in the tile manager without knowing if it's a
> > software resource then we should also be able to do that for on-demand
raster.
> 
> So for the same reason above, SoftwareRenderer has to use RasterDirect.
> 
> GLRenderer could use RasterDirect too if it copies the code that clears the
> SkCanvas I guess? But RasterToBitmap is also called tile_manager.cc (not
> entirely sure what for) so can't get rid of it after.

Ok, in that case "RunOnDemandRasterTask(Task* on_demand_raster_task) is probably
better but I don't think we should use the same TaskGraphRunner instance as the
RasterWorkerPool instance for the gpu raster case.

[boliu, 2014-06-19 18:24:15]

Reverted back to RunOnDemandRasterTask, getting a bit hairy and doesn't look
like a clear win over PS3.

Chose to just create new TaskGraphRunner for each on-demand task with gpu
rasterization, which means also handling the namespace token separately. And now
PixelTest needs its own implementation of RunOnDemandRasterTask as well.

Also found that I can no longer repeat LayerTreeHostOnDemandRasterPixelTest in
the same process, since those tests creates and tears down a new compositor
thread each time, but then the OnDemandRasterTaskImpl asks for the thread id
from the same global RasterWorkerPool. In the second test, this fails because
the RasterWorkerPool is neither created nor run on that new compositor thread. I
guess this is a separate issue that's exposed only exposed by this though...

[reveman, 2014-06-20 00:38:41]

https://codereview.chromium.org/334133002/diff/60001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/334133002/diff/60001/cc/output/direct_rendere...
cc/output/direct_renderer.h:94: void RunOnDemandRasterTask(Task*
on_demand_raster_task);
Do you still need this function or can derived classes call
RendererClient::RunOnDemandRasterTask directly instead?

https://codereview.chromium.org/334133002/diff/60001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:151: TaskGraphRunner temp_task_graph_runner;
nit: remove temp_ prefix

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2770: if (use_gpu_rasterization_) {
Can you remove these use_gpu_rasterization_ checks from here and handle that in
CreateAndSetTileManager instead where you're already checking that variable?
What to do here depends more on what the code in that function does than
use_gpu_rasterization_. That way you can also avoid creating a new task graph
runner for each on-demand task..

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:116: class
LayerTreeHostOnDemandRasterPixelTestWithoutTaskGraphRunner
s/WithoutTaskGraphRunner/WithGpuRasterizationForced/ as "WithoutTaskGraphRunner"
is making assumptions about the implementation that I don't think should be done
here.

[boliu, 2014-06-20 00:43:40]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:116: class
LayerTreeHostOnDemandRasterPixelTestWithoutTaskGraphRunner
On 2014/06/20 00:38:41, reveman wrote:
> s/WithoutTaskGraphRunner/WithGpuRasterizationForced/ as
"WithoutTaskGraphRunner"
> is making assumptions about the implementation that I don't think should be
done
> here.

Any thoughts on the crash in the global TaskGraphRunner when running these two
tests consecutively. Copying from #13.

"""
Also found that I can no longer repeat LayerTreeHostOnDemandRasterPixelTest in
the same process, since those tests creates and tears down a new compositor
thread each time, but then the OnDemandRasterTaskImpl asks for the thread id
from the same global RasterWorkerPool. In the second test, this fails because
the RasterWorkerPool is neither created nor run on that new compositor thread. I
guess this is a separate issue that's exposed only exposed by this though...
"""

I think have the test clean up the global TaskGraphRunner at the end would work,
but wanted to get your thoughts.

[reveman, 2014-06-20 01:01:39]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:116: class
LayerTreeHostOnDemandRasterPixelTestWithoutTaskGraphRunner
On 2014/06/20 00:43:39, boliu wrote:
> On 2014/06/20 00:38:41, reveman wrote:
> > s/WithoutTaskGraphRunner/WithGpuRasterizationForced/ as
> "WithoutTaskGraphRunner"
> > is making assumptions about the implementation that I don't think should be
> done
> > here.
> 
> Any thoughts on the crash in the global TaskGraphRunner when running these two
> tests consecutively. Copying from #13.
> 
> """
> Also found that I can no longer repeat LayerTreeHostOnDemandRasterPixelTest in
> the same process, since those tests creates and tears down a new compositor
> thread each time, but then the OnDemandRasterTaskImpl asks for the thread id
> from the same global RasterWorkerPool. In the second test, this fails because
> the RasterWorkerPool is neither created nor run on that new compositor thread.
I
> guess this is a separate issue that's exposed only exposed by this though...
> """

Do you mean that the TaskGraphRunner instance is created on a different thread
than the cc thread used in the second test and this causes the TLS not to be set
correctly?

Maybe use index 0 when TLS is not set instead of setting it to 0 for origin
thread. Not sure we can rely on it to be NULL initially though. This picture
cloning stuff is annoying, I hope we can remove it soon...

> 
> I think have the test clean up the global TaskGraphRunner at the end would
work,
> but wanted to get your thoughts.

[boliu, 2014-06-20 01:27:06]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2770: if (use_gpu_rasterization_) {
On 2014/06/20 00:38:41, reveman wrote:
> Can you remove these use_gpu_rasterization_ checks from here and handle that
in
> CreateAndSetTileManager instead where you're already checking that variable?
> What to do here depends more on what the code in that function does than
> use_gpu_rasterization_. That way you can also avoid creating a new task graph
> runner for each on-demand task..

Another question. If we do this, do we have to worry about on-demand raster when
impl-side painting is off? I think the SoftwareRenderer path is not specific to
impl-side painting?

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:116: class
LayerTreeHostOnDemandRasterPixelTestWithoutTaskGraphRunner
On 2014/06/20 01:01:38, reveman wrote:
> On 2014/06/20 00:43:39, boliu wrote:
> > On 2014/06/20 00:38:41, reveman wrote:
> > > s/WithoutTaskGraphRunner/WithGpuRasterizationForced/ as
> > "WithoutTaskGraphRunner"
> > > is making assumptions about the implementation that I don't think should
be
> > done
> > > here.
> > 
> > Any thoughts on the crash in the global TaskGraphRunner when running these
two
> > tests consecutively. Copying from #13.
> > 
> > """
> > Also found that I can no longer repeat LayerTreeHostOnDemandRasterPixelTest
in
> > the same process, since those tests creates and tears down a new compositor
> > thread each time, but then the OnDemandRasterTaskImpl asks for the thread id
> > from the same global RasterWorkerPool. In the second test, this fails
because
> > the RasterWorkerPool is neither created nor run on that new compositor
thread.
> I
> > guess this is a separate issue that's exposed only exposed by this though...
> > """
> 
> Do you mean that the TaskGraphRunner instance is created on a different thread
> than the cc thread used in the second test and this causes the TLS not to be
set
> correctly?

Yep. The cc thread in the two threads are not the same one and it's the same
TaskGraphRunner instance that's created on in first cc thread, then used again
on the second cc thread.

> 
> Maybe use index 0 when TLS is not set instead of setting it to 0 for origin
> thread. Not sure we can rely on it to be NULL initially though. This picture
> cloning stuff is annoying, I hope we can remove it soon...

Using 0 is fine at least for the test. It's not an issue in pratice because
compositor thread is always the same one in chrome.

> 
> > 
> > I think have the test clean up the global TaskGraphRunner at the end would
> work,
> > but wanted to get your thoughts.
>

[boliu, 2014-06-20 01:28:16]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2792: if (use_gpu_rasterization_) {
Also this branch can't move to CreateAndSetTileManager.

[reveman, 2014-06-20 01:29:21]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2770: if (use_gpu_rasterization_) {
On 2014/06/20 01:27:06, boliu wrote:
> On 2014/06/20 00:38:41, reveman wrote:
> > Can you remove these use_gpu_rasterization_ checks from here and handle that
> in
> > CreateAndSetTileManager instead where you're already checking that variable?
> > What to do here depends more on what the code in that function does than
> > use_gpu_rasterization_. That way you can also avoid creating a new task
graph
> > runner for each on-demand task..
> 
> Another question. If we do this, do we have to worry about on-demand raster
when
> impl-side painting is off? I think the SoftwareRenderer path is not specific
to
> impl-side painting?

I don't think we can generate picture-draw-quads without impl-side painting
turned on.

[reveman, 2014-06-20 01:32:38]

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2792: if (use_gpu_rasterization_) {
On 2014/06/20 01:28:16, boliu wrote:
> Also this branch can't move to CreateAndSetTileManager.

Could it instead be something like the following?

"if (on_demand_task_graph_runner_ == synchronous_task_graph_runner_.get())"

which would depend on what you do in CreateAndSetTileManager.

[boliu, 2014-06-20 01:43:15]

New patch up. All comments addressed I think. Thanks for answering all my
questions :)

[reveman, 2014-06-20 03:56:30]

https://codereview.chromium.org/334133002/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/334133002/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:50: // Use index 0 for origin thread.
Please update and move this comment to GetPictureCloneIndexForCurrentThread.

https://codereview.chromium.org/334133002/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:51: current_tls_.Set(new
ThreadLocalState(0));
Please remove this now that you handle this below instead.

https://codereview.chromium.org/334133002/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:58: return 0;
How about using a temporary ThreadLocalState* variable here and "return tls ?
tls->picture_clone_index : 0;"

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1876: on_demand_task_graph_runner_ = NULL;
nit: doesn't look like you need this line

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1890:
synchronous_on_demand_task_graph_runner_.reset(new TaskGraphRunner);
A TaskGraphRunner instance is relatively lightweight. I think you can create
this in the ctor instead of doing it lazily here if you like. One less "if"
statement :)

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2792: } else {
can you remove this "else" and always call WaitForTasksToFinishRunning?

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:577: scoped_ptr<TaskGraphRunner>
synchronous_on_demand_task_graph_runner_;
you could just call it |synchronous_task_graph_runner_| if you like as there's
nothing that prevents it from being used for some other synchronous tasks in the
future.

[boliu, 2014-06-20 04:21:06]

All done

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1890:
synchronous_on_demand_task_graph_runner_.reset(new TaskGraphRunner);
On 2014/06/20 03:56:30, reveman wrote:
> A TaskGraphRunner instance is relatively lightweight. I think you can create
> this in the ctor instead of doing it lazily here if you like. One less "if"
> statement :)

Then don't need scoped_ptr either. Just embed the actual object.

[boliu, 2014-06-20 18:38:47]

Ping?

[reveman, 2014-06-20 18:44:20]

lgtm after fixing a few nits

https://codereview.chromium.org/334133002/diff/100001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/334133002/diff/100001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:54: // Use index 0 for origin thread and any
unknown threads.
nit: "Use index 0 if called on non-raster thread."

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1887: 
nit: this blank line looks inconsistent with lines below where assignment is
grouped together with raster_worker_pool_ assignment. grouping them makes sense
imo.

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1940: on_demand_task_namespace_ =
on_demand_task_graph_runner_->GetNamespaceToken();
Note: we'll end up creating a new task namespace each time
::CreateAndSetTileManager is called but I think that's fine.

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2785: if (&synchronous_task_graph_runner_ ==
on_demand_task_graph_runner_)
nit: do you mind flipping this around? not sure why but I prefer "if
(on_demand_task_graph_runner_ == &synchronous_task_graph_runner_)" for some
 reason :)

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2786:
on_demand_task_graph_runner_->RunUntilIdle();
nit: I think a blank line here would make the code a bit easier to read

[boliu, 2014-06-20 19:02:19]

All done

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1887: 
On 2014/06/20 18:44:19, reveman wrote:
> nit: this blank line looks inconsistent with lines below where assignment is
> grouped together with raster_worker_pool_ assignment. grouping them makes
sense
> imo.

Removed this blank line then.

https://codereview.chromium.org/334133002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1940: on_demand_task_namespace_ =
on_demand_task_graph_runner_->GetNamespaceToken();
On 2014/06/20 18:44:19, reveman wrote:
> Note: we'll end up creating a new task namespace each time
> ::CreateAndSetTileManager is called but I think that's fine.

Yeah I thought about this. In webview CreateAndSetTileManager can actually
happen more frequently than chrome. But they are mostly only from user action,
so 2 billion (or something on that order) should be more than enough :)

[boliu, 2014-06-20 19:06:51]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:10:21]

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

[commit-bot: I haz the power, 2014-06-21 01:12:01]

Change committed as 278887