share dm and command flags

share dm and command flags

Share command flags between dm and unit tests.
Also, allow dm's core to be included by itself and iOSShell.

Command line flags that are the same (or nearly the same) in DM
and in skia_tests have been moved to common_flags. Authors,
please check to see that the shared common flag is correct for
the tool.

For iOS, the 'tool_main' entry point has a wrapper to allow multiple
tools to be statically linked in the iOSShell. 
Since SkCommandLineFlags::Parse can only be called once, these calls
are disabled in the IOS build.

Since the iOS app directory is dynamically assigned a name, use '@' to
select it. (This is the same convention chosen by the Mobile Harness
iOS file system utilities.)

Move the heart of dm.gyp into dm.gypi so that it can be included by
itself and iOSShell.gyp.

Add tools/flags/SkCommonFlags.* to define and declare common 
command line flags.

Add support for dm to iOSShell.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/17f0b6df7248b9bbdaddacc3a6c9c6efe4ae278e

[scroggo, 2014-07-11 21:12:15]

lgtm.

I'm fine with you checking in as-is, but I do have some suggestions as well:

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gyp
File gyp/dm.gyp (right):

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gyp#newcode8
gyp/dm.gyp:8: 'type': 'executable',
I wonder if this is the right separation between dm.gyp and dm.gypi. For
example, you needed to add some of the same includes to iOSShell.gyp just to get
the same thing as dm has elsewhere. Why not include those in dm.gypi?

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gypi
File gyp/dm.gypi (right):

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gypi#newcode2
gyp/dm.gypi:2: # vim: set expandtab tabstop=4 shiftwidth=4
I like the explanatory comment on line 1, but I think we removed the vim
settings from our other gyp files?

https://codereview.chromium.org/389653004/diff/1/gyp/flags.gyp
File gyp/flags.gyp (right):

https://codereview.chromium.org/389653004/diff/1/gyp/flags.gyp#newcode9
gyp/flags.gyp:9: '../tools/flags/SkCommonFlags.cpp',
I'm not sure these belong in the same library. What if a target wants to use
SkCommandLineFlags but doesn't want the common ones? Or it wants to define them
differently?

(If you push back I'm not completely opposed; but I do lean towards separating
it out.)

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode124
tests/skia_test.cpp:124: int TOOL_MAIN(int argc, char** argv);
Is there any harm in always naming this test_main? Same question for dm.

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode128
tests/skia_test.cpp:128: SkCommandLineFlags::SetUsage("");
What if we moved this code into main, down below? Then we don't need the extra
ifdef, and I think we don't need to pass argc or argv to test_main. Same
suggestion for dm.

[caryclark, 2014-07-14 12:24:46]

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gyp
File gyp/dm.gyp (right):

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gyp#newcode8
gyp/dm.gyp:8: 'type': 'executable',
On 2014/07/11 21:12:14, scroggo wrote:
> I wonder if this is the right separation between dm.gyp and dm.gypi. For
> example, you needed to add some of the same includes to iOSShell.gyp just to
get
> the same thing as dm has elsewhere. Why not include those in dm.gypi?

Done.

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gypi
File gyp/dm.gypi (right):

https://codereview.chromium.org/389653004/diff/1/gyp/dm.gypi#newcode2
gyp/dm.gypi:2: # vim: set expandtab tabstop=4 shiftwidth=4
On 2014/07/11 21:12:14, scroggo wrote:
> I like the explanatory comment on line 1, but I think we removed the vim
> settings from our other gyp files?

Done.

https://codereview.chromium.org/389653004/diff/1/gyp/flags.gyp
File gyp/flags.gyp (right):

