Allow GM to run multiple GPU configs in one run. Also add MSAA config.

[bsalomon, 2012-04-19 14:13:02]

Right now GM can only run one type of GPU configuration at a time. This change

1) Runs all GPU configs in one run of GM and gives them separate file names.

2) Adds configs for testing multisampling rather than custom AA.

If the bots are passing any of the flags I'm removing they'd have to stop doing
so before this gets checked in.  Also, because I'm creating an RT for each test
at its requested size some GPU images will need to be rebaselined due to slight
rasterization changes.

[reed1, 2012-04-19 14:15:35]

lgtm

[epoger, 2012-04-19 14:43:41]

let's please discuss in person for a couple minutes...

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode634
gm/gmmain.cpp:634: { SkBitmap::kARGB_8888_Config, kRaster_Backend,
GrContextFactory::kNative_GLContextType, 0, kNone_ConfigFlag, "8888" },
It would be nice if these lines could be wrapped at 80 chars, or at most 100.

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode641
gm/gmmain.cpp:641: #ifdef SK_ANGLE
please un-indent all preprocessor directives; right now they get lost in the
text

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode650
gm/gmmain.cpp:650: { SkBitmap::kARGB_8888_Config, kPDF_Backend,
GrContextFactory::kNative_GLContextType,    0, CAN_IMAGE_PDF ? kNone_ConfigFlag
: kNoRead_ConfigFlag, "pdf" },
I'm going to ask you to walk me through the ConfigFlag stuff. I find it a bit
confusing.

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode650
gm/gmmain.cpp:650: { SkBitmap::kARGB_8888_Config, kPDF_Backend,
GrContextFactory::kNative_GLContextType,    0, CAN_IMAGE_PDF ? kNone_ConfigFlag
: kNoRead_ConfigFlag, "pdf" },
I'm going to ask you to walk me through the ConfigFlag stuff in person; I find
it a bit confusing.

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode682
gm/gmmain.cpp:682: class AutoResetGr : SkNoncopyable {
please add a short comment indicating what this class does

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory.h
File include/gpu/GrContextFactory.h (right):

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:13: #include "gl/SkMesaGLContext.h"
some indentation would make this stuff easier to read

[robertphillips, 2012-04-19 14:57:57]

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode228
gm/gmmain.cpp:228: kNone_ConfigFlag    = 0x0,
Reversing the sense of these to be positive might make this easier to follow

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory.h
File include/gpu/GrContextFactory.h (right):

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:2: * Copyright 2011 Google Inc.
date?

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:66: break;
Why not just return here?

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:96: } else if (!glCtx.get()->init(kBogusSize,
kBogusSize)) {
I would either fuse these into one test or remove the else

[bsalomon, 2012-04-19 15:30:01]

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode228
gm/gmmain.cpp:228: kNone_ConfigFlag    = 0x0,
On 2012/04/19 14:57:57, robertphillips wrote:
> Reversing the sense of these to be positive might make this easier to follow

Done.

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode634
gm/gmmain.cpp:634: { SkBitmap::kARGB_8888_Config, kRaster_Backend,
GrContextFactory::kNative_GLContextType, 0, kNone_ConfigFlag, "8888" },
On 2012/04/19 14:43:41, epoger wrote:
> It would be nice if these lines could be wrapped at 80 chars, or at most 100.

In an editor it is nice to be able to see these as columns in a table.

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode641
gm/gmmain.cpp:641: #ifdef SK_ANGLE
On 2012/04/19 14:43:41, epoger wrote:
> please un-indent all preprocessor directives; right now they get lost in the
> text

partial indent of nested #ifdef/#endifs

http://codereview.appspot.com/6061051/diff/4001/gm/gmmain.cpp#newcode682
gm/gmmain.cpp:682: class AutoResetGr : SkNoncopyable {
On 2012/04/19 14:43:41, epoger wrote:
> please add a short comment indicating what this class does

Done.

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory.h
File include/gpu/GrContextFactory.h (right):

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:2: * Copyright 2011 Google Inc.
On 2012/04/19 14:57:57, robertphillips wrote:
> date?

Done.

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:13: #include "gl/SkMesaGLContext.h"
On 2012/04/19 14:43:41, epoger wrote:
> some indentation would make this stuff easier to read

Done and alphabetized.

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:66: break;
On 2012/04/19 14:57:57, robertphillips wrote:
> Why not just return here?

Done. (an earlier version did more work below)

http://codereview.appspot.com/6061051/diff/4001/include/gpu/GrContextFactory....
include/gpu/GrContextFactory.h:96: } else if (!glCtx.get()->init(kBogusSize,
kBogusSize)) {
On 2012/04/19 14:57:57, robertphillips wrote:
> I would either fuse these into one test or remove the else

Done.

[epoger, 2012-04-19 15:58:02]

LGTM

I have already removed the --debuggl gm run from the bots.

[robertphillips, 2012-04-19 18:18:06]

LGTM

[bsalomon, 2012-04-20 14:25:02]

Closed with r3734