cc: Move worker threads to content.

cc: Move worker threads to RenderThreadImpl.

This allows us to Join the threads when RenderThreadImpl
is shutdown.

BUG=468785
TEST=cc_unittests

Committed: https://crrev.com/34b7a15218828edc105d4c408556b05c52994840
Cr-Commit-Position: refs/heads/master@{#321836}

[reveman, 2015-03-20 06:00:39]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-03-20 06:28:35]

https://codereview.chromium.org/999173004/diff/100001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/999173004/diff/100001/ui/compositor/composito...
ui/compositor/compositor.cc:96: raster_thread_(new
RasterThread(task_graph_runner_.get())),
We wouldn't want one raster thread per compositor.... We have one compositor per
window.

I think this should go onto the ui::ContextFactory (GpuProcessTransportFactory
in content/ for prod, ui::InProcessContextFactory for tests).

Also, we probably don't want to create the thread if
!IsUIImplSidePaintingEnabled()

[reveman, 2015-03-20 07:42:03]

PTAL

https://codereview.chromium.org/999173004/diff/100001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/999173004/diff/100001/ui/compositor/composito...
ui/compositor/compositor.cc:96: raster_thread_(new
RasterThread(task_graph_runner_.get())),
On 2015/03/20 at 06:28:35, piman (Very slow to review) wrote:
> We wouldn't want one raster thread per compositor.... We have one compositor
per window.
> 
> I think this should go onto the ui::ContextFactory (GpuProcessTransportFactory
in content/ for prod, ui::InProcessContextFactory for tests).
> 
> Also, we probably don't want to create the thread if
!IsUIImplSidePaintingEnabled()

Makes sense. Done.

[danakj, 2015-03-20 17:10:24]

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

[danakj, 2015-03-20 17:10:26]

https://codereview.chromium.org/999173004/diff/180001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/999173004/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:98:
recording_source_->SetGatherPixelRefs(host->settings().gather_pixel_refs);
is this related to this CL?

https://codereview.chromium.org/999173004/diff/180001/cc/resources/display_li...
File cc/resources/display_list_recording_source.cc (right):

https://codereview.chromium.org/999173004/diff/180001/cc/resources/display_li...
cc/resources/display_list_recording_source.cc:139: void
DisplayListRecordingSource::SetGatherPixelRefs(bool gather_pixel_refs) {
how is this change related to this CL?

[danakj, 2015-03-20 17:15:36]

https://codereview.chromium.org/999173004/diff/180001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (left):

https://codereview.chromium.org/999173004/diff/180001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:546: bool gather_pixel_refs =
TileTaskWorkerPool::GetNumWorkerThreads() > 1;
Ah.. ok I found it. There are so many test changes it's hard to see what this
patch is actually trying to do :( I wish we didn't make our tests like that.

https://codereview.chromium.org/999173004/diff/180001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/999173004/diff/180001/ui/compositor/composito...
ui/compositor/compositor.cc:142: context_factory_->GetTaskGraphRunner(),
settings, task_runner_, nullptr);
Do we never want to GatherPixelRefs here? can you leave a TODO or a comment
about this?

[reveman, 2015-03-20 17:18:52]

https://codereview.chromium.org/999173004/diff/180001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/999173004/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:98:
recording_source_->SetGatherPixelRefs(host->settings().gather_pixel_refs);
On 2015/03/20 at 17:10:26, danakj wrote:
> is this related to this CL?

I have to remove TileTaskWorkerPool::GetNumWorkerThreads() as part of this
change so we can't use that anymore to determine if we should gather pixel refs.
CC is no longer aware of the number of worker threads used.

[reveman, 2015-03-20 17:56:30]

https://codereview.chromium.org/999173004/diff/180001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (left):

https://codereview.chromium.org/999173004/diff/180001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:546: bool gather_pixel_refs =
TileTaskWorkerPool::GetNumWorkerThreads() > 1;
On 2015/03/20 17:15:36, danakj wrote:
> Ah.. ok I found it. There are so many test changes it's hard to see what this
> patch is actually trying to do :( I wish we didn't make our tests like that.

Me too. Making a change like this is really painful as a result of our tests.

https://codereview.chromium.org/999173004/diff/180001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/999173004/diff/180001/ui/compositor/composito...
ui/compositor/compositor.cc:142: context_factory_->GetTaskGraphRunner(),
settings, task_runner_, nullptr);
On 2015/03/20 at 17:15:36, danakj wrote:
> Do we never want to GatherPixelRefs here? can you leave a TODO or a comment
about this?

Done.

[danakj, 2015-03-20 18:07:07]

LGTM % piman

[reveman, 2015-03-20 18:18:15]

reveman@chromium.org changed reviewers:
+ boliu@chromium.org, jamesr@chromium.org

[reveman, 2015-03-20 18:18:17]

+boliu for android_webview/
+jamesr for mojo/

[boliu, 2015-03-20 18:20:26]

On 2015/03/20 18:18:17, reveman wrote:
> +boliu for android_webview/

rs lgtm

> +jamesr for mojo/

[piman, 2015-03-20 19:32:18]

lgtm

[reveman, 2015-03-20 21:31:41]

reveman@chromium.org changed reviewers:
+ ben@chromium.org

[reveman, 2015-03-20 21:31:43]

+ben for mojo/

[jamesr, 2015-03-23 17:55:36]

lgtm w/ a question

https://codereview.chromium.org/999173004/diff/200001/mojo/services/html_view...
File mojo/services/html_viewer/weblayertreeview_impl.cc (right):

https://codereview.chromium.org/999173004/diff/200001/mojo/services/html_view...
mojo/services/html_viewer/weblayertreeview_impl.cc:39: cc::TaskGraphRunner*
task_graph_runner = NULL;
these 3 should all be nullptr

does this being null mean that mojo html_viewer can't use any worker threads for
raster? what would need to be implemented for it to do so?

[reveman, 2015-03-23 18:39:46]

https://codereview.chromium.org/999173004/diff/200001/mojo/services/html_view...
File mojo/services/html_viewer/weblayertreeview_impl.cc (right):

https://codereview.chromium.org/999173004/diff/200001/mojo/services/html_view...
mojo/services/html_viewer/weblayertreeview_impl.cc:39: cc::TaskGraphRunner*
task_graph_runner = NULL;
On 2015/03/23 17:55:36, jamesr wrote:
> these 3 should all be nullptr

https://codereview.chromium.org/1023813004

> 
> does this being null mean that mojo html_viewer can't use any worker threads
for
> raster? what would need to be implemented for it to do so?

Yes, we'd have to create a TaskGraphRunner instance and at least 1 worker thread
to process tasks. A simple example can be found in the
content/browser/compositor/gpu_process_transport_factory.cc change of this
patch.

We would also need a SharedBitmapManager and/or a GpuMemoryBufferManager
instance to perform non-GPU raster. SharedBitmaps for sw compositing,
GpuMemoryBuffer for hw compositing.

[reveman, 2015-03-23 18:41:46]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-03-23 18:42:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/999173004/200001

[commit-bot: I haz the power, 2015-03-23 20:28:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-23 20:29:19]

Patchset 11 (id:??) landed as
https://crrev.com/34b7a15218828edc105d4c408556b05c52994840
Cr-Commit-Position: refs/heads/master@{#321836}