[webkit-reviews] review granted: [Bug 123185] Refactor option parsing in WebKitTestRunner : [Attachment 219277] Updated patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tamas Gergely
<tgergely.u-szeged at partner.samsung.com>'s request for review:
Bug 123185: Refactor option parsing in WebKitTestRunner
https://bugs.webkit.org/show_bug.cgi?id=123185

Attachment 219277: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=219277&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219277&action=review


All fine to land, although we could do better.

> Tools/WebKitTestRunner/Options.cpp:122
> +OptionsHandler::OptionsHandler(Options& o)

Should name the argument options rather than o.

> Tools/WebKitTestRunner/Options.cpp:136
> +    optionList.append(Option("--timeout", "Sets long timeout to <param> and
scales short timeout.", handleOptionTimeout, true));
> +    optionList.append(Option("--no-timeout", "Disables timeout.",
handleOptionNoTimeout));
> +    optionList.append(Option("--no-timeout-at-all", "Disables all
timeouts.", handleOptionNoTimeoutAtAll));
> +    optionList.append(Option("--verbose", "Turns on messages.",
handleOptionVerbose));
> +    optionList.append(Option("--gc-between-tests", "Garbage collection
between tests.", handleOptionGcBetweenTests));
> +    optionList.append(Option("--pixel-tests", "Check pixels.",
handleOptionPixelTests));
> +    optionList.append(Option("-p", "Check pixels.",
handleOptionPixelTests));
> +    optionList.append(Option("--print-supported-features", "For
DumpRenderTree compatibility.", handleOptionPrintSupportedFeatures));
> +    optionList.append(Option("--complex-text", "Force complex tests.",
handleOptionComplexText));
> +    optionList.append(Option("--accelerated-drawing", "Use accelerated
drawing.", handleOptionAcceleratedDrawing));
> +    optionList.append(Option("--remote-layer-tree", "Use remote layer
tree.", handleOptionRemoteLayerTree));
> +    optionList.append(Option(0, 0, handleOptionUnmatched));

Not really sure why we are building up a vector at runtime for something that
is a fixed list known at compile time.

> Tools/WebKitTestRunner/Options.cpp:140
> +const char * OptionsHandler::usage = "Usage: WebKitTestRunner [options]
filename [filename2..n]";
> +const char * OptionsHandler::help = "Displays this help.";

Extra space here. Should just be "const char*", not "const char *".

> Tools/WebKitTestRunner/Options.cpp:143
> +Option::Option(const char* name, const char* description,
std::function<bool(Options&, const char*, const char*)> parameterHandler, bool
hasArgument)
> +    : name(name), description(description),
parameterHandler(parameterHandler), hasArgument(hasArgument) { };

Formatting here is not right for our usual syntax. We put the function body at
the far left, not on the end of the initializers line.

Also, an extraneous semicolon at the end.

> Tools/WebKitTestRunner/Options.cpp:164
> +		       const char * currentArgument = argv[++argCounter];

Extra space here. Should just be "const char*", not "const char *".

> Tools/WebKitTestRunner/Options.h:39
> +    Options(double, double);

Need argument names here, otherwise it’s not clear what the two arguments are.

> Tools/WebKitTestRunner/Options.h:53
> +    double defaultLongTimeout;
> +    double defaultShortTimeout;

These two could be private and const.

> Tools/WebKitTestRunner/Options.h:57
> +class Option {
> +public:

Since everything is public I would probably make this a struct.

> Tools/WebKitTestRunner/Options.h:58
> +    Option(const char* name, const char* description,
std::function<bool(Options&, const char*, const char*)> parameterHandler, bool
hasArgument = false);

Need names for the two const char* arguments to make clear what they are. That
means we probably should use a typedef.

> Tools/WebKitTestRunner/Options.h:75
> +    static const char* usage;
> +    static const char* help;

Not sure why these are members. They could just be non-member globals. Also,
they should be const:

    static const char* const usage;