Stubbed in new test: graphics_WindowManagerGraphicsCapture

Stubbed in new test: graphics_WindowManagerGraphicsCapture

This is the beginning of a functional test described by issue 2054 "Test graphics stack in window manager":
http://code.google.com/p/chromium-os/issues/detail?id=2054

glbench has been modified to create a new target, "windowmanagertest". This application is used in a new site_test that logs in (if needed), displays the full screen app, and then logs out.

Following changes will perform window manager screen captures and compare against known good images.

[piman, 2010-04-06 01:23:20]

Overall looks good, but there's several style issues.

http://codereview.chromium.org/1523011/diff/1/2
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/1/2#newcode9
client/deps/glbench/src/Makefile:9: SOURCES_ALL = $(SOURCES_COMMON)
$(SOURCES_GL_BENCH) $(SOURCES_TEARTEST)  $(SOURCES_WINDOWMANAGERTEST)
80 chars, you can use \ or += to split.

http://codereview.chromium.org/1523011/diff/1/2#newcode46
client/deps/glbench/src/Makefile:46: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) $(OBJS_ALL) $(DEPS_ALL)
80 chars (use \)

http://codereview.chromium.org/1523011/diff/1/3
File client/deps/glbench/src/windowmanagertest.cc (right):

http://codereview.chromium.org/1523011/diff/1/3#newcode31
client/deps/glbench/src/windowmanagertest.cc:31: char *CreateBitmap(int w, int
h) {
Stars on the Left (SotL)

http://codereview.chromium.org/1523011/diff/1/3#newcode32
client/deps/glbench/src/windowmanagertest.cc:32: char *bitmap = new char[w * h];
SotL

http://codereview.chromium.org/1523011/diff/1/3#newcode33
client/deps/glbench/src/windowmanagertest.cc:33: unsigned char *p = (unsigned
char*) bitmap;
SotL

http://codereview.chromium.org/1523011/diff/1/3#newcode33
client/deps/glbench/src/windowmanagertest.cc:33: unsigned char *p = (unsigned
char*) bitmap;
use reinterpret_cast instead of C-style cast.

http://codereview.chromium.org/1523011/diff/1/3#newcode42
client/deps/glbench/src/windowmanagertest.cc:42: if (dist2 > 1) dist2 = 1;
1->1.f

http://codereview.chromium.org/1523011/diff/1/3#newcode43
client/deps/glbench/src/windowmanagertest.cc:43: *p = (1-dist2) * 255;
1->1.f
255->255.f

http://codereview.chromium.org/1523011/diff/1/3#newcode51
client/deps/glbench/src/windowmanagertest.cc:51: const char *vertex_shader =
string constant should be declared like (note naming, and use of [] instead of
*):
const char kVertexShader[] =

http://codereview.chromium.org/1523011/diff/1/3#newcode67
client/deps/glbench/src/windowmanagertest.cc:67: int seconds_to_run = 10;
Naming for a global should be g_seconds_to_run
Also, mark it static.

http://codereview.chromium.org/1523011/diff/1/3#newcode93
client/deps/glbench/src/windowmanagertest.cc:93: char *bitmap =
CreateBitmap(g_height, g_width);
SotL

[Alexey Marinichev, 2010-04-06 01:27:02]

http://codereview.chromium.org/1523011/diff/1/3
File client/deps/glbench/src/windowmanagertest.cc (right):

http://codereview.chromium.org/1523011/diff/1/3#newcode16
client/deps/glbench/src/windowmanagertest.cc:16: #include "xlib_window.h"
xlib_window.h is not really needed, is it?

http://codereview.chromium.org/1523011/diff/1/3#newcode43
client/deps/glbench/src/windowmanagertest.cc:43: *p = (1-dist2) * 255;
Cast it well.

http://codereview.chromium.org/1523011/diff/1/3#newcode68
client/deps/glbench/src/windowmanagertest.cc:68: static void ParseArgs(int argc,
char* argv[]) {
gflags would work nicely here.  See teartest.c.

[scheib, 2010-04-06 22:16:22]

addressed comments, converted to using gflags.

http://codereview.chromium.org/1523011/diff/1/2
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/1/2#newcode9
client/deps/glbench/src/Makefile:9: SOURCES_ALL = $(SOURCES_COMMON)
$(SOURCES_GL_BENCH) $(SOURCES_TEARTEST)  $(SOURCES_WINDOWMANAGERTEST)
done

http://codereview.chromium.org/1523011/diff/1/2#newcode46
client/deps/glbench/src/Makefile:46: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) $(OBJS_ALL) $(DEPS_ALL)
is under 80

http://codereview.chromium.org/1523011/diff/1/3
File client/deps/glbench/src/windowmanagertest.cc (right):

http://codereview.chromium.org/1523011/diff/1/3#newcode16
client/deps/glbench/src/windowmanagertest.cc:16: #include "xlib_window.h"
cut

http://codereview.chromium.org/1523011/diff/1/3#newcode31
client/deps/glbench/src/windowmanagertest.cc:31: char *CreateBitmap(int w, int
h) {
done throughout

http://codereview.chromium.org/1523011/diff/1/3#newcode33
client/deps/glbench/src/windowmanagertest.cc:33: unsigned char *p = (unsigned
char*) bitmap;
done

http://codereview.chromium.org/1523011/diff/1/3#newcode42
client/deps/glbench/src/windowmanagertest.cc:42: if (dist2 > 1) dist2 = 1;
done

http://codereview.chromium.org/1523011/diff/1/3#newcode43
client/deps/glbench/src/windowmanagertest.cc:43: *p = (1-dist2) * 255;
done: literals explicitly float

http://codereview.chromium.org/1523011/diff/1/3#newcode51
client/deps/glbench/src/windowmanagertest.cc:51: const char *vertex_shader =
done

http://codereview.chromium.org/1523011/diff/1/3#newcode68
client/deps/glbench/src/windowmanagertest.cc:68: static void ParseArgs(int argc,
char* argv[]) {
Converted to gflags for consistency with rest of glbench

[piman, 2010-04-06 22:59:59]

http://codereview.chromium.org/1523011/diff/1/2
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/1/2#newcode46
client/deps/glbench/src/Makefile:46: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) $(OBJS_ALL) $(DEPS_ALL)
On 2010/04/06 22:16:23, scheib wrote:
> is under 80
not with 8-chars tabs :) see the diff in rietveld

http://codereview.chromium.org/1523011/diff/7001/2003
File client/deps/glbench/src/windowmanagertest.cc (right):

http://codereview.chromium.org/1523011/diff/7001/2003#newcode43
client/deps/glbench/src/windowmanagertest.cc:43: if (dist2 > 1) dist2 = 1.f;
you'll need to fix the 1 in the comparison too.
by the way, you need the assignment to be on a separate line

http://codereview.chromium.org/1523011/diff/7001/2005
File
client/site_tests/graphics_WindowManagerGraphicsCapture/graphics_WindowManagerGraphicsCapture.py
(right):

http://codereview.chromium.org/1523011/diff/7001/2005#newcode43
client/site_tests/graphics_WindowManagerGraphicsCapture/graphics_WindowManagerGraphicsCapture.py:43:
options = "-seconds_to_run 10"
shouldn't this be --seconds_to_run ?

[scheib, 2010-04-06 23:40:17]

http://codereview.chromium.org/1523011/diff/1/2
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/1/2#newcode46
client/deps/glbench/src/Makefile:46: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) $(OBJS_ALL) $(DEPS_ALL)
On 2010/04/06 22:59:59, piman wrote:
> On 2010/04/06 22:16:23, scheib wrote:
> > is under 80
> not with 8-chars tabs :) see the diff in rietveld
> 

