cc: Add BitmapRasterWorkerPool.

cc: Add BitmapRasterWorkerPool.

This adds a rasterizer implementation explicitly used for
rasterizing to bitmap resources. Each rasterizer is already
aware of specific resource details and it is much easier
and less confusing to implement a different rasterizer that
handle bitmap resources then trying to emulate support for
GL-ish APIs that are used by other rasterizer implementations.

This allows us to clean up LTHI a bit and reduces the
complexity of the ResourceProvider class significantly.

It also makes future changes that will allow GpuMemoryBuffers
to be created and mapped in worker threads easier implement.

BUG=

Committed: https://crrev.com/5ae838bba3f6cc7a14f95cd1ee2fb37bacadde53
Cr-Commit-Position: refs/heads/master@{#296558}

[reveman, 2014-09-24 02:03:42]

reveman@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[vmpstr, 2014-09-24 16:31:51]

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_perftest.cc:233: output_surface_ =
FakeOutputSurface::Create3d(context_provider_).Pass();
Can you refactor this a bit like 

CreateOutputSurfaceAndSurfaceProvider(bool software) {
  output_surface_ = softare ? ... : ...;
  CHECK(... BindToClient);
  resource_provider_ = ...
}

That way it's easy to spot the differences between each enum.

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_unittest.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_unittest.cc:132: output_surface_ =
FakeOutputSurface::Create3d(context_provider_).Pass();
here as well

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:96: if (!context_provider)
Can you instead DCHECK(context_provider)

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:121: if (!context_provider)
Same here.

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1989: } else if (UseZeroCopyRasterizer() &&
context_provider) {
I think this whole if/else if sequence is better written as:

if (!context_provider) {
  ...
} else if (use_gpu_rasterization) {
  DCHECK(context_provider);
  ...
} else if (UseOneCopyRasterizer()) {
  ...
} ... {
} else {
  // context_provider && !(use_gpu_rasteriztion || UseZeroCopy || UseOneCopy)
}

It's just kind of awkward to check that we have context provider in every
if/else if, where we could just handle it not being there in the first if.

[reveman, 2014-09-24 17:25:27]

PTAL

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_perftest.cc:233: output_surface_ =
FakeOutputSurface::Create3d(context_provider_).Pass();
On 2014/09/24 16:31:51, vmpstr wrote:
> Can you refactor this a bit like 
> 
> CreateOutputSurfaceAndSurfaceProvider(bool software) {
>   output_surface_ = softare ? ... : ...;
>   CHECK(... BindToClient);
>   resource_provider_ = ...
> }
> 
> That way it's easy to spot the differences between each enum.

Done.

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool_unittest.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool_unittest.cc:132: output_surface_ =
FakeOutputSurface::Create3d(context_provider_).Pass();
On 2014/09/24 16:31:51, vmpstr wrote:
> here as well

Done.

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:96: if (!context_provider)
On 2014/09/24 16:31:51, vmpstr wrote:
> Can you instead DCHECK(context_provider)

I think it was always awkward that we could pass NULL to this function. In
general, I think it's a bit silly to check if the first parameter passed to a
function is non-NULL. That DCHECK is better left to the caller who knows better
if it's needed.

Anyhow, I changed this to take a const ref instead as I think that's more
appropriate.

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:121: if (!context_provider)
On 2014/09/24 16:31:51, vmpstr wrote:
> Same here.

Same reply as above.

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/602493003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1989: } else if (UseZeroCopyRasterizer() &&
context_provider) {
On 2014/09/24 16:31:51, vmpstr wrote:
> I think this whole if/else if sequence is better written as:
> 
> if (!context_provider) {
>   ...
> } else if (use_gpu_rasterization) {
>   DCHECK(context_provider);
>   ...
> } else if (UseOneCopyRasterizer()) {
>   ...
> } ... {
> } else {
>   // context_provider && !(use_gpu_rasteriztion || UseZeroCopy || UseOneCopy)
> }
> 
> It's just kind of awkward to check that we have context provider in every
> if/else if, where we could just handle it not being there in the first if.

Yes, much better. Done.

[vmpstr, 2014-09-24 17:43:07]

lgtm

[reveman, 2014-09-24 19:41:17]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:42:42]

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

[commit-bot: I haz the power, 2014-09-24 20:52:38]

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

[commit-bot: I haz the power, 2014-09-24 20:52:39]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2014-09-24 22:16:11]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-24 22:17:19]

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

[commit-bot: I haz the power, 2014-09-24 23:30:21]

Committed patchset #4 (id:60001) as 9eb0e62632bb491d986ab440f3c2d21348a7c705

[commit-bot: I haz the power, 2014-09-24 23:46:53]

Patchset 4 (id:??) landed as
https://crrev.com/5ae838bba3f6cc7a14f95cd1ee2fb37bacadde53
Cr-Commit-Position: refs/heads/master@{#296558}