Update the native_viewport interface to allow specification of the surface configuration, currently…

Update the native_viewport interface to allow specification of the surface configuration, currently only needed for and used by EGL on Android.

This also fixes an issue where eglChooseConfig was
only being called in InitializeOneOff, which is
only called once per process.  This CL makes
choosing the config happen once per GLSurface
instead, which will ultimately permit apps to
create multiple native_viewports with different
surface configurations on the same display.  The
eglDisplay object is still a global, though.

R=abarth@chromium.org, viettrungluu@chromium.org, jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/f66f4370dd9eef42cec3ec11cd654934ea38c3d4

[iansf, 2015-06-09 00:10:42]

This replaces issue 1162003008, which I am closing.

[abarth-chromium, 2015-06-09 00:30:22]

This looks great!  LGTM

I didn't mark all the nullptrs and const Foo& changes, but hopefully you see the
pattern.

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/in_process_command_buffer.cc:347:
gfx::SurfaceConfiguration requested_configuration) {
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/in_process_command_buffer.h:88:
gfx::SurfaceConfiguration requested_configuration);
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&  ?

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/in_process_command_buffer.h:183:
gfx::SurfaceConfiguration requested_configuration)
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/mojo/converters/native_view...
File mojo/converters/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/1168993002/diff/1/mojo/converters/native_view...
mojo/converters/native_viewport/BUILD.gn:6: output_name =
"mojo_native_viewport_lib"
You can skip this output_name.  The GN system will pick a reasonable name for
you.

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
File services/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.cc:57: gfx::SurfaceConfiguration
requested_configuration)
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.cc:70: gfx::SurfaceConfiguration
requested_configuration)
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
File services/gles2/command_buffer_driver.h (right):

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.h:51: gfx::SurfaceConfiguration
requested_configuration);
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.h:57: gfx::SurfaceConfiguration
requested_configuration);
gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/na...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/na...
services/native_viewport/native_viewport_impl.cc:20: #include
"ui/gl/gl_surface_egl.h"
Do we need gl_implementation.h and gl_surface_egl.h?  It seems like gl_surface.h
ought to be enough.

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
File services/native_viewport/onscreen_context_provider.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
services/native_viewport/onscreen_context_provider.cc:51: }
This function should be defined in the header and should be named
set_surface_configuration.  The difference between StudlyCaps and unix_hacker
names is that unix_hacker names are for trivial getters/setters.  That way you
can see from the call site that calling set_surface_configuration won't have any
nutty side effects.  By contrast SetAcceleratedWidget causes all kinds of side
effects and hence has the StudlyCaps name.

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
File services/native_viewport/onscreen_context_provider.h (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
services/native_viewport/onscreen_context_provider.h:31:
gfx::SurfaceConfiguration requested_configuration);
const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode34
ui/gl/gl_surface.h:34: stencil_bits(0) {}
You can delete this constructor and just write:

uint8_t red_bits = 8;

below.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode175
ui/gl/gl_surface.h:175: gfx::SurfaceConfiguration requested_configuration);
const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode184
ui/gl/gl_surface.h:184: gfx::SurfaceConfiguration requested_configuration);
const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode190
ui/gl/gl_surface.h:190: gfx::SurfaceConfiguration requested_configuration);
const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode199
ui/gl/gl_surface.h:199: };
const GetSurfaceConfiguration&

s/GetSurfaceConfiguration/surface_configuration/