Done. This is clearly a bug in Rietveld, tabs in Makefiles should be 3.5 spaces.

http://codereview.chromium.org/1523011/diff/7001/2003
File client/deps/glbench/src/windowmanagertest.cc (right):

http://codereview.chromium.org/1523011/diff/7001/2003#newcode43
client/deps/glbench/src/windowmanagertest.cc:43: if (dist2 > 1) dist2 = 1.f;
On 2010/04/06 22:59:59, piman wrote:
> you'll need to fix the 1 in the comparison too.
> by the way, you need the assignment to be on a separate line

Done.

http://codereview.chromium.org/1523011/diff/7001/2005
File
client/site_tests/graphics_WindowManagerGraphicsCapture/graphics_WindowManagerGraphicsCapture.py
(right):

http://codereview.chromium.org/1523011/diff/7001/2005#newcode43
client/site_tests/graphics_WindowManagerGraphicsCapture/graphics_WindowManagerGraphicsCapture.py:43:
options = "-seconds_to_run 10"
On 2010/04/06 22:59:59, piman wrote:
> shouldn't this be --seconds_to_run ?

Done. Single dash works, but --is_nice.

[Alexey Marinichev, 2010-04-07 00:52:18]

LGTM with a bonus matter of taste comment.

http://codereview.chromium.org/1523011/diff/13002/14001
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/13002/14001#newcode49
client/deps/glbench/src/Makefile:49: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) \
If you use two $(RM)s, you won't need ugly backslash.

[piman, 2010-04-07 00:55:32]

LGTM

[scheib, 2010-04-07 16:55:32]

On 2010/04/07 00:55:32, piman wrote:
> LGTM

Hurray, now I need a volunteer to commit this for me.

[scheib, 2010-04-07 16:55:42]

http://codereview.chromium.org/1523011/diff/13002/14001
File client/deps/glbench/src/Makefile (right):

http://codereview.chromium.org/1523011/diff/13002/14001#newcode49
client/deps/glbench/src/Makefile:49: $(RM) $(GL_BENCH) $(TEARTEST)
$(WINDOWMANAGERTEST) \
On 2010/04/07 00:52:18, Alexey Marinichev wrote:
> If you use two $(RM)s, you won't need ugly backslash.

Done.