cc: Rename DirectRasterWorkerPool to GpuRasterWorkerPool

cc: Rename DirectRasterWorkerPool to GpuRasterWorkerPool

This renames DirectRasterWorkerPool to GpuRasterWorkerPool 
to make it obvious that TileManager uses Gpu rasterization, 
and also limits the usage of this implementation to gpu raster.

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

[sohanjg, 2014-06-27 08:35:28]

Please take a look.
reveman@, if you could comment how we can limit directrasterworker only for gpu
raster ?
Thanks.

[reveman, 2014-06-27 15:47:54]

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:150: return
resource_provider_->MapDirectRasterBuffer(task->resource()->id());
Can we rename RasterBuffer API too in this patch? ie. Map/UnmapGrRasterBuffer to
match GrContext()..

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:191: if (context_provider_) {
I don't think this check should be needed anymore if this class is limited to
Gpu raster.

[sohanjg, 2014-06-30 08:46:19]

Have renamed DirectRasterBuffer to GpuRasterBuffer in resourceprovider, along
with changing the name of related map/unmap api's.
Please take a look. Thanks.

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:150: return
resource_provider_->MapDirectRasterBuffer(task->resource()->id());
On 2014/06/27 15:47:53, reveman wrote:
> Can we rename RasterBuffer API too in this patch? ie. Map/UnmapGrRasterBuffer
to
> match GrContext()..

Done.
Have updated it as Map/UnmapGpuRasterBuffer to be consistent with
GpuRasterWorkerPool naming. or do you think this may create confusion with GPU
memory buffers used in zero/one-copy implementation ?

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:191: if (context_provider_) {
On 2014/06/27 15:47:53, reveman wrote:
> I don't think this check should be needed anymore if this class is limited to
> Gpu raster.

The class creation is already limited to gpu raster using use_gpu_rasterization_
flag in LTHI.
how do we further limit it ? in GpuRasterWorkerPool::Create ?

[reveman, 2014-06-30 13:20:15]

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:191: if (context_provider_) {
On 2014/06/30 08:46:19, sohanjg wrote:
> On 2014/06/27 15:47:53, reveman wrote:
> > I don't think this check should be needed anymore if this class is limited
to
> > Gpu raster.
> 
> The class creation is already limited to gpu raster using
use_gpu_rasterization_
> flag in LTHI.
> how do we further limit it ? in GpuRasterWorkerPool::Create ?

No, you should just remove all support for non-gpu raster from the
implementation. This check for example.

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:203: }
This ContextProvider related code doesn't belong here. It should be hidden
behind some GPU raster related ResourceProvider API. But it's better to do that
in a separate patch.

https://codereview.chromium.org/328263005/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:482: case Bitmap: {
You'll have to remove this case for the the name to be correct. I think we
should remove the switch statement completely and just DCHECK_EQ(GLTexture,
resource()->type).

[sohanjg, 2014-06-30 15:02:35]

Can you please take a look.
Thanks.

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/20001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:191: if (context_provider_) {
On 2014/06/30 13:20:15, reveman wrote:
> On 2014/06/30 08:46:19, sohanjg wrote:
> > On 2014/06/27 15:47:53, reveman wrote:
> > > I don't think this check should be needed anymore if this class is limited
> to
> > > Gpu raster.
> > 
> > The class creation is already limited to gpu raster using
> use_gpu_rasterization_
> > flag in LTHI.
> > how do we further limit it ? in GpuRasterWorkerPool::Create ?
> 
> No, you should just remove all support for non-gpu raster from the
> implementation. This check for example.

Done.

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:203: }
On 2014/06/30 13:20:15, reveman wrote:
> This ContextProvider related code doesn't belong here. It should be hidden
> behind some GPU raster related ResourceProvider API. But it's better to do
that
> in a separate patch.

We already have ResourceProvider API's for glcontext and grcontext, should we
target a separate patch still ?

https://codereview.chromium.org/328263005/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:482: case Bitmap: {
On 2014/06/30 13:20:15, reveman wrote:
> You'll have to remove this case for the the name to be correct. I think we
> should remove the switch statement completely and just DCHECK_EQ(GLTexture,
> resource()->type).

Done.

[reveman, 2014-06-30 15:22:33]

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:203: }
On 2014/06/30 15:02:35, sohanjg wrote:
> On 2014/06/30 13:20:15, reveman wrote:
> > This ContextProvider related code doesn't belong here. It should be hidden
> > behind some GPU raster related ResourceProvider API. But it's better to do
> that
> > in a separate patch.
> 
> We already have ResourceProvider API's for glcontext and grcontext, should we
> target a separate patch still ?

It'd be better if this code didn't make any assumptions about how GPU raster
buffers are implemented. The GpuRasterWorkerPool name would make more sense that
way. As it is right now, this class should really be named
GpuUsingGLAndGanashRasterWorkerPool as it would do the wrong thing if GPU raster
was implemented some other way.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:37: weak_ptr_factory_(this) {
please add a DCHECK(context_provider_) here.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:459: skia::RefPtr<SkSurface> surface;
I don't think we need this temporary variable anymore.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:463: if (gr_context) {
Please early out if !gr_context instead and add "// TODO(alokp): Implement
TestContextProvider::GrContext()." just above the if statement so we know why
this conditional is needed.

[sohanjg, 2014-06-30 16:05:50]

Please take a look.
Thanks.

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:203: }
On 2014/06/30 15:22:32, reveman wrote:
> On 2014/06/30 15:02:35, sohanjg wrote:
> > On 2014/06/30 13:20:15, reveman wrote:
> > > This ContextProvider related code doesn't belong here. It should be hidden
> > > behind some GPU raster related ResourceProvider API. But it's better to do
> > that
> > > in a separate patch.
> > 
> > We already have ResourceProvider API's for glcontext and grcontext, should
we
> > target a separate patch still ?
> 
> It'd be better if this code didn't make any assumptions about how GPU raster
> buffers are implemented. The GpuRasterWorkerPool name would make more sense
that
> way. As it is right now, this class should really be named
> GpuUsingGLAndGanashRasterWorkerPool as it would do the wrong thing if GPU
raster
> was implemented some other way.

OK. I will put a separate patch for it.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/328263005/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:37: weak_ptr_factory_(this) {
On 2014/06/30 15:22:32, reveman wrote:
> please add a DCHECK(context_provider_) here.

Done.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:459: skia::RefPtr<SkSurface> surface;
On 2014/06/30 15:22:33, reveman wrote:
> I don't think we need this temporary variable anymore.

Done.

https://codereview.chromium.org/328263005/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:463: if (gr_context) {
On 2014/06/30 15:22:33, reveman wrote:
> Please early out if !gr_context instead and add "// TODO(alokp): Implement
> TestContextProvider::GrContext()." just above the if statement so we know why
> this conditional is needed.

Done.

[reveman, 2014-06-30 16:27:15]

lgtm after updating the description to make it clear that this is also limiting
the implementation to gpu raster.

[alokp, 2014-06-30 16:36:26]

lgtm

[sohanjg, 2014-06-30 18:17:42]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-06-30 18:18:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/328263005/80001

[commit-bot: I haz the power, 2014-06-30 20:12:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-30 20:19:28]

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

[commit-bot: I haz the power, 2014-06-30 20:19:29]

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

[sohanjg, 2014-07-01 01:57:00]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-07-01 01:57:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/328263005/80001

[commit-bot: I haz the power, 2014-07-01 02:07:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-01 02:14:45]

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

[commit-bot: I haz the power, 2014-07-01 02:14:46]

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

[sohanjg, 2014-07-01 05:48:34]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-07-01 05:49:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/328263005/80001

[commit-bot: I haz the power, 2014-07-01 06:07:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

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

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

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

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[sohanjg, 2014-07-01 07:18:46]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-07-01 07:21:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/328263005/100001

[commit-bot: I haz the power, 2014-07-01 09:21:23]

Change committed as 280789

[sohanjg, 2015-11-18 09:40:12]

Description was changed from

==========
cc: Rename DirectRasterWorkerPool to GpuRasterWorkerPool

This renames DirectRasterWorkerPool to GpuRasterWorkerPool 
to make it obvious that TileManager uses Gpu rasterization, 
and also limits the usage of this implementation to gpu raster.

BUG=389447

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

to

==========
cc: Rename DirectRasterWorkerPool to GpuRasterWorkerPool

This renames DirectRasterWorkerPool to GpuRasterWorkerPool 
to make it obvious that TileManager uses Gpu rasterization, 
and also limits the usage of this implementation to gpu raster.

BUG=389447

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