Again, this is a case of using a unix_hacker name for a trivial getter.  If the
function was virtual, then using the StudlyCaps name would be correct.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc
File ui/gl/gl_surface_android.cc (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc...
ui/gl/gl_surface_android.cc:35: SurfaceConfiguration requested_configuration) {
const gfx::SurfaceConfiguration&

Below you use the gfx:: namespace.  We should make it consistent one way or
another.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc...
ui/gl/gl_surface_android.cc:62: gfx::SurfaceConfiguration
requested_configuration) {
const gfx::SurfaceConfiguration&

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:183: return NULL;
nullptr

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:193: return NULL;
ditto

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:550: config_(NULL) {
nullptr

[viettrungluu, 2015-06-09 00:48:14]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-06-09 00:48:15]

Drive-by questions (plus nits)....

https://codereview.chromium.org/1168993002/diff/20001/mojo/services/native_vi...
File mojo/services/native_viewport/public/interfaces/native_viewport.mojom
(right):

https://codereview.chromium.org/1168993002/diff/20001/mojo/services/native_vi...
mojo/services/native_viewport/public/interfaces/native_viewport.mojom:28:
Create(Size size, SurfaceConfiguration requested_configuration) =>
(ViewportMetrics metrics);
Did you consider letting this parameter be nullable (with null meaning the
default)?

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
services/native_viewport/native_viewport_impl.cc:50:
requested_configuration.To<gfx::SurfaceConfiguration>());
Is there any validation of requested_configuration that you should do here?

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:199: };
no semicolon

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:213: SurfaceConfiguration surface_configuration_;
nit: I believe members should be declared after friends.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:25: GLSurfaceEGL(SurfaceConfiguration
requested_configuration);
nit: explicit

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:53: explicit NativeViewGLSurfaceEGL(EGLNativeWindowType
window,
nit: explicit no longer needed

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:97: explicit PbufferGLSurfaceEGL(const gfx::Size& size,
"

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:127: explicit SurfacelessEGL(const gfx::Size& size,
"

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_glx.h
File ui/gl/gl_surface_glx.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_glx.h#...
ui/gl/gl_surface_glx.h:24: GLSurfaceGLX(SurfaceConfiguration
requested_configuration);
explicit

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_stub.h
File ui/gl/gl_surface_stub.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_stub.h...
ui/gl/gl_surface_stub.h:15: GLSurfaceStub(SurfaceConfiguration
requested_configuration);
explicit

[iansf, 2015-06-09 01:52:05]

Thanks for the comments, both of you!  Tomorrow I'll get the linux builders
working and look into making some basic validation for the egl configs as
suggested.

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1168993002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/in_process_command_buffer.cc:347:
gfx::SurfaceConfiguration requested_configuration) {
On 2015/06/09 00:30:21, abarth wrote:
> gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/mojo/converters/native_view...
File mojo/converters/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/1168993002/diff/1/mojo/converters/native_view...
mojo/converters/native_viewport/BUILD.gn:6: output_name =
"mojo_native_viewport_lib"
On 2015/06/09 00:30:21, abarth wrote:
> You can skip this output_name.  The GN system will pick a reasonable name for
> you.

Done.

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
File services/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.cc:57: gfx::SurfaceConfiguration
requested_configuration)
On 2015/06/09 00:30:21, abarth wrote:
> gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.cc:70: gfx::SurfaceConfiguration
requested_configuration)
On 2015/06/09 00:30:21, abarth wrote:
> gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
File services/gles2/command_buffer_driver.h (right):

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.h:51: gfx::SurfaceConfiguration
requested_configuration);
On 2015/06/09 00:30:21, abarth wrote:
> gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/services/gles2/command_buff...
services/gles2/command_buffer_driver.h:57: gfx::SurfaceConfiguration
requested_configuration);
On 2015/06/09 00:30:21, abarth wrote:
> gfx::SurfaceConfiguration -> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/na...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/na...
services/native_viewport/native_viewport_impl.cc:20: #include
"ui/gl/gl_surface_egl.h"
On 2015/06/09 00:30:21, abarth wrote:
> Do we need gl_implementation.h and gl_surface_egl.h?  It seems like
gl_surface.h
> ought to be enough.

Done.

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
File services/native_viewport/onscreen_context_provider.cc (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
services/native_viewport/onscreen_context_provider.cc:51: }
On 2015/06/09 00:30:21, abarth wrote:
> This function should be defined in the header and should be named
> set_surface_configuration.  The difference between StudlyCaps and unix_hacker
> names is that unix_hacker names are for trivial getters/setters.  That way you
> can see from the call site that calling set_surface_configuration won't have
any
> nutty side effects.  By contrast SetAcceleratedWidget causes all kinds of side
> effects and hence has the StudlyCaps name.

Done.

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
File services/native_viewport/onscreen_context_provider.h (right):

