Refactor and improve gpu_info_collector: collect information on linux;...

Refactor and improve gpu_info_collector: collect information on linux;
collect extra information (driver_vendor, gl_renderer, etc.) on Mac/linux.
Note that this CL takes some code from rlp's long outstandingCL (with her
permission).

BUG=49579
TEST=unittest


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

[Zhenyao Mo, 2011-01-19 18:50:34]

Please review.

[Ken Russell (switch to Gerrit), 2011-01-19 22:11:43]

LGTM. A few nits. Also you have a build failure on the try bots.

http://codereview.chromium.org/6346007/diff/42001/chrome/browser/dom_ui/gpu_i...
File chrome/browser/dom_ui/gpu_internals_ui.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/browser/dom_ui/gpu_i...
chrome/browser/dom_ui/gpu_internals_ui.cc:284: gpu_info.driver_version()));
nit: align like the other expressions.

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h
File chrome/common/gpu_info.h (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h#ne...
chrome/common/gpu_info.h:50: // This will typically be something
Could you complete this previously incomplete comment?

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h#ne...
chrome/common/gpu_info.h:68: uint32 gl_version() const;
How can the user of this class tell whether they're running on top of desktop
OpenGL or OpenGL ES (say 2.0 or some later version)?

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector.cc:41: void FinalizeGLContext(gfx::GLContext*&
context) {
Mutable references aren't allowed in the Google C++ style guide.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_linux.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_linux.cc:133: gfx::GetGLImplementation() ==
gfx::kGLImplementationEGLGLES2);
What if we're running on desktop GL and the GL_ARB_robustness extension is
available? We don't have that code path enabled right now during our context
creation, but will probably need to in the near future.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_win.cc:36: IDirect3DDevice9* device =
display->getDevice();
We should add a TODO indicating that this code will only work on top of ANGLE on
Windows. As has been discussed recently on chromium-dev, some graphics card
vendors are starting to provide native EGL and OpenGL ES 2.0 drivers on Windows,
in which case this code will need to be restructured.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_win.cc:136: size_t pos =
gl_version_string.find_last_not_of("0123456789.");
Could you add a comment indicating the expected format of the GL version string
on this platform? This logic looks different than that on the other platforms.

[Zhenyao Mo, 2011-01-19 23:36:22]

The windows bot failure is due to gcl failed to pick up one particular file I
modified.  I still don't understand what happened, but per Vangelis's
suggestion, after I delete the CL and create a new one, that file is included. 
I'll only commit once Win trybot returns green.

http://codereview.chromium.org/6346007/diff/42001/chrome/browser/dom_ui/gpu_i...
File chrome/browser/dom_ui/gpu_internals_ui.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/browser/dom_ui/gpu_i...
chrome/browser/dom_ui/gpu_internals_ui.cc:284: gpu_info.driver_version()));
On 2011/01/19 22:11:43, kbr wrote:
> nit: align like the other expressions.

