[Webkit-unassigned] [Bug 123185] Refactor option parsing in WebKitTestRunner

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123185


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #219277|review?, commit-queue?      |review+, commit-queue+
               Flag|                            |




--- Comment #17 from Darin Adler <darin at apple.com>  2013-12-15 20:27:17 PST ---
(From update of attachment 219277)
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;

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.