https://codereview.chromium.org/1168993002/diff/1/services/native_viewport/on...
services/native_viewport/onscreen_context_provider.h:31:
gfx::SurfaceConfiguration requested_configuration);
On 2015/06/09 00:30:21, abarth wrote:
> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode34
ui/gl/gl_surface.h:34: stencil_bits(0) {}
On 2015/06/09 00:30:22, abarth wrote:
> You can delete this constructor and just write:
> 
> uint8_t red_bits = 8;
> 
> below.

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode175
ui/gl/gl_surface.h:175: gfx::SurfaceConfiguration requested_configuration);
On 2015/06/09 00:30:22, abarth wrote:
> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode184
ui/gl/gl_surface.h:184: gfx::SurfaceConfiguration requested_configuration);
On 2015/06/09 00:30:22, abarth wrote:
> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode190
ui/gl/gl_surface.h:190: gfx::SurfaceConfiguration requested_configuration);
On 2015/06/09 00:30:21, abarth wrote:
> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface.h#newcode199
ui/gl/gl_surface.h:199: };
On 2015/06/09 00:30:21, abarth wrote:
> const GetSurfaceConfiguration&
> 
> s/GetSurfaceConfiguration/surface_configuration/
> 
> Again, this is a case of using a unix_hacker name for a trivial getter.  If
the
> function was virtual, then using the StudlyCaps name would be correct.

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc
File ui/gl/gl_surface_android.cc (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc...
ui/gl/gl_surface_android.cc:35: SurfaceConfiguration requested_configuration) {
On 2015/06/09 00:30:22, abarth wrote:
> const gfx::SurfaceConfiguration&
> 
> Below you use the gfx:: namespace.  We should make it consistent one way or
> another.

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_android.cc...
ui/gl/gl_surface_android.cc:62: gfx::SurfaceConfiguration
requested_configuration) {
On 2015/06/09 00:30:22, abarth wrote:
> const gfx::SurfaceConfiguration&

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:183: return NULL;
On 2015/06/09 00:30:22, abarth wrote:
> nullptr

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:193: return NULL;
On 2015/06/09 00:30:22, abarth wrote:
> ditto

Done.

https://codereview.chromium.org/1168993002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:550: config_(NULL) {
On 2015/06/09 00:30:22, abarth wrote:
> nullptr

Done.

https://codereview.chromium.org/1168993002/diff/20001/mojo/services/native_vi...
File mojo/services/native_viewport/public/interfaces/native_viewport.mojom
(right):

https://codereview.chromium.org/1168993002/diff/20001/mojo/services/native_vi...
mojo/services/native_viewport/public/interfaces/native_viewport.mojom:28:
Create(Size size, SurfaceConfiguration requested_configuration) =>
(ViewportMetrics metrics);
On 2015/06/09 00:48:15, viettrungluu wrote:
> Did you consider letting this parameter be nullable (with null meaning the
> default)?

Done.

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
services/native_viewport/native_viewport_impl.cc:50:
requested_configuration.To<gfx::SurfaceConfiguration>());
On 2015/06/09 00:48:15, viettrungluu wrote:
> Is there any validation of requested_configuration that you should do here?

I wonder if the validation should be done in a platform-specific manner in each
of the gl_surface_*.cc's?  I imagine there will be some configurations that are
invalid on some platforms but not on others.  For example, I think that 565
surfaces may not be available on desktop configs.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:199: };
On 2015/06/09 00:48:15, viettrungluu wrote:
> no semicolon

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:213: SurfaceConfiguration surface_configuration_;
On 2015/06/09 00:48:15, viettrungluu wrote:
> nit: I believe members should be declared after friends.

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:25: GLSurfaceEGL(SurfaceConfiguration
requested_configuration);
On 2015/06/09 00:48:15, viettrungluu wrote:
> nit: explicit

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:97: explicit PbufferGLSurfaceEGL(const gfx::Size& size,
On 2015/06/09 00:48:15, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:127: explicit SurfacelessEGL(const gfx::Size& size,
On 2015/06/09 00:48:15, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_glx.h
File ui/gl/gl_surface_glx.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_glx.h#...
ui/gl/gl_surface_glx.h:24: GLSurfaceGLX(SurfaceConfiguration
requested_configuration);
On 2015/06/09 00:48:15, viettrungluu wrote:
> explicit

Done.

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_stub.h
File ui/gl/gl_surface_stub.h (right):

https://codereview.chromium.org/1168993002/diff/20001/ui/gl/gl_surface_stub.h...
ui/gl/gl_surface_stub.h:15: GLSurfaceStub(SurfaceConfiguration
requested_configuration);
On 2015/06/09 00:48:15, viettrungluu wrote:
> explicit

Done.

[qsr, 2015-06-09 11:02:51]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-06-09 11:02:52]

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:347: const
gfx::SurfaceConfiguration requested_configuration) {
Same here.

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:88: const
gfx::SurfaceConfiguration requested_configuration);
You can pass this by reference:
const gfx::SurfaceConfiguration& requested_configuration

