[webkit-reviews] review granted: [Bug 123185] Refactor option parsing in WebKitTestRunner : [Attachment 219167] 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 219167: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=219167&action=review

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


Looks OK, lost of style mistakes.

> Tools/WebKitTestRunner/Options.cpp:34
> +Options::Options(double _default_long_timeout, double
_default_short_timeout)

Should not have identifiers with underscores in them, especially not leading
underscores. defaultLongTimeout and defaultShortTimeout.

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

Not our style to have a space before the *.

> Tools/WebKitTestRunner/Options.cpp:159
> +	   for (Option &option : optionlist) {

Not our style to have a space before the & and no space after the &.

> Tools/WebKitTestRunner/Options.cpp:163
> +		   if (!option.paramhandler) {
> +		       status = false;
> +		   } else if (option.hasargument) {

Not our style to have braces around one line if statement body.

> Tools/WebKitTestRunner/Options.cpp:184
> +    for (Option &option : optionlist) {

Same problem with option formatting.

> Tools/WebKitTestRunner/Options.h:52
> +    double m_longTimeout;
> +    double m_shortTimeout;
> +    bool m_useWaitToDumpWatchdogTimer;
> +    bool m_forceNoTimeout;
> +    bool m_verbose;
> +    bool m_gcBetweenTests;
> +    bool m_shouldDumpPixelsForAllTests;
> +    bool m_printSupportedFeatures;
> +    bool m_forceComplexText;
> +    bool m_shouldUseAcceleratedDrawing;
> +    bool m_shouldUseRemoteLayerTree;
> +    std::vector<std::string> m_paths;

We normally use the "m_" for private data members, not public ones. I also
suggest making this a struct.

> Tools/WebKitTestRunner/Options.h:54
> +    double constDefaultLongTimeout;
> +    double constDefaultShortTimeout;

What does “const” mean here? These are not const. What	is this?

> Tools/WebKitTestRunner/Options.h:63
> +    std::function<bool(Options&, const char*, const char*)> paramhandler;

Should be parameterHandler rather than paramhandler.

> Tools/WebKitTestRunner/Options.h:64
> +    bool hasargument;

Should be hasArgument with capital “A”.

> Tools/WebKitTestRunner/Options.h:69
> +    OptionsHandler(Options&);

Should be marked explicit.

> Tools/WebKitTestRunner/Options.h:70
> +    bool parse(int, const char*[]);

Should have argument names here.

> Tools/WebKitTestRunner/Options.h:71
> +    void printhelp(FILE* channel = stderr);

Should be printHelp not printhelp.

> Tools/WebKitTestRunner/Options.h:73
> +    WTF::Vector<Option> optionlist;

No need for WTF:: here. Also, should be optionList rather than optionlist with
no capital letter.

> Tools/WebKitTestRunner/TestController.cpp:270
> +    Options wtrOptions(defaultLongTimeout, defaultShortTimeout);

Strange to have “wtr” here. Can we just call this local variable “options”.

> Tools/WebKitTestRunner/TestController.cpp:271
> +    OptionsHandler handleCLargs(wtrOptions);

A handler should not have a verb as its name. Also strange that “args” is lower
case. I would call this “commandLineOptionsHandler” or “optionsHandler” or
“handler” rather than “handleCLargs”.

> Tools/WebKitTestRunner/TestController.cpp:731
> -	   if (strlen(filenameBuffer) == 0)
> +	   if (!strlen(filenameBuffer))
>	       continue;

If changing this at all, I suggest changing it to:

    if (!filenameBuffer[0])

It’s silly to find the string length just to check if the first character is a
null character.