https://codereview.chromium.org/389653004/diff/1/gyp/flags.gyp#newcode9
gyp/flags.gyp:9: '../tools/flags/SkCommonFlags.cpp',
My thought is the target can #include "SkCommandLineFlags.h" if they don't want
the common ones. If they want to define them differently, then, Houston, we have
a problem. The common flags should only be ones we feel comfortable with across
all tools.

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode124
tests/skia_test.cpp:124: int TOOL_MAIN(int argc, char** argv);
On 2014/07/11 21:12:14, scroggo wrote:
> Is there any harm in always naming this test_main? Same question for dm.

It's a bit conservative in case there was some further interest in Derek's code
that dynamically loads libraries, and therefore would want a known named entry
point. However, if we want to remove that, then there's no harm naming this
test_main and the other dm_main.

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode128
tests/skia_test.cpp:128: SkCommandLineFlags::SetUsage("");
Same argument: if we want to support dynamic loading, the entry point has to do
all the work. But, if we want to remove that code, then your suggestion sounds
good to me.

[caryclark, 2014-07-14 13:00:47]

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode124
tests/skia_test.cpp:124: int TOOL_MAIN(int argc, char** argv);
I checked with Derek and he now uses main() instead of test_main() as his entry
point, so that removes my main (no pun intended) concern. Renamed as Leon
initially suggested (done).

https://codereview.chromium.org/389653004/diff/1/tests/skia_test.cpp#newcode128
tests/skia_test.cpp:128: SkCommandLineFlags::SetUsage("");
Code moved as Leon suggested. (done)

[scroggo, 2014-07-14 13:24:16]

LGTM

https://codereview.chromium.org/389653004/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/389653004/diff/20001/dm/DM.cpp#newcode224
dm/DM.cpp:224: int dm_main(int argc, char** argv) {
argc and argv are no longer used.

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:119: int test_main(int argc, char** argv) {
I don't think argc and argv are necessary here anymore.

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:221: SkCommandLineFlags::SetUsage("");
I don't think this line is necessary (it just changes the usage string from ""
to "" - note that it's left out of DM.cpp).

[mtklein, 2014-07-14 13:29:59]

What would this all look like if we just remove support for tests and replace it
with DM?

[caryclark, 2014-07-14 15:07:12]

https://codereview.chromium.org/389653004/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/389653004/diff/20001/dm/DM.cpp#newcode224
dm/DM.cpp:224: int dm_main(int argc, char** argv) {
On 2014/07/14 13:24:16, scroggo wrote:
> argc and argv are no longer used.

Done.

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:119: int test_main(int argc, char** argv) {
On 2014/07/14 13:24:16, scroggo wrote:
> I don't think argc and argv are necessary here anymore.

Done.

https://codereview.chromium.org/389653004/diff/20001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:221: SkCommandLineFlags::SetUsage("");
On 2014/07/14 13:24:16, scroggo wrote:
> I don't think this line is necessary (it just changes the usage string from ""
> to "" - note that it's left out of DM.cpp).

Done.

[caryclark, 2014-07-14 15:07:37]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2014-07-14 15:07:58]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/caryclark@google.com/389653004/40001

[commit-bot: I haz the power, 2014-07-14 15:34:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[commit-bot: I haz the power, 2014-07-14 15:39:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-14 15:39:26]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[caryclark, 2014-07-15 14:15:24]

put Jim on the review so he can play with the CL

[caryclark, 2014-07-22 15:49:16]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2014-07-22 15:49:53]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/caryclark@google.com/389653004/230001

[commit-bot: I haz the power, 2014-07-22 15:50:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 15:50:01]

Presubmit check for 389653004-230001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[caryclark, 2014-07-22 15:51:23]

Brian, please review (addition to public api)

[bsalomon, 2014-07-22 16:02:21]

On 2014/07/22 15:51:23, caryclark wrote:
> Brian, please review (addition to public api)

lgtm

[caryclark, 2014-07-22 17:03:03]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2014-07-22 17:03:59]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/caryclark@google.com/389653004/230001

[commit-bot: I haz the power, 2014-07-22 17:15:40]

Change committed as 17f0b6df7248b9bbdaddacc3a6c9c6efe4ae278e