cc: Refactor ResourceProvider.

cc: Refactor ResourceProvider.

This moves classes and apis which are not associated with Resource
to their caller.  We invoke the GL methods directly instead of
wrapping them, and we move the ScopedGpuRaster class out of
ResourceProvider to its own file.

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

[sohanjg, 2014-07-09 11:34:23]

Can you please take a look, if the direction is correct.
Thanks.

[danakj, 2014-07-09 16:01:34]

I think the ResourceHelper class as it is in this CL doesn't need to exist. We
can call the GL methods directly instead of wrapping them, and we can move the
ScopedGpuRaster class out to be a top-level class instead?

https://codereview.chromium.org/375303002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:1069: DCHECK_EQ(GL_TEXTURE0,
ResourceHelper::GetActiveTextureUnit(gl_));
This function is super simple I don't think we need to expose a public method do
this. Can we just add a file-static method in this file to do this and call that
instead? Same thing in ResourceProvider?

https://codereview.chromium.org/375303002/diff/1/cc/resources/image_copy_rast...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/image_copy_rast...
cc/resources/image_copy_raster_worker_pool.cc:229:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
can we just call ShallowFlushCHROMIUM directly here?

https://codereview.chromium.org/375303002/diff/1/cc/resources/pixel_buffer_ra...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/pixel_buffer_ra...
cc/resources/pixel_buffer_raster_worker_pool.cc:318:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
can we just call ShallowFlushCHROMIUM() directly here?

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_helper.h
File cc/resources/resource_helper.h (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_helper...
cc/resources/resource_helper.h:34: class CC_EXPORT ScopedGpuRaster {
How about making this a top-level class instead of nesting it?

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1302: GetResourceHelper()->Finish();
can we just call gl->Finish() directly?

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (left):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:641: gpu::gles2::GLES2Interface* ContextGL()
const;
These are private methods used internally in ResourceProvider, let's leave them.

https://codereview.chromium.org/375303002/diff/1/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/test/pixel_test.cc#newcode74
cc/test/pixel_test.cc:74: resource_provider_->GetResourceHelper()->Finish();
can we just call gl->Finish() directly?

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

https://codereview.chromium.org/375303002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1356:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
can we just call output_surface_->...->ShallowFlushCHROMIUM() directly?

https://codereview.chromium.org/375303002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1270: impl()
can we just call Finish() on the GL context directly?

[sohanjg, 2014-07-10 15:11:07]

We are still calling resourceprovider for the glcontext (where output surface
isnt available), and we had to pull grcontext and glcontext to public, to make
it accessible from scopedgpuraster. hope that is ok. Please take a look.
Thanks.

https://codereview.chromium.org/375303002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:1069: DCHECK_EQ(GL_TEXTURE0,
ResourceHelper::GetActiveTextureUnit(gl_));
On 2014/07/09 16:01:33, danakj wrote:
> This function is super simple I don't think we need to expose a public method
do
> this. Can we just add a file-static method in this file to do this and call
that
> instead? Same thing in ResourceProvider?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/resources/image_copy_rast...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/image_copy_rast...
cc/resources/image_copy_raster_worker_pool.cc:229:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
On 2014/07/09 16:01:33, danakj wrote:
> can we just call ShallowFlushCHROMIUM directly here?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/resources/pixel_buffer_ra...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/pixel_buffer_ra...
cc/resources/pixel_buffer_raster_worker_pool.cc:318:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
On 2014/07/09 16:01:33, danakj wrote:
> can we just call ShallowFlushCHROMIUM() directly here?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_helper.h
File cc/resources/resource_helper.h (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_helper...
cc/resources/resource_helper.h:34: class CC_EXPORT ScopedGpuRaster {
On 2014/07/09 16:01:33, danakj wrote:
> How about making this a top-level class instead of nesting it?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1302: GetResourceHelper()->Finish();
On 2014/07/09 16:01:33, danakj wrote:
> can we just call gl->Finish() directly?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (left):

https://codereview.chromium.org/375303002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:641: gpu::gles2::GLES2Interface* ContextGL()
const;
On 2014/07/09 16:01:33, danakj wrote:
> These are private methods used internally in ResourceProvider, let's leave
them.

Done.

Had to pull them to public though, because ScopedGpuRaster would need them, and
we had pulled that class out of ResourceProvider.

https://codereview.chromium.org/375303002/diff/1/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/test/pixel_test.cc#newcode74
cc/test/pixel_test.cc:74: resource_provider_->GetResourceHelper()->Finish();
On 2014/07/09 16:01:33, danakj wrote:
> can we just call gl->Finish() directly?

Done.

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

https://codereview.chromium.org/375303002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1356:
resource_provider_->GetResourceHelper()->ShallowFlushIfSupported();
On 2014/07/09 16:01:33, danakj wrote:
> can we just call output_surface_->...->ShallowFlushCHROMIUM() directly?

Done.

https://codereview.chromium.org/375303002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1270: impl()
On 2014/07/09 16:01:33, danakj wrote:
> can we just call Finish() on the GL context directly?

Done.

[danakj, 2014-07-10 17:01:44]

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

https://codereview.chromium.org/375303002/diff/40001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:136: static GLint
GetActiveTextureUnit(gpu::gles2::GLES2Interface* gl);
why public and a member of the class? just make it a static in the .cc file?

https://codereview.chromium.org/375303002/diff/40001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:29:
ImageCopyRasterWorkerPool::ImageCopyRasterWorkerPool(
We can pass the ContextProvider to this class, then we don't have to dig it out
of the ResourceProvider.

https://codereview.chromium.org/375303002/diff/40001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:42:
PixelBufferRasterWorkerPool::PixelBufferRasterWorkerPool(
We can pass the ContextProvider* to this class along with the ResourceProvider*

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

https://codereview.chromium.org/375303002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2252:
ScopedGpuRaster::ScopedGpuRaster(ResourceProvider* resource_provider)
This class should take a ContextProvider* instead of a ResourceProvider*

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

https://codereview.chromium.org/375303002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.h:658: class CC_EXPORT ScopedGpuRaster {
move to its own file, 1 class per file

https://codereview.chromium.org/375303002/diff/40001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:77: GLES2Interface* gl = resource_provider_->ContextGL();
use the output_surface_->context_provider() instead of the resource_provider_

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

https://codereview.chromium.org/375303002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1358: if (output_surface_ && !visible_) {
you'll have to check that context_provider() is not null too, it will be when in
software mode

https://codereview.chromium.org/375303002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1273: impl().layer_tree_host_impl->resource_provider();
use the lthi->output_surface() instead, and null check the context_provider()

[sohanjg, 2014-07-11 16:43:33]

Can you please take a look,
Thanks.

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

https://codereview.chromium.org/375303002/diff/40001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:136: static GLint
GetActiveTextureUnit(gpu::gles2::GLES2Interface* gl);
On 2014/07/10 17:01:43, danakj wrote:
> why public and a member of the class? just make it a static in the .cc file?

Done.

https://codereview.chromium.org/375303002/diff/40001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:29:
ImageCopyRasterWorkerPool::ImageCopyRasterWorkerPool(
On 2014/07/10 17:01:43, danakj wrote:
> We can pass the ContextProvider to this class, then we don't have to dig it
out
> of the ResourceProvider.

Done.

https://codereview.chromium.org/375303002/diff/40001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:42:
PixelBufferRasterWorkerPool::PixelBufferRasterWorkerPool(
On 2014/07/10 17:01:43, danakj wrote:
> We can pass the ContextProvider* to this class along with the
ResourceProvider*

Done.

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

https://codereview.chromium.org/375303002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:2252:
ScopedGpuRaster::ScopedGpuRaster(ResourceProvider* resource_provider)
On 2014/07/10 17:01:44, danakj wrote:
> This class should take a ContextProvider* instead of a ResourceProvider*

Done.

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

https://codereview.chromium.org/375303002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.h:658: class CC_EXPORT ScopedGpuRaster {
On 2014/07/10 17:01:44, danakj wrote:
> move to its own file, 1 class per file

Done.

https://codereview.chromium.org/375303002/diff/40001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:77: GLES2Interface* gl = resource_provider_->ContextGL();
On 2014/07/10 17:01:44, danakj wrote:
> use the output_surface_->context_provider() instead of the resource_provider_

Done.

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

https://codereview.chromium.org/375303002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1358: if (output_surface_ && !visible_) {
On 2014/07/10 17:01:44, danakj wrote:
> you'll have to check that context_provider() is not null too, it will be when
in
> software mode

Done.

https://codereview.chromium.org/375303002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1273: impl().layer_tree_host_impl->resource_provider();
On 2014/07/10 17:01:44, danakj wrote:
> use the lthi->output_surface() instead, and null check the context_provider()

Done.

[danakj, 2014-07-11 16:49:05]

https://codereview.chromium.org/375303002/diff/60001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:240: GLES2Interface* gl =
context_provider_->ContextGL();
if (context_provider_)
  context_provider_->ContextGL()->ShallowFlushCHROMIUM()

https://codereview.chromium.org/375303002/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:325: GLES2Interface* gl =
resource_provider_->ContextGL();
same

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

https://codereview.chromium.org/375303002/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.h:369: // Returns NULL if the output_surface_
does not have a ContextProvider.
don't need to be public?

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:26: DCHECK(gl);
not needed

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:34: if (gr_context)
nto needed

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:40: DCHECK(gl);
not needed

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:43: if (gr_context)
not needed

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

https://codereview.chromium.org/375303002/diff/60001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:77: GLES2Interface* gl =
output_surface_->context_provider()->ContextGL();
if context_provider() isn't null. it's null when software compositing.

https://codereview.chromium.org/375303002/diff/60001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:78: if (gl)
don't need to check gl though

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

https://codereview.chromium.org/375303002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1380: if (output_surface_ && !visible_) {
if os and os->context_provider() && !visible

https://codereview.chromium.org/375303002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1382: if (gl)
don't need to check gl

https://codereview.chromium.org/375303002/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1284: if (gl)
don't need

[sohanjg, 2014-07-11 17:12:46]

Can you please take a look.
Thanks.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:240: GLES2Interface* gl =
context_provider_->ContextGL();
On 2014/07/11 16:49:04, danakj wrote:
> if (context_provider_)
>   context_provider_->ContextGL()->ShallowFlushCHROMIUM()

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:325: GLES2Interface* gl =
resource_provider_->ContextGL();
On 2014/07/11 16:49:04, danakj wrote:
> same

Done.

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

https://codereview.chromium.org/375303002/diff/60001/cc/resources/resource_pr...
cc/resources/resource_provider.h:369: // Returns NULL if the output_surface_
does not have a ContextProvider.
On 2014/07/11 16:49:04, danakj wrote:
> don't need to be public?

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:26: DCHECK(gl);
On 2014/07/11 16:49:04, danakj wrote:
> not needed

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:34: if (gr_context)
On 2014/07/11 16:49:04, danakj wrote:
> nto needed

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:40: DCHECK(gl);
On 2014/07/11 16:49:04, danakj wrote:
> not needed

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:43: if (gr_context)
On 2014/07/11 16:49:04, danakj wrote:
> not needed

Done.

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

https://codereview.chromium.org/375303002/diff/60001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:77: GLES2Interface* gl =
output_surface_->context_provider()->ContextGL();
On 2014/07/11 16:49:04, danakj wrote:
> if context_provider() isn't null. it's null when software compositing.

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/test/pixel_test.cc#ne...
cc/test/pixel_test.cc:78: if (gl)
On 2014/07/11 16:49:04, danakj wrote:
> don't need to check gl though

Done.

https://codereview.chromium.org/375303002/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/60001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1284: if (gl)
On 2014/07/11 16:49:04, danakj wrote:
> don't need

Done.

[danakj, 2014-07-11 17:39:59]

https://codereview.chromium.org/375303002/diff/80001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:242: 
nit: extra whitespace

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
2014

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.h (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
2014

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

https://codereview.chromium.org/375303002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1380: if (output_surface_ && !visible_) {
check if the context_provider is NULL instead.

this function is part of the OutputSurfaceClient API, so the output_surface_
itself can't be null.

https://codereview.chromium.org/375303002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1382: if (gl)
not needed

[sohanjg, 2014-07-12 11:08:26]

Can you please take a look.
Thanks.

https://codereview.chromium.org/375303002/diff/80001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:242: 
On 2014/07/11 17:39:59, danakj wrote:
> nit: extra whitespace

Done.

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.cc (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.cc:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/07/11 17:39:59, danakj wrote:
> 2014

Done.

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
File cc/resources/scoped_gpu_raster.h (right):

https://codereview.chromium.org/375303002/diff/80001/cc/resources/scoped_gpu_...
cc/resources/scoped_gpu_raster.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
On 2014/07/11 17:39:59, danakj wrote:
> 2014

Done.

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

https://codereview.chromium.org/375303002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1380: if (output_surface_ && !visible_) {
On 2014/07/11 17:39:59, danakj wrote:
> check if the context_provider is NULL instead.
> 
> this function is part of the OutputSurfaceClient API, so the output_surface_
> itself can't be null.

Done.

https://codereview.chromium.org/375303002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1380: if (output_surface_ && !visible_) {
On 2014/07/11 17:39:59, danakj wrote:
> check if the context_provider is NULL instead.
> 
> this function is part of the OutputSurfaceClient API, so the output_surface_
> itself can't be null.

Done.

https://codereview.chromium.org/375303002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1382: if (gl)
On 2014/07/11 17:39:59, danakj wrote:
> not needed

Done.

[danakj, 2014-07-14 15:29:12]

https://codereview.chromium.org/375303002/diff/100001/cc/resources/image_copy...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/resources/image_copy...
cc/resources/image_copy_raster_worker_pool.cc:15: using
gpu::gles2::GLES2Interface;
not needed?

https://codereview.chromium.org/375303002/diff/100001/cc/resources/pixel_buff...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/resources/pixel_buff...
cc/resources/pixel_buffer_raster_worker_pool.cc:15: using
gpu::gles2::GLES2Interface;
not needed?

https://codereview.chromium.org/375303002/diff/100001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/test/pixel_test.cc#n...
cc/test/pixel_test.cc:31: using gpu::gles2::GLES2Interface;
not needed?

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

https://codereview.chromium.org/375303002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:73: using gpu::gles2::GLES2Interface;
not needed?

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:31: using gpu::gles2::GLES2Interface;
not needed?

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1279: if
(impl().layer_tree_host_impl->output_surface()->context_provider())
need {} for multi-line body.

how about a temp var for the ContextProvider to make this all read nicer?

[sohanjg, 2014-07-14 15:59:28]

Can you please take a look,
Thanks.

https://codereview.chromium.org/375303002/diff/100001/cc/resources/image_copy...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/resources/image_copy...
cc/resources/image_copy_raster_worker_pool.cc:15: using
gpu::gles2::GLES2Interface;
On 2014/07/14 15:29:11, danakj wrote:
> not needed?

Done.

https://codereview.chromium.org/375303002/diff/100001/cc/resources/pixel_buff...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/resources/pixel_buff...
cc/resources/pixel_buffer_raster_worker_pool.cc:15: using
gpu::gles2::GLES2Interface;
On 2014/07/14 15:29:11, danakj wrote:
> not needed?

Done.

https://codereview.chromium.org/375303002/diff/100001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/test/pixel_test.cc#n...
cc/test/pixel_test.cc:31: using gpu::gles2::GLES2Interface;
On 2014/07/14 15:29:11, danakj wrote:
> not needed?

Done.

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

https://codereview.chromium.org/375303002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:73: using gpu::gles2::GLES2Interface;
On 2014/07/14 15:29:11, danakj wrote:
> not needed?

Done.

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:31: using gpu::gles2::GLES2Interface;
On 2014/07/14 15:29:12, danakj wrote:
> not needed?

Done.

https://codereview.chromium.org/375303002/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1279: if
(impl().layer_tree_host_impl->output_surface()->context_provider())
On 2014/07/14 15:29:11, danakj wrote:
> need {} for multi-line body.
> 
> how about a temp var for the ContextProvider to make this all read nicer?

Done.

[danakj, 2014-07-14 16:29:19]

LGTM 1 more thing.

https://codereview.chromium.org/375303002/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/140001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1278:
impl().layer_tree_host_impl->output_surface()->context_provider();
Oh, I think you need to null-check the OutputSurface too, it could be NULL and
you'd crash then (sorry).

[danakj, 2014-07-14 16:29:33]

Can you update the patch title/description?

[sohanjg, 2014-07-14 16:44:47]

Can you please take a look.
Thanks.

https://codereview.chromium.org/375303002/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/375303002/diff/140001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1278:
impl().layer_tree_host_impl->output_surface()->context_provider();
On 2014/07/14 16:29:19, danakj wrote:
> Oh, I think you need to null-check the OutputSurface too, it could be NULL and
> you'd crash then (sorry).

Done.
No problem :)

[danakj, 2014-07-14 20:32:40]

LGTM

[danakj, 2014-07-14 20:32:51]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-07-14 20:34:21]

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

[commit-bot: I haz the power, 2014-07-15 00:31:28]

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...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-15 10:29:49]

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

[commit-bot: I haz the power, 2014-07-15 10:29:50]

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-15 12:25:42]

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

[commit-bot: I haz the power, 2014-07-15 12:27:47]

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

[sohanjg, 2014-07-16 07:34:52]

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

[sohanjg, 2014-07-16 07:54:23]

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

[commit-bot: I haz the power, 2014-07-16 07:55:54]

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

[sohanjg, 2014-07-16 09:43:52]

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

[sohanjg, 2014-07-16 09:47:37]

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

[commit-bot: I haz the power, 2014-07-16 09:49:52]

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

[commit-bot: I haz the power, 2014-07-16 13:21:20]

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

[commit-bot: I haz the power, 2014-07-16 15:52:50]

Change committed as 283404

[sohanjg, 2015-11-18 09:46:57]

Description was changed from

==========
cc: Refactor ResourceProvider.

This moves classes and apis which are not associated with Resource
to their caller.  We invoke the GL methods directly instead of
wrapping them, and we move the ScopedGpuRaster class out of
ResourceProvider to its own file.

BUG=391190

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

to

==========
cc: Refactor ResourceProvider.

This moves classes and apis which are not associated with Resource
to their caller.  We invoke the GL methods directly instead of
wrapping them, and we move the ScopedGpuRaster class out of
ResourceProvider to its own file.

BUG=391190

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