Support format using enum argument for Async readback

Support format using enum argument for Async readback.

As of now we are passing bool to detect the format like rgb565, but ideally the format itself should be passed.

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

[sivag, 2014-01-21 13:15:35]

PTAL...

[piman, 2014-01-21 21:30:55]

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1052: if (config ==
SkBitmap::kRGB_565_Config) {
nit: config != SkBitmap::kARGB_8888_Config

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:1139: if (config ==
SkBitmap::kRGB_565_Config) {
nit: config != SkBitmap::kARGB_8888_Config

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:358: default:
I don't think we should transparently fallback to 8888. Just assert here, and
make sure callers fail if a different config is used.

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:397: default:
Same here.

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:463: default:
And here

[sivag, 2014-01-22 10:35:08]

PTAL....

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1052: if (config ==
SkBitmap::kRGB_565_Config) {
On 2014/01/21 21:30:55, piman wrote:
> nit: config != SkBitmap::kARGB_8888_Config

Done.

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/143683003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:1139: if (config ==
SkBitmap::kRGB_565_Config) {
On 2014/01/21 21:30:55, piman wrote:
> nit: config != SkBitmap::kARGB_8888_Config

Done.

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:358: default:
On 2014/01/21 21:30:55, piman wrote:
> I don't think we should transparently fallback to 8888. Just assert here, and
> make sure callers fail if a different config is used.

Done.

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:397: default:
On 2014/01/21 21:30:55, piman wrote:
> Same here.

Done.

https://codereview.chromium.org/143683003/diff/1/content/common/gpu/client/gl...
content/common/gpu/client/gl_helper.cc:463: default:
On 2014/01/21 21:30:55, piman wrote:
> And here

Done.

[piman, 2014-01-22 22:10:20]

https://codereview.chromium.org/143683003/diff/170001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/143683003/diff/170001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1092: const
SkBitmap::Config config) {
This needs to test the config (and fail if the config is not 8888 or 565),
because GLHelper will assert if it's not correct.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:344: LOG(ERROR) << "Format not
supported.";
no need for this LOG. DCHECK is enough.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:345: return GL_INVALID_ENUM;
Maybe return 0 rather than GL_INVALID_ENUM. GL_INVALID_ENUM will look like a
valid texture id.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:370: NOTIMPLEMENTED();
nit: NOTREACHED() since it's tested above.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:398: LOG(ERROR) << "Format not
supported.";
no need for the LOG.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:408: GLenum format = GL_RGBA , type =
GL_UNSIGNED_BYTE;
nit: no space before ,

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:421: NOTIMPLEMENTED();
nit: NOTREACHED()

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:461: LOG(ERROR) << "Format not
supported.";
nit: remove.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:477: if(texture == GL_INVALID_ENUM) {
nit: you can just DCHECK(texture), since you tested above.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:499: NOTIMPLEMENTED();
NOTREACHED()

[sivag, 2014-01-23 12:29:19]

PTAL..

https://codereview.chromium.org/143683003/diff/170001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/143683003/diff/170001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1092: const
SkBitmap::Config config) {
On 2014/01/22 22:10:21, piman wrote:
> This needs to test the config (and fail if the config is not 8888 or 565),
> because GLHelper will assert if it's not correct.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:344: LOG(ERROR) << "Format not
supported.";
On 2014/01/22 22:10:21, piman wrote:
> no need for this LOG. DCHECK is enough.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:345: return GL_INVALID_ENUM;
On 2014/01/22 22:10:21, piman wrote:
> Maybe return 0 rather than GL_INVALID_ENUM. GL_INVALID_ENUM will look like a
> valid texture id.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:370: NOTIMPLEMENTED();
On 2014/01/22 22:10:21, piman wrote:
> nit: NOTREACHED() since it's tested above.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:398: LOG(ERROR) << "Format not
supported.";
On 2014/01/22 22:10:21, piman wrote:
> no need for the LOG.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:408: GLenum format = GL_RGBA , type =
GL_UNSIGNED_BYTE;
On 2014/01/22 22:10:21, piman wrote:
> nit: no space before ,

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:408: GLenum format = GL_RGBA , type =
GL_UNSIGNED_BYTE;
On 2014/01/22 22:10:21, piman wrote:
> nit: no space before ,

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:421: NOTIMPLEMENTED();
On 2014/01/22 22:10:21, piman wrote:
> nit: NOTREACHED()

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:461: LOG(ERROR) << "Format not
supported.";
On 2014/01/22 22:10:21, piman wrote:
> nit: remove.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:477: if(texture == GL_INVALID_ENUM) {
On 2014/01/22 22:10:21, piman wrote:
> nit: you can just DCHECK(texture), since you tested above.

Done.

https://codereview.chromium.org/143683003/diff/170001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:499: NOTIMPLEMENTED();
On 2014/01/22 22:10:21, piman wrote:
> NOTREACHED()

Done.

[piman, 2014-01-23 18:41:14]

LGTM after last one thing is fixed.

https://codereview.chromium.org/143683003/diff/400001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/400001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:475: DCHECK(texture);
What I meant is, you don't need to do the if (texture==0). You're already
checking, before ScaleTexture, that the conditions are correct, and ScaleTexture
will return a valid texture.

So you can omit everything, and just DCHECK your post-condition -
DCHECK(texture); - with no if, no early return etc.

[sivag, 2014-01-24 10:19:33]

Done.

https://codereview.chromium.org/143683003/diff/400001/content/common/gpu/clie...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/143683003/diff/400001/content/common/gpu/clie...
content/common/gpu/client/gl_helper.cc:475: DCHECK(texture);
On 2014/01/23 18:41:15, piman wrote:
> What I meant is, you don't need to do the if (texture==0). You're already
> checking, before ScaleTexture, that the conditions are correct, and
ScaleTexture
> will return a valid texture.
> 
> So you can omit everything, and just DCHECK your post-condition -
> DCHECK(texture); - with no if, no early return etc.

Done.

[commit-bot: I haz the power, 2014-01-24 10:20:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/143683003/330002

[commit-bot: I haz the power, 2014-01-24 11:55:49]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-24 12:25:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/143683003/330002

[commit-bot: I haz the power, 2014-01-24 14:11:14]

Change committed as 246888