https://codereview.chromium.org/1168993002/diff/40001/services/gles2/command_...
File services/gles2/command_buffer_driver.h (right):

https://codereview.chromium.org/1168993002/diff/40001/services/gles2/command_...
services/gles2/command_buffer_driver.h:50: const gfx::SurfaceConfiguration
requested_configuration);
Here too.

https://codereview.chromium.org/1168993002/diff/40001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.h (right):

https://codereview.chromium.org/1168993002/diff/40001/services/native_viewpor...
services/native_viewport/native_viewport_impl.h:43: void Create(const
mojo::SizePtr size,
Why this const? And the underlying ones? You are taking ownership of these
objects, so marking them const is kind of weird.

[iansf, 2015-06-09 17:23:25]

Thanks for the comments!

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:347: const
gfx::SurfaceConfiguration requested_configuration) {
On 2015/06/09 11:02:52, qsr wrote:
> Same here.

Done.

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1168993002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.h:88: const
gfx::SurfaceConfiguration requested_configuration);
On 2015/06/09 11:02:52, qsr wrote:
> You can pass this by reference:
> const gfx::SurfaceConfiguration& requested_configuration

Done.

https://codereview.chromium.org/1168993002/diff/40001/services/gles2/command_...
File services/gles2/command_buffer_driver.h (right):

https://codereview.chromium.org/1168993002/diff/40001/services/gles2/command_...
services/gles2/command_buffer_driver.h:50: const gfx::SurfaceConfiguration
requested_configuration);
On 2015/06/09 11:02:52, qsr wrote:
> Here too.

Done.

https://codereview.chromium.org/1168993002/diff/40001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.h (right):

https://codereview.chromium.org/1168993002/diff/40001/services/native_viewpor...
services/native_viewport/native_viewport_impl.h:43: void Create(const
mojo::SizePtr size,
On 2015/06/09 11:02:52, qsr wrote:
> Why this const? And the underlying ones? You are taking ownership of these
> objects, so marking them const is kind of weird.

Done.

[iansf, 2015-06-09 17:53:49]

iansf@google.com changed reviewers:
+ cstout@google.com

[viettrungluu, 2015-06-09 22:18:08]

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
services/native_viewport/native_viewport_impl.cc:50:
requested_configuration.To<gfx::SurfaceConfiguration>());
On 2015/06/09 01:52:05, iansf wrote:
> On 2015/06/09 00:48:15, viettrungluu wrote:
> > Is there any validation of requested_configuration that you should do here?
> 
> I wonder if the validation should be done in a platform-specific manner in
each
> of the gl_surface_*.cc's?  I imagine there will be some configurations that
are
> invalid on some platforms but not on others.  For example, I think that 565
> surfaces may not be available on desktop configs.

The other possibility (or maybe even requirement) is to make sure that there's a
clean (with respect to the native viewport service) failure path if/when things
fail later on.

[iansf, 2015-06-09 22:29:06]

Builds are green -- should I land this?

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
services/native_viewport/native_viewport_impl.cc:50:
requested_configuration.To<gfx::SurfaceConfiguration>());
On 2015/06/09 22:18:07, viettrungluu wrote:
> On 2015/06/09 01:52:05, iansf wrote:
> > On 2015/06/09 00:48:15, viettrungluu wrote:
> > > Is there any validation of requested_configuration that you should do
here?
> > 
> > I wonder if the validation should be done in a platform-specific manner in
> each
> > of the gl_surface_*.cc's?  I imagine there will be some configurations that
> are
> > invalid on some platforms but not on others.  For example, I think that 565
> > surfaces may not be available on desktop configs.
> 
> The other possibility (or maybe even requirement) is to make sure that there's
a
> clean (with respect to the native viewport service) failure path if/when
things
> fail later on.

Do you have a suggestion for how that would look?  Currently in debug builds it
will log and crash if eglChooseConfig fails.  In release builds it will just log
(and presumably display a black screen).  These are both the same behavior as
before, although they happen slightly later in app startup.  I felt like
changing the error reporting mechanism was out-of-scope for this set of fixes,
but I'm happy to discuss what such a mechanism would look like.

[viettrungluu, 2015-06-09 22:38:35]

