Port content::GLHelper over to GLES2Interface

Port content::GLHelper over to GLES2Interface

This is necessary to remove the WebGraphicsContext3D getter from
cc::ContextProvider.

TBR=maruel@ for trivial PRESUBMIT.py change
BUG=181120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243499

[danakj, 2013-12-19 23:40:45]

LG mostly but a few questions as a psuedo-lesser-piman.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:110: " " +
std::string(reinterpret_cast<const char*>(extensions)) + " ";
Should this be on ContextProvider::Capabilities? If so maybe we should be
sending a ContextProvider* in here instead of a GLES2Interface*/ContextSupport*
pair.

Out of scope for this CL but something to chew on.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:580: GLuint shader =
gl_->CreateShader(type);
I see you removed ScopedShader. If this was the only use I thought that's kinda
unfortunate but ok. But I see it was not. This function should really be
returning a ScopedShader if anything..

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:587: // LOG(ERROR) <<
std::string(gl_->getShaderInfoLog(shader));
I doubt this was meant to be left as a comment?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.h (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:37: typedef void
(gpu::gles2::GLES2Interface::*GenFunc)(GLsizei, GLuint*);
can you give these arguments variable names?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:92: typedef void
(gpu::gles2::GLES2Interface::*BindFunc)(GLenum, GLuint);
can you give these arguments variable names?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:207: // current gl.
Hm, this reads kinda weird, but no better suggestion. If you have any ideas to
make this read better, that'd be cool. If not then ok.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (left):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:75: ScopedProgram program_;
Why'd ScopedProgram go away?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:877: ScopedShader
vertex_shader(context_, helper_->CompileShaderFromSource(
Why did ScopedShader go away? I think this is a loss here.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:495: -1.0f, -1.0f, 0.0f, 0.0f,
1.0f, -1.0f, 1.0f, 0.0f,
Nitty, but hm, clangformat not dealing with a matrix well here. Wonder if a [][]
would format any better due to nested {}s?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:828: void
ShaderProgram::Setup(const GLchar* vertex_shader_text,
If this is going to return void, should it be DCHECKing that it succeeded if the
context is not lost? We kinda lost the ability to poll for loss right? How can
we verify the shaders are not broken in the future?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:842:
gl_->DeleteShader(vertex_shader);
You already deleted the vertex shader above didn't you? (scoped stuff was nice
for this don't you think?)

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:880: 2 * sizeof(GLfloat))  //
NOLINT(runtime/sizeof
missing )

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:887: 4 * sizeof(GLfloat),
Do you need NOLINT here? Or do you not need it above?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:895: 4 * sizeof(GLfloat),
Do you need NOLINT here? Or do you not need it above?

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:63:
WebGraphicsContext3DInProcessCommandBufferImpl::CreateOffscreenContext(
Can you re-use cc's TestContextProviderInProcess to avoid WGC3D types in here?

[jamesr, 2013-12-19 23:47:06]

The problem with the various scoped helpers is that they assume there's a
uniform API to create/destroy the various resources they manage, which is
somewhat true in blink::WebGraphicsContext3D because we add helper
createFoo()/deleteFoo() functions, but isn't true for actual OpenGL.  For
instance a texture is created/deleted with glGenTextures(1, &id) and 
glDeleteTextures(1, &id) but a program is created/deleted with
glCreateProgram(void) / glDeleteProgram(id) and a shader with
glCreateShader(type) / glDeleteShader(id).  When taking these different
signatures into account it turns out that most of the scoped helpers cannot
actually share code with each other which, given how rarely most of them are
used, means that having a generic scoped helper is a lot more code than just
making the GL calls directly.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:110: " " +
std::string(reinterpret_cast<const char*>(extensions)) + " ";
On 2013/12/19 23:40:45, danakj wrote:
> Should this be on ContextProvider::Capabilities? If so maybe we should be
> sending a ContextProvider* in here instead of a
GLES2Interface*/ContextSupport*
> pair.

It probably should be.  Good fodder for a follow-up.

> 
> Out of scope for this CL but something to chew on.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:828: void
ShaderProgram::Setup(const GLchar* vertex_shader_text,
On 2013/12/19 23:40:45, danakj wrote:
> If this is going to return void, should it be DCHECKing that it succeeded if
the
> context is not lost? We kinda lost the ability to poll for loss right? How can
> we verify the shaders are not broken in the future?

I do DCHECK(program->Initialized()) before the UseProgram() call.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:842:
gl_->DeleteShader(vertex_shader);
On 2013/12/19 23:40:45, danakj wrote:
> You already deleted the vertex shader above didn't you?
Whoops, yes.


> (scoped stuff was nice for this don't you think?)

would be if it worked

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_unittests.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_unittests.cc:63:
WebGraphicsContext3DInProcessCommandBufferImpl::CreateOffscreenContext(
On 2013/12/19 23:40:45, danakj wrote:
> Can you re-use cc's TestContextProviderInProcess to avoid WGC3D types in here?

Maybe, but one thing at a time.

[danakj, 2013-12-19 23:57:39]

On 2013/12/19 23:47:06, jamesr wrote:
> The problem with the various scoped helpers is that they assume there's a
> uniform API to create/destroy the various resources they manage, which is
> somewhat true in blink::WebGraphicsContext3D because we add helper
> createFoo()/deleteFoo() functions, but isn't true for actual OpenGL.  For
> instance a texture is created/deleted with glGenTextures(1, &id) and 
> glDeleteTextures(1, &id) but a program is created/deleted with
> glCreateProgram(void) / glDeleteProgram(id) and a shader with
> glCreateShader(type) / glDeleteShader(id).  When taking these different
> signatures into account it turns out that most of the scoped helpers cannot
> actually share code with each other which, given how rarely most of them are
> used, means that having a generic scoped helper is a lot more code than just
> making the GL calls directly.

I see what you're saying. These two are the odd ones out and can't directly use
the new ScopedGLuint base class. We'd need a ScopedGLuintGenerator interface
that could create a single GLuint and implement it for the GenTextures(1, &a)
and CreateProgram() type things. Probably overkill..

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:828: void
ShaderProgram::Setup(const GLchar* vertex_shader_text,
On 2013/12/19 23:47:07, jamesr wrote:
> On 2013/12/19 23:40:45, danakj wrote:
> > If this is going to return void, should it be DCHECKing that it succeeded if
> the
> > context is not lost? We kinda lost the ability to poll for loss right? How
can
> > we verify the shaders are not broken in the future?
> 
> I do DCHECK(program->Initialized()) before the UseProgram() call.

Ah I see. Cool.

[jamesr, 2013-12-20 23:38:16]

PTAL

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.cc:587: // LOG(ERROR) <<
std::string(gl_->getShaderInfoLog(shader));
On 2013/12/19 23:40:45, danakj wrote:
> I doubt this was meant to be left as a comment?

Added back - it just takes a lot of lines to actually invoke GetShaderInfoLog()
and I didn't have it in place initially

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper.h (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:37: typedef void
(gpu::gles2::GLES2Interface::*GenFunc)(GLsizei, GLuint*);
On 2013/12/19 23:40:45, danakj wrote:
> can you give these arguments variable names?

Done.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:92: typedef void
(gpu::gles2::GLES2Interface::*BindFunc)(GLenum, GLuint);
On 2013/12/19 23:40:45, danakj wrote:
> can you give these arguments variable names?

Done.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper.h:207: // current gl.
On 2013/12/19 23:40:45, danakj wrote:
> Hm, this reads kinda weird, but no better suggestion. If you have any ideas to
> make this read better, that'd be cool. If not then ok.

replace "this current gl" with "the current OpenGL context"

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:495: -1.0f, -1.0f, 0.0f, 0.0f,
1.0f, -1.0f, 1.0f, 0.0f,
On 2013/12/19 23:40:45, danakj wrote:
> Nitty, but hm, clangformat not dealing with a matrix well here. Wonder if a
[][]
> would format any better due to nested {}s?

Improved the formatting a bit with comments

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:880: 2 * sizeof(GLfloat))  //
NOLINT(runtime/sizeof
On 2013/12/19 23:40:45, danakj wrote:
> missing )

turns out this isn't needed

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:887: 4 * sizeof(GLfloat),
On 2013/12/19 23:40:45, danakj wrote:
> Do you need NOLINT here? Or do you not need it above?

Done.

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:895: 4 * sizeof(GLfloat),
On 2013/12/19 23:40:45, danakj wrote:
> Do you need NOLINT here? Or do you not need it above?

Done.

[danakj, 2013-12-20 23:45:23]

LGTM

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gl_helper_scaling.cc (right):

https://codereview.chromium.org/117233006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gl_helper_scaling.cc:495: -1.0f, -1.0f, 0.0f, 0.0f,
1.0f, -1.0f, 1.0f, 0.0f,
On 2013/12/20 23:38:17, jamesr wrote:
> On 2013/12/19 23:40:45, danakj wrote:
> > Nitty, but hm, clangformat not dealing with a matrix well here. Wonder if a
> [][]
> > would format any better due to nested {}s?
> 
> Improved the formatting a bit with comments

FTW!

[commit-bot: I haz the power, 2013-12-20 23:59:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/240001

[jam, 2013-12-21 00:17:56]

rubberstamp lgtm

[no sievers, 2013-12-21 00:18:40]

content/common/gpu LGTM

[commit-bot: I haz the power, 2013-12-21 00:21:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-21 00:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/240001

[commit-bot: I haz the power, 2013-12-21 00:42:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jamesr, 2013-12-21 00:43:40]

will tbr maruel@ on the PRESUBMIT.py change - it's just marking another
benchmark file as exempt from the printf() restriction.

[commit-bot: I haz the power, 2013-12-21 00:44:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/240001

[commit-bot: I haz the power, 2013-12-21 01:41:46]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-07 09:56:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/540001

[commit-bot: I haz the power, 2014-01-07 11:40:33]

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

[commit-bot: I haz the power, 2014-01-07 23:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/540001

[commit-bot: I haz the power, 2014-01-08 00:39:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/117233006/910001

[piman, 2014-01-08 02:18:26]

lgtm

[commit-bot: I haz the power, 2014-01-08 06:25:27]

Change committed as 243499

[teambuildvngroup, 2017-07-31 09:16:35]

Thank you !

http://acdc40.com/pixal-evolution-review-bonus/

http://goo.gl/ZSr5xM

https://www.youtube.com/watch?v=msSaqLGeFlY

http://www.dailymotion.com/video/x5vbt70

http://lanyrd.com/2017/flipside-profits-review-2/calls/qfmfz/

https://www.google.com/maps/d/viewer?mid=1Gg012hJKBVoEq1lGnlgR0YB5_hc&usp=sha...

https://mary--rose.tumblr.com/

https://medium.com/@p1videomagnet/pixal-evolution-review-bonus-8980efead9e2

https://www.linkedin.com/pulse/what-pixal-evolution-my-honest-review-best-bon...

https://twitter.com/chinhmarketing/status/891665309521903616

https://repeatingdays-lourdes.tumblr.com/post/163598795031/pixal-evolution-li...

https://www.reddit.com/r/Fun/comments/6qhl4m/pixal_evolution_review_pixal_evo...

https://plus.google.com/u/2/+ChinhTony/posts/VuBcoF9WyeW

https://plus.google.com/u/2/+ChinhTony/posts/QEnPUByWLLv

http://amara.org/en/videos/nBewgIIa5rGu/info/pixal-evolution-review-pixal-evo...

https://vk.com/wall439007461_2

https://vk.com/wall439007461_3

http://husc.edu.vn/forum/viewthread.php?thread_id=475337

https://tackk.com/tj2fjt

https://www.changemakers.com/users/pixal-evolution-review-and-new-bonus

http://listography.com/7979525933/notes/pixal_evolution_review___100%25_best_...

http://groupspaces.com/myreview/pages/pixal-evolution-review-and-exclusive-bo...

https://onmogul.com/ask_anythings/what-is-pixal-evolution-review

http://www.theglobalexperience.org/reports/show/1313

https://slides.com/pixalevolutionreviewbonus

https://zeegal.com/p/pixel-evolution-review-best-bonus-2017/

https://jira.atlassian.com/browse/BAMJ-2959

https://www.youtube.com/watch?v=TUvApFZXO0Q

https://sites.google.com/site/imreviewbonusblog/pixal-evolution-review-bonuses

https://sites.google.com/site/imreviewbonusblog/