Make gm use SkFlags.

Make gm use SkFlags.

Make flag parser its own project. It is still in the tools folder,
but can now be used by other projects.

Some changes to behavior in order to work with SkFlags:
enable-missing-warning and disable-missing-warning have become
enableMissingWarning and noenableMissingWarning.

exclude-config is now excludeConfig

--config now can only be listed once, but all configs listed
after it will be included (same with excludeConfig and match).

In addition, writeJsonSummary has been changed to
writeJsonSummaryPath for consistency with other path flags.

Provide an option to SkFlags to provide a short name, and use
it for the flags which have short names.

--tileGridReplayScales now takes space separated arguments,
like other inputs.

BUG=https://code.google.com/p/skia/issues/detail?id=1094

Committed: https://code.google.com/p/skia/source/detail?r=8235

[epoger, 2013-03-15 04:23:08]

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode11
gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
gm/tests/run.sh
As alluded to here, I think you'll have to update run.sh and its results due to
the changes in flag handling.

For example, the repeated use of --config in
https://code.google.com/p/skia/source/browse/trunk/gm/tests/run.sh?r=8126#24
will need to be replaced, which will in turn affect
https://code.google.com/p/skia/source/browse/trunk/gm/tests/outputs/compared-...
and similar files.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode898
gm/gmmain.cpp:898: installFilter(&canvas);
I'm leery of combining this change with the much larger flags change... For
example, if we end up rolling back this CL due to either this part or the flags
part, it's too bad that *both* will be rolled back.

On a related note, it disturbs me that this error went unnoticed, and ideally
this fix would be accompanied by self-test improvements that exercise it and
related behavior.  (Which, again, would be better off segregated into a separate
CL)

On the other hand, I know that every additional CL takes more time, so it's a
balancing act...