(A couple more nits, but LGTM otherwise.)

https://codereview.chromium.org/1168993002/diff/100001/apps/moterm/gl_helper_...
File apps/moterm/gl_helper_test_app.cc (right):

https://codereview.chromium.org/1168993002/diff/100001/apps/moterm/gl_helper_...
apps/moterm/gl_helper_test_app.cc:221: viewport_size_.Clone(),
mojo::SurfaceConfiguration::New(),
It's up to you, but you could also just pass nullptr here (and elsewhere), I
think.

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
services/native_viewport/native_viewport_impl.cc:45: 
nit: no blank line here

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
services/native_viewport/native_viewport_impl.cc:46: if (requested_configuration
== nullptr) {
nit: for consistency with the rest of the file, omit braces for one-line blocks

[viettrungluu, 2015-06-09 22:44:39]

On 2015/06/09 22:29:06, iansf wrote:
> Builds are green -- should I land this?
> 
>
https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
> File services/native_viewport/native_viewport_impl.cc (right):
> 
>
https://codereview.chromium.org/1168993002/diff/20001/services/native_viewpor...
> services/native_viewport/native_viewport_impl.cc:50:
> requested_configuration.To<gfx::SurfaceConfiguration>());
> On 2015/06/09 22:18:07, viettrungluu wrote:
> > On 2015/06/09 01:52:05, iansf wrote:
> > > On 2015/06/09 00:48:15, viettrungluu wrote:
> > > > Is there any validation of requested_configuration that you should do
> here?
> > > 
> > > I wonder if the validation should be done in a platform-specific manner in
> > each
> > > of the gl_surface_*.cc's?  I imagine there will be some configurations
that
> > are
> > > invalid on some platforms but not on others.  For example, I think that
565
> > > surfaces may not be available on desktop configs.
> > 
> > The other possibility (or maybe even requirement) is to make sure that
there's
> a
> > clean (with respect to the native viewport service) failure path if/when
> things
> > fail later on.
> 
> Do you have a suggestion for how that would look?  Currently in debug builds
it
> will log and crash if eglChooseConfig fails.  In release builds it will just
log
> (and presumably display a black screen).  These are both the same behavior as
> before, although they happen slightly later in app startup.  I felt like
> changing the error reporting mechanism was out-of-scope for this set of fixes,
> but I'm happy to discuss what such a mechanism would look like.

Agreed with it being out of scope for this change.

One way may be to report it in the response (in the viewport metrics). Arguably,
something better would be to refactor the way the native viewport service's
interfaces look. E.g., you might imagine something like:

interface NativeViewportService {  // Probably should have a better name.
  // |native_viewport| is closed on failure.
  Create(Size size, SurfaceConfiguration? requested_configuration,
NativeViewport& native_viewport) => (ViewportMetrics metrics);

  // Maybe other methods. (E.g., to query possible configurations, etc. Also,
what if there are multiple screens?)
};

interface NativeViewport {
  Show();
  Hide();
  ...
};

[iansf, 2015-06-09 23:45:26]

Thanks!  I'm landing as soon as these changes return green.

https://codereview.chromium.org/1168993002/diff/100001/apps/moterm/gl_helper_...
File apps/moterm/gl_helper_test_app.cc (right):

https://codereview.chromium.org/1168993002/diff/100001/apps/moterm/gl_helper_...
apps/moterm/gl_helper_test_app.cc:221: viewport_size_.Clone(),
mojo::SurfaceConfiguration::New(),
On 2015/06/09 22:38:35, viettrungluu wrote:
> It's up to you, but you could also just pass nullptr here (and elsewhere), I
> think.

That's true!  For example apps like this, do you think it's better to have a
potentially mysterious nullptr, or a more verbose explicit default?  I'm fine
either way.

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
services/native_viewport/native_viewport_impl.cc:45: 
On 2015/06/09 22:38:35, viettrungluu wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/1168993002/diff/100001/services/native_viewpo...
services/native_viewport/native_viewport_impl.cc:46: if (requested_configuration
== nullptr) {
On 2015/06/09 22:38:35, viettrungluu wrote:
> nit: for consistency with the rest of the file, omit braces for one-line
blocks

Done.

[iansf, 2015-06-09 23:50:38]

Committed patchset #7 (id:120001) manually as
f66f4370dd9eef42cec3ec11cd654934ea38c3d4 (presubmit successful).