Upstream: GL implementation on Android

Upstream: GL implementation on Android


BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110997

[jonathan.backer, 2011-11-18 19:33:32]

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_context_android.cc
File ui/gfx/gl/gl_context_android.cc (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_context_android.cc...
ui/gfx/gl/gl_context_android.cc:19: scoped_refptr<GLContextEGL> context(new
GLContextEGL(share_group));
Don't you want to return a GLContextStub if GetGLImplementation() is
kGLImplementationMockGL?

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface.h
File ui/gfx/gl/gl_surface.h (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface.h#newcode19
ui/gfx/gl/gl_surface.h:19: // in SurfaceTexture more.
s/more/mode?

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc
File ui/gfx/gl/gl_surface_egl.cc (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc#new...
ui/gfx/gl/gl_surface_egl.cc:193: #if defined(OS_ANDROID)
NOTREACHED()?

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc#new...
ui/gfx/gl/gl_surface_egl.cc:364: #if defined(OS_ANDROID)
NOTREACHED()?

[michaelbai, 2011-11-21 18:28:58]

Sorry for late response, all comments were addressed.
PTAL

Thanks

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_context_android.cc
File ui/gfx/gl/gl_context_android.cc (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_context_android.cc...
ui/gfx/gl/gl_context_android.cc:19: scoped_refptr<GLContextEGL> context(new
GLContextEGL(share_group));
On 2011/11/18 19:33:32, jonathan.backer wrote:
> Don't you want to return a GLContextStub if GetGLImplementation() is
> kGLImplementationMockGL?

Done.

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface.h
File ui/gfx/gl/gl_surface.h (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface.h#newcode19
ui/gfx/gl/gl_surface.h:19: // in SurfaceTexture more.
On 2011/11/18 19:33:32, jonathan.backer wrote:
> s/more/mode?

Done.

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc
File ui/gfx/gl/gl_surface_egl.cc (right):

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc#new...
ui/gfx/gl/gl_surface_egl.cc:193: #if defined(OS_ANDROID)
On 2011/11/18 19:33:32, jonathan.backer wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/8588059/diff/1/ui/gfx/gl/gl_surface_egl.cc#new...
ui/gfx/gl/gl_surface_egl.cc:364: #if defined(OS_ANDROID)
On 2011/11/18 19:33:32, jonathan.backer wrote:
> NOTREACHED()?

Done.

[jonathan.backer, 2011-11-21 18:54:45]

LGTM with nits.

AFAIK, in the CL description you should have BUG=none and TEST=none if there are
no tests.

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_implementation_...
File ui/gfx/gl/gl_implementation_android.cc (right):

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_implementation_...
ui/gfx/gl/gl_implementation_android.cc:67: // many GL functions and the
rendering is broken.
I see what you mean. glBlitFramebuffer for example. It's set in
InitializeGLBindingsGL, without querying the extension first.

There is some code later on InitializeGLExtensionBindingsGL that seems to be
doing something closer to what should be done (check for extension string before
loading).

Could you file a bug to keep track of this?

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_surface.h
File ui/gfx/gl/gl_surface.h (right):

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_surface.h#newco...
ui/gfx/gl/gl_surface.h:20: class AndroidNativeWindowInterface {
I don't see where this is referenced in this CL? Maybe put this in another CL?

[michaelbai, 2011-11-21 19:29:04]

Description was also changed.

Thanks

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_implementation_...
File ui/gfx/gl/gl_implementation_android.cc (right):

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_implementation_...
ui/gfx/gl/gl_implementation_android.cc:67: // many GL functions and the
rendering is broken.
On 2011/11/21 18:54:45, jonathan.backer wrote:
> I see what you mean. glBlitFramebuffer for example. It's set in
> InitializeGLBindingsGL, without querying the extension first.
> 
> There is some code later on InitializeGLExtensionBindingsGL that seems to be
> doing something closer to what should be done (check for extension string
before
> loading).
> 
> Could you file a bug to keep track of this?

Done. Also add issue link here.

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_surface.h
File ui/gfx/gl/gl_surface.h (right):

http://codereview.chromium.org/8588059/diff/5001/ui/gfx/gl/gl_surface.h#newco...
ui/gfx/gl/gl_surface.h:20: class AndroidNativeWindowInterface {
On 2011/11/21 18:54:45, jonathan.backer wrote:
> I don't see where this is referenced in this CL? Maybe put this in another CL?

OK, Removed

[commit-bot: I haz the power, 2011-11-21 19:37:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/8588059/8002

[commit-bot: I haz the power, 2011-11-21 21:24:16]

Change committed as 110997