Long story short: I just wanted to make sure you were aware of such
considerations.  I trust your judgment as to whether, on balance, we should
include this change here or bring it in as its own CL.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1055
gm/gmmain.cpp:1055: // NOTE in the change log: appended 'Path' to the flag
delete the NOTE line?

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1168
gm/gmmain.cpp:1168: SkDebugf("unrecognized config %s\n", FLAGS_config[i]);
As of https://code.google.com/p/skia/source/detail?r=8126 , we use gm_fprintf()
in place of fprintf() and SkDebugf() in this file.  You'll need to incorporate
that change sometime before you commit this CL.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1185
gm/gmmain.cpp:1185: // FIXME: auto delete this
fix the fixme? Or change to TODO(scroggo) for later work?

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1547
gm/gmmain.cpp:1547: if (FLAGS_writeJsonSummaryPath.count() == 1) {
IIRC, our style guide calls for writing "1 == " on the left hand side.

[epoger, 2013-03-15 14:33:51]

P.S. I neglected to mention... thanks for doing this!  Let me know if I can help
with any of the merge/cleanup stuff I mentioned previously.

[scroggo, 2013-03-15 15:15:59]

I'll go through the other comments next, but first I wanted to separate out the
installFilter changes (see https://codereview.chromium.org/12812016)

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode898
gm/gmmain.cpp:898: installFilter(&canvas);
On 2013/03/15 04:23:08, epoger wrote:
> I'm leery of combining this change with the much larger flags change... For
> example, if we end up rolling back this CL due to either this part or the
flags
> part, it's too bad that *both* will be rolled back.

Agreed. I've created a separate CL for these:
https://codereview.chromium.org/12812016.

> 
> On a related note, it disturbs me that this error went unnoticed, and ideally
> this fix would be accompanied by self-test improvements that exercise it and
> related behavior.  (Which, again, would be better off segregated into a
separate
> CL)

installFilter only does anything if the flag --forceBWtext is used, which forces
using non-antialiased text. I don't know how important that is, but I'm guessing
it's not high priority since it appears we are not testing it. Is it worth the
extra run time to run all the tests in that mode as well? Tiled pipe playback
(the other addition of installFilter) is deliberately not tested since it fails
for other reasons (see https://code.google.com/p/skia/issues/detail?id=635).

> 
> On the other hand, I know that every additional CL takes more time, so it's a
> balancing act...
> 
> Long story short: I just wanted to make sure you were aware of such
> considerations.  I trust your judgment as to whether, on balance, we should
> include this change here or bring it in as its own CL.

[scroggo, 2013-03-15 16:34:20]

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode11
gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
gm/tests/run.sh
On 2013/03/15 04:23:08, epoger wrote:
> As alluded to here, I think you'll have to update run.sh and its results due
to
> the changes in flag handling.
> 
> For example, the repeated use of --config in
> https://code.google.com/p/skia/source/browse/trunk/gm/tests/run.sh?r=8126#24
> will need to be replaced, which will in turn affect
>
https://code.google.com/p/skia/source/browse/trunk/gm/tests/outputs/compared-...
> and similar files.

Done.

Oddly, with or without my changes, the script tells me that the last step
failed, but follows that by stating that all tests passed.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1055
gm/gmmain.cpp:1055: // NOTE in the change log: appended 'Path' to the flag
On 2013/03/15 04:23:08, epoger wrote:
> delete the NOTE line?

Done.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1168
gm/gmmain.cpp:1168: SkDebugf("unrecognized config %s\n", FLAGS_config[i]);
On 2013/03/15 04:23:08, epoger wrote:
> As of https://code.google.com/p/skia/source/detail?r=8126 , we use
gm_fprintf()
> in place of fprintf() and SkDebugf() in this file.  You'll need to incorporate
> that change sometime before you commit this CL.

Done.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1185
gm/gmmain.cpp:1185: // FIXME: auto delete this
On 2013/03/15 04:23:08, epoger wrote:
> fix the fixme? Or change to TODO(scroggo) for later work?

Already fixed, just left in the comment...

Removed.

And then I took it a step further and removed the creation of the object to
begin with. Now tileGridReplayScales uses space as a separator, so it works like
everything else.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1547
gm/gmmain.cpp:1547: if (FLAGS_writeJsonSummaryPath.count() == 1) {
On 2013/03/15 04:23:08, epoger wrote:
> IIRC, our style guide calls for writing "1 == " on the left hand side.

The style guide actually states to put lvalues on the right hand side when
comparing r- and l-values. These are both rvalues, so I think it can go either
way.

That said, if you think it's clearer to have 1 on the left I'm happy to change.
After all count() could be malicious and return an int& which could then be
modified. (We have other calls that surprisingly return an lvalue, like
SkTDArray::append() which returns a pointer.)

[epoger, 2013-03-15 17:13:38]

LGTM!

Thanks for the thorough job!

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode11
gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
gm/tests/run.sh
On 2013/03/15 16:34:20, scroggo wrote:
> On 2013/03/15 04:23:08, epoger wrote:
> > As alluded to here, I think you'll have to update run.sh and its results due
> to
> > the changes in flag handling.
> > 
> > For example, the repeated use of --config in
> > https://code.google.com/p/skia/source/browse/trunk/gm/tests/run.sh?r=8126#24
> > will need to be replaced, which will in turn affect
> >
>
https://code.google.com/p/skia/source/browse/trunk/gm/tests/outputs/compared-...
> > and similar files.
> 
> Done.
> 
> Oddly, with or without my changes, the script tells me that the last step
> failed, but follows that by stating that all tests passed.

You mean something like this?

GM: drawing... selftest1 [300 200]
GM: ---- 8888/selftest1: 60000 (of 60000) differing pixels, max per-channel
mismatch R=0 G=111 B=103 A=0
GM: ---- 565/selftest1: not computing max per-channel pixel mismatch because
non-8888
GM: Ran 1 tests: 0 passed, 1 failed, 0 missing reference images
GM: 		8888/selftest1 pixel_error
GM: 		565/selftest1 pixel_error
  Leaked SkRefCnt: 1
     Leaked SkPathRef: 1
All tests passed.

Yes, some of the gm runs executed by the self-test report failures.  And they're
supposed to.  I can see how that is confusing, though.  Let me know if you have
any ideas on making that clearer.

https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1547
gm/gmmain.cpp:1547: if (FLAGS_writeJsonSummaryPath.count() == 1) {
On 2013/03/15 16:34:20, scroggo wrote:
> On 2013/03/15 04:23:08, epoger wrote:
> > IIRC, our style guide calls for writing "1 == " on the left hand side.
> 
> The style guide actually states to put lvalues on the right hand side when
> comparing r- and l-values. These are both rvalues, so I think it can go either
> way.
> 
> That said, if you think it's clearer to have 1 on the left I'm happy to
change.
> After all count() could be malicious and return an int& which could then be
> modified. (We have other calls that surprisingly return an lvalue, like
> SkTDArray::append() which returns a pointer.)

I suppose I would put the "1" on the left hand side, since I *know* I'm not
gonna mess up "1".  :-)  But I'm happy either way.

https://codereview.chromium.org/12632015/diff/19001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-different-pixels-images/output-expected/command_line
(right):

https://codereview.chromium.org/12632015/diff/19001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-different-pixels-images/output-expected/command_line:1:
out/Debug/gm --hierarchy --match selftest1 --config 8888 565 -r
gm/tests/inputs/images/different-pixels --writeJsonSummaryPath
gm/tests/outputs/compared-against-different-pixels-images/output-actual/json-summary.txt
Cool, thanks for updating the self-test stuff!

Hopefully, you found gm/tests/rebaseline.sh that will update this stuff for you.
 I now realize I should have pointed  you at that file...

[scroggo, 2013-03-18 18:05:47]

> P.S. I neglected to mention... thanks for doing this!
On 2013/03/15 17:13:38, epoger wrote:
> LGTM!
> 
> Thanks for the thorough job!

Glad to hear it's appreciated! I was doing this selfishly because I find our
current system a mess and a hassle for me to update, and I hope people besides
me find it useful.

> 
> https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp
> File gm/gmmain.cpp (right):
> 
> https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode11
> gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
> gm/tests/run.sh
> On 2013/03/15 16:34:20, scroggo wrote:
> > On 2013/03/15 04:23:08, epoger wrote:
> > > As alluded to here, I think you'll have to update run.sh and its results
due
> > to
> > > the changes in flag handling.
> > > 
> > > For example, the repeated use of --config in
> > >
https://code.google.com/p/skia/source/browse/trunk/gm/tests/run.sh?r=8126#24
> > > will need to be replaced, which will in turn affect
> > >
> >
>
https://code.google.com/p/skia/source/browse/trunk/gm/tests/outputs/compared-...
> > > and similar files.
> > 
> > Done.
> > 
> > Oddly, with or without my changes, the script tells me that the last step
> > failed, but follows that by stating that all tests passed.
> 
> You mean something like this?
> 
> GM: drawing... selftest1 [300 200]
> GM: ---- 8888/selftest1: 60000 (of 60000) differing pixels, max per-channel
> mismatch R=0 G=111 B=103 A=0
> GM: ---- 565/selftest1: not computing max per-channel pixel mismatch because
> non-8888
> GM: Ran 1 tests: 0 passed, 1 failed, 0 missing reference images
> GM: 		8888/selftest1 pixel_error
> GM: 		565/selftest1 pixel_error
>   Leaked SkRefCnt: 1
>      Leaked SkPathRef: 1
> All tests passed.
> 
> Yes, some of the gm runs executed by the self-test report failures.  And
they're
> supposed to.  I can see how that is confusing, though.  Let me know if you
have
> any ideas on making that clearer.

Hmmm... maybe a comment after the failure saying the above failure is okay?
That's probably no better than the "All tests passed." which did not comfort me
enough...

> 
> https://codereview.chromium.org/12632015/diff/1006/gm/gmmain.cpp#newcode1547
> gm/gmmain.cpp:1547: if (FLAGS_writeJsonSummaryPath.count() == 1) {
> On 2013/03/15 16:34:20, scroggo wrote:
> > On 2013/03/15 04:23:08, epoger wrote:
> > > IIRC, our style guide calls for writing "1 == " on the left hand side.
> > 
> > The style guide actually states to put lvalues on the right hand side when
> > comparing r- and l-values. These are both rvalues, so I think it can go
either
> > way.
> > 
> > That said, if you think it's clearer to have 1 on the left I'm happy to
> change.
> > After all count() could be malicious and return an int& which could then be
> > modified. (We have other calls that surprisingly return an lvalue, like
> > SkTDArray::append() which returns a pointer.)
> 
> I suppose I would put the "1" on the left hand side, since I *know* I'm not
> gonna mess up "1".  :-)  But I'm happy either way.
> 
>
https://codereview.chromium.org/12632015/diff/19001/gm/tests/outputs/compared...
> File
>
gm/tests/outputs/compared-against-different-pixels-images/output-expected/command_line
> (right):
> 
>
https://codereview.chromium.org/12632015/diff/19001/gm/tests/outputs/compared...
>
gm/tests/outputs/compared-against-different-pixels-images/output-expected/command_line:1:
> out/Debug/gm --hierarchy --match selftest1 --config 8888 565 -r
> gm/tests/inputs/images/different-pixels --writeJsonSummaryPath
>
gm/tests/outputs/compared-against-different-pixels-images/output-actual/json-summary.txt
> Cool, thanks for updating the self-test stuff!
> 
> Hopefully, you found gm/tests/rebaseline.sh that will update this stuff for
you.
>  I now realize I should have pointed  you at that file...

No, I used a search and replace on all the files, including ones which were
generated so I didn't need to change :( Oh well. Will this cover what the bots
run? (Just want to double check to make sure I didn't break the build by
changing writeJsonSummary to writeJsonSummaryPath.)

[scroggo, 2013-03-19 17:28:34]

Committed patchset #4 manually as r8235 (presubmit successful).