[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 #219167|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #15 from Darin Adler <darin at apple.com>  2013-12-13 11:04:53 PST ---
(From update of attachment 219167)
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.

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