Done.

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h
File chrome/common/gpu_info.h (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h#ne...
chrome/common/gpu_info.h:50: // This will typically be something
On 2011/01/19 22:11:43, kbr wrote:
> Could you complete this previously incomplete comment?

Done.

http://codereview.chromium.org/6346007/diff/42001/chrome/common/gpu_info.h#ne...
chrome/common/gpu_info.h:68: uint32 gl_version() const;
On 2011/01/19 22:11:43, kbr wrote:
> How can the user of this class tell whether they're running on top of desktop
> OpenGL or OpenGL ES (say 2.0 or some later version)?

Can't at the moment.  I'll add a FIXME here.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector.cc:41: void FinalizeGLContext(gfx::GLContext*&
context) {
Changed to gfx::GLContext**.

On 2011/01/19 22:11:43, kbr wrote:
> Mutable references aren't allowed in the Google C++ style guide.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_linux.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_linux.cc:133: gfx::GetGLImplementation() ==
gfx::kGLImplementationEGLGLES2);
Added a FIXME here for this.

On 2011/01/19 22:11:43, kbr wrote:
> What if we're running on desktop GL and the GL_ARB_robustness extension is
> available? We don't have that code path enabled right now during our context
> creation, but will probably need to in the near future.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_win.cc:36: IDirect3DDevice9* device =
display->getDevice();
In the beginning of this function we already handled the non-ANGLE situation.

On 2011/01/19 22:11:43, kbr wrote:
> We should add a TODO indicating that this code will only work on top of ANGLE
on
> Windows. As has been discussed recently on chromium-dev, some graphics card
> vendors are starting to provide native EGL and OpenGL ES 2.0 drivers on
Windows,
> in which case this code will need to be restructured.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_win.cc:136: size_t pos =
gl_version_string.find_last_not_of("0123456789.");
On 2011/01/19 22:11:43, kbr wrote:
> Could you add a comment indicating the expected format of the GL version
string
> on this platform? This logic looks different than that on the other platforms.

Done.

[Ken Russell (switch to Gerrit), 2011-01-19 23:46:50]

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_win.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_win.cc:36: IDirect3DDevice9* device =
display->getDevice();
Running on EGL+GLES2 on Windows doesn't necessarily mean we are running on
ANGLE. We might be running with real EGL/GLES2 drivers.

On 2011/01/19 23:36:22, Zhenyao Mo wrote:
> In the beginning of this function we already handled the non-ANGLE situation.
> 
> On 2011/01/19 22:11:43, kbr wrote:
> > We should add a TODO indicating that this code will only work on top of
ANGLE
> on
> > Windows. As has been discussed recently on chromium-dev, some graphics card
> > vendors are starting to provide native EGL and OpenGL ES 2.0 drivers on
> Windows,
> > in which case this code will need to be restructured.
>

[Zhenyao Mo, 2011-01-20 00:12:19]

On 2011/01/19 23:46:50, kbr wrote:
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> File chrome/gpu/gpu_info_collector_win.cc (right):
> 
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> chrome/gpu/gpu_info_collector_win.cc:36: IDirect3DDevice9* device =
> display->getDevice();
> Running on EGL+GLES2 on Windows doesn't necessarily mean we are running on
> ANGLE. We might be running with real EGL/GLES2 drivers.

I see.  TODO added.

> 
> On 2011/01/19 23:36:22, Zhenyao Mo wrote:
> > In the beginning of this function we already handled the non-ANGLE
situation.
> > 
> > On 2011/01/19 22:11:43, kbr wrote:
> > > We should add a TODO indicating that this code will only work on top of
> ANGLE
> > on
> > > Windows. As has been discussed recently on chromium-dev, some graphics
card
> > > vendors are starting to provide native EGL and OpenGL ES 2.0 drivers on
> > Windows,
> > > in which case this code will need to be restructured.
> >

[rpetterson, 2011-01-20 00:48:03]

Overall, most the gpu_info and collector classes look okay.

I'd really like to see some tests for the new GL functionality. At the very
least some tests for pulling out driver versions since those vary by platform.

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
File chrome/gpu/gpu_info_collector_linux.cc (right):

http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
chrome/gpu/gpu_info_collector_linux.cc:155: if (device->device_class == 0x0300)
{  // Device class is DISPLAY_VGA.
0x0300 was the most common in my research, but there may be other classes which
qualify as display devices. This will probably provide a significant amount of
info now though.

[Zhenyao Mo, 2011-01-20 01:02:52]

On 2011/01/20 00:48:03, rpetterson wrote:
> Overall, most the gpu_info and collector classes look okay.
> 
> I'd really like to see some tests for the new GL functionality. At the very
> least some tests for pulling out driver versions since those vary by platform.

I agree more testing would be great.  I didn't incorporate the
gpu_info_collector test from your CL because I feel the test depending on the
bot environment is quite dangerous.  Yet I couldn't think of a better way to
test it.  I've tested manually on my two machines though.

> 
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> File chrome/gpu/gpu_info_collector_linux.cc (right):
> 
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> chrome/gpu/gpu_info_collector_linux.cc:155: if (device->device_class ==
0x0300)
> {  // Device class is DISPLAY_VGA.
> 0x0300 was the most common in my research, but there may be other classes
which
> qualify as display devices. This will probably provide a significant amount of
> info now though.

I see.  Added a TODO here.

[Zhenyao Mo, 2011-01-20 17:02:26]

Ah, forget about my comments on the test being dangerous.  I see that you mock
the gl interface.

Then I'll "steal" another piece from you CL.

On 2011/01/20 01:02:52, Zhenyao Mo wrote:
> On 2011/01/20 00:48:03, rpetterson wrote:
> > Overall, most the gpu_info and collector classes look okay.
> > 
> > I'd really like to see some tests for the new GL functionality. At the very
> > least some tests for pulling out driver versions since those vary by
platform.
> 
> I agree more testing would be great.  I didn't incorporate the
> gpu_info_collector test from your CL because I feel the test depending on the
> bot environment is quite dangerous.  Yet I couldn't think of a better way to
> test it.  I've tested manually on my two machines though.
> 
> > 
> >
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> > File chrome/gpu/gpu_info_collector_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/6346007/diff/42001/chrome/gpu/gpu_info_collect...
> > chrome/gpu/gpu_info_collector_linux.cc:155: if (device->device_class ==
> 0x0300)
> > {  // Device class is DISPLAY_VGA.
> > 0x0300 was the most common in my research, but there may be other classes
> which
> > qualify as display devices. This will probably provide a significant amount
of
> > info now though.
> 
> I see.  Added a TODO here.

[rpetterson, 2011-01-20 23:58:25]

LGTM

Thanks for getting those tests in there.