Refactor gmmain to separate out config parsing

Refactor gmmain to separate out config parsing

Committed: http://code.google.com/p/skia/source/detail?r=10703

[ducky, 2013-08-02 03:59:04]

This refactors tool_main in gmmain.cpp to separate out the config parsing into
helper functions. This is in preparation for adding support for multiple PDF
renderer backends to the GM as I don't want to stack yet more stuff onto a
already lengthy function. 

The changes are mostly cosmetic (as in, if I made any serious behavior altering
changes, that's a mistake) with these notes:
- All parsing regarding --config has been moved together, so the execution order
isn't exactly the same. The GPU context factory creation happens at the
beginning, and the GPU context check happens immediately after the configs are
parsed. I don't think this affects anything.
- For --config parsing, it adds the defaults if there are no configs in the list
(like when you specify a list of only negative configs), instead of when you
specify an empty configs list. Furthermore, it seems that this functionality is
duplicated with --excludeConfigs - it sounds cleaner to only keep one of the two
(personally, I like the negative --config), but I'm reluctant to do that right
now since it will probably break something somewhere...
- There may be some more reordering of config parsing operations, though I don't
think it matters.
- The final moduloRemainder/moduloDivisor check was eliminated as it is
redundant. Checking only happens on user input, it is assumed the default
initialized values do not need to be sanity checked.

Finally, there may be other parts that can be separated out into helper
functions, but:
- I'm not yet sure how best to address the subdirectory creation
- I don't know what the JPEG quality check failure does. Is it just a warning,
or did someone forget a "return -1"?

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode2062
gm/gmmain.cpp:2062: if (FLAGS_pdfJpegQuality < -1 || FLAGS_pdfJpegQuality > 100)
{
Is this supposed to do anything, like exit the program? Or is it just a warning?

[edisonn, 2013-08-02 12:32:41]

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1770
gm/gmmain.cpp:1770: bool parse_flags_configs(SkTDArray<size_t>* outConfigs,
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1866
gm/gmmain.cpp:1866: (i == outConfigs->count() - 1) ? "\n" : " ");
make it (i < outConfigs->count()) ? " " : "");

and add an append of \n after the for. it is more clear that way.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1873
gm/gmmain.cpp:1873: bool parse_flags_ignore_error_types(ErrorCombination*
outErrorTypes) {
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1890
gm/gmmain.cpp:1890: bool parse_flags_modulo(int* moduloRemainder, int*
moduloDivisor) {
make it static, otherwise you might get a warning as error on a few bots!

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1903
gm/gmmain.cpp:1903: bool parse_flags_gpu_cache(int* sizeBytes, int* sizeCount) {
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1918
gm/gmmain.cpp:1918: bool parse_tile_grid_replay_scales(SkTDArray<SkScalar>*
outScales) {
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1937
gm/gmmain.cpp:1937: bool parse_read_path(SkAutoTUnref<ExpectationsSource>*
source) {
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1960
gm/gmmain.cpp:1960: bool parse_match_strs(SkTDArray<const char*>* matchStrs) {
static

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode2062
gm/gmmain.cpp:2062: if (FLAGS_pdfJpegQuality < -1 || FLAGS_pdfJpegQuality > 100)
{
On 2013/08/02 03:59:04, ducky wrote:
> Is this supposed to do anything, like exit the program? Or is it just a
warning?

please add return -1 here. good find.

[ducky, 2013-08-02 18:14:38]

Review comments fixed.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1770
gm/gmmain.cpp:1770: bool parse_flags_configs(SkTDArray<size_t>* outConfigs,
On 2013/08/02 12:32:41, edisonn wrote:
> static

Fixed as of latest patchset last night, after I realized the Mac build broke. I
guess the review request doesn't automatically track updated patchsets, so I'll
re-send review requests as this stuff gets updated.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1866
gm/gmmain.cpp:1866: (i == outConfigs->count() - 1) ? "\n" : " ");
On 2013/08/02 12:32:41, edisonn wrote:
> make it (i < outConfigs->count()) ? " " : "");
> 
> and add an append of \n after the for. it is more clear that way.

Done. It now just adds a space all the time, since trailing spaces are invisible
anyways.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1873
gm/gmmain.cpp:1873: bool parse_flags_ignore_error_types(ErrorCombination*
outErrorTypes) {
On 2013/08/02 12:32:41, edisonn wrote:
> static

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1890
gm/gmmain.cpp:1890: bool parse_flags_modulo(int* moduloRemainder, int*
moduloDivisor) {
On 2013/08/02 12:32:41, edisonn wrote:
> make it static, otherwise you might get a warning as error on a few bots!

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1903
gm/gmmain.cpp:1903: bool parse_flags_gpu_cache(int* sizeBytes, int* sizeCount) {
On 2013/08/02 12:32:41, edisonn wrote:
> static

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1918
gm/gmmain.cpp:1918: bool parse_tile_grid_replay_scales(SkTDArray<SkScalar>*
outScales) {
On 2013/08/02 12:32:41, edisonn wrote:
> static

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1937
gm/gmmain.cpp:1937: bool parse_read_path(SkAutoTUnref<ExpectationsSource>*
source) {
On 2013/08/02 12:32:41, edisonn wrote:
> static

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode1960
gm/gmmain.cpp:1960: bool parse_match_strs(SkTDArray<const char*>* matchStrs) {
On 2013/08/02 12:32:41, edisonn wrote:
> static

Done.

https://codereview.chromium.org/21715002/diff/2001/gm/gmmain.cpp#newcode2062
gm/gmmain.cpp:2062: if (FLAGS_pdfJpegQuality < -1 || FLAGS_pdfJpegQuality > 100)
{
On 2013/08/02 12:32:41, edisonn wrote:
> On 2013/08/02 03:59:04, ducky wrote:
> > Is this supposed to do anything, like exit the program? Or is it just a
> warning?
> 
> please add return -1 here. good find.

Done.

[vandebo (ex-Chrome), 2013-08-07 17:29:48]

LGTM

https://codereview.chromium.org/21715002/diff/16001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21715002/diff/16001/gm/gmmain.cpp#newcode2012
gm/gmmain.cpp:2012: int moduloRemainder = -1, moduloDivisor = -1;
nit: two int lines.

[ducky, 2013-08-12 22:01:30]

Fixes the modulo vars; additionally some more significant changes to un-break
the NoGPU builds

[vandebo (ex-Chrome), 2013-08-13 17:50:32]

LGTM

[commit-bot: I haz the power, 2013-08-13 20:13:01]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/21715002/37002

[commit-bot: I haz the power, 2013-08-14 11:36:18]

Change committed as 10703

[scroggo, 2013-08-14 14:11:22]

Change was reverted with https://code.google.com/p/skia/source/detail?r=10708.
Please fix the problems and resubmit.