Bug 123185 - Refactor option parsing in WebKitTestRunner
Summary: Refactor option parsing in WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-22 16:39 PDT by Tamas Gergely
Modified: 2013-12-15 20:55 PST (History)
12 users (show)

See Also:


Attachments
Refactor options handling in WKRT. (18.97 KB, patch)
2013-10-22 16:42 PDT, Tamas Gergely
ossy: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (279.95 KB, application/zip)
2013-10-22 17:17 PDT, Build Bot
no flags Details
Refactor options parsing. (19.05 KB, patch)
2013-10-30 03:18 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff
Refactor options parsing in WTR (19.04 KB, patch)
2013-10-31 05:56 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff
Options refactoring (19.25 KB, patch)
2013-11-22 04:13 PST, Tamas Gergely
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Options refactoring (corrected) (19.25 KB, patch)
2013-11-22 05:17 PST, Tamas Gergely
no flags Details | Formatted Diff | Diff
Updated patch. (21.03 KB, patch)
2013-12-13 04:44 PST, Tamas Gergely
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (20.61 KB, patch)
2013-12-15 07:59 PST, Tamas Gergely
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tamas Gergely 2013-10-22 16:39:05 PDT
There is a FIXME in TestContoller.cpp on refactoring the options parsing of WebKitTestRunner in order to automatically generate help message.
Comment 1 Tamas Gergely 2013-10-22 16:42:48 PDT
Created attachment 214902 [details]
Refactor options handling in WKRT.
Comment 2 WebKit Commit Bot 2013-10-22 16:45:22 PDT
Attachment 214902 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/CMakeLists.txt', u'Tools/WebKitTestRunner/GNUmakefile.am', u'Tools/WebKitTestRunner/Options.cpp', u'Tools/WebKitTestRunner/Options.h', u'Tools/WebKitTestRunner/TestController.cpp', u'Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj']" exit_code: 1
Tools/WebKitTestRunner/Options.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.h:34:  Streams are highly discouraged.  [readability/streams] [3]
Tools/WebKitTestRunner/Options.h:50:  c_defaultLongTimeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.h:51:  c_defaultShortTimeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.h:66:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/Options.h:68:  Missing spaces around =  [whitespace/operators] [4]
Tools/WebKitTestRunner/Options.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.cpp:29:  Streams are highly discouraged.  [readability/streams] [3]
Tools/WebKitTestRunner/Options.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/Options.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:46:  handle_option_timeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:53:  handle_option_no_timeout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:58:  handle_option_no_timeout_at_all is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:64:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:64:  handle_option_verbose is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:69:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:69:  handle_option_gc_between_tests is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:74:  handle_option_pixel_tests is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:79:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:79:  handle_option_print_supported_features is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:84:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:84:  handle_option_unmatched is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:92:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:108:  Missing space inside { }.  [whitespace/braces] [5]
Tools/WebKitTestRunner/Options.cpp:110:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:116:  Missing spaces around =  [whitespace/operators] [4]
Tools/WebKitTestRunner/Options.cpp:116:  Missing spaces around <  [whitespace/operators] [3]
Tools/WebKitTestRunner/Options.cpp:116:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:116:  arg_counter is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Tools/WebKitTestRunner/Options.cpp:117:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:118:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:121:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:122:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:124:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:125:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:127:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:129:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:131:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:133:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:135:  Use 0 instead of NULL.  [readability/null] [5]
Tools/WebKitTestRunner/Options.cpp:136:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:146:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:147:  Should have a space between // and comment  [whitespace/comments] [4]
Tools/WebKitTestRunner/Options.cpp:148:  Missing space before ( in for(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:149:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/WebKitTestRunner/Options.cpp:151:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/Options.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Tools/WebKitTestRunner/TestController.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 56 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2013-10-22 17:17:33 PDT
Comment on attachment 214902 [details]
Refactor options handling in WKRT.

Attachment 214902 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/9358035

New failing tests:
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
http/tests/cache/display-image-unset-allows-cached-image-load.html
animations/3d/change-transform-in-end-event.html
http/tests/cache/content-type-ignored-during-revalidation.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/cancel-during-revalidation-succeeded.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
accessibility/adjacent-continuations-cause-assertion-failure.html
compositing/absolute-position-changed-with-composited-parent-layer.html
Comment 4 Build Bot 2013-10-22 17:17:35 PDT
Created attachment 214909 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Csaba Osztrogonác 2013-10-24 09:19:56 PDT
Comment on attachment 214902 [details]
Refactor options handling in WKRT.

The idea is good to have a better option parser.
But first please fix the style issues and crashes.
Comment 6 Tamas Gergely 2013-10-30 03:18:04 PDT
Created attachment 215489 [details]
Refactor options parsing.

This will probably fix Mac issues.
Comment 7 Tamas Gergely 2013-10-31 05:56:34 PDT
Created attachment 215645 [details]
Refactor options parsing in WTR
Comment 8 Peter Gal 2013-11-21 01:32:57 PST
Comment on attachment 215645 [details]
Refactor options parsing in WTR

View in context: https://bugs.webkit.org/attachment.cgi?id=215645&action=review

> Tools/WebKitTestRunner/Options.cpp:34
> +Options::Options(double dlto, double dsto):

What does dlto/dsto mean? If possible we should not use abbreviations.

> Tools/WebKitTestRunner/Options.cpp:48
> +bool handleOptionTimeout(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:56
> +bool handleOptionNoTimeout(Options& o, const char* opt, const char* arg)

ditto

> Tools/WebKitTestRunner/Options.cpp:62
> +bool handleOptionNoTimeoutAtAll(Options& o, const char* opt, const char* arg)

ditto

> Tools/WebKitTestRunner/Options.cpp:69
> +bool handleOptionVerbose(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:75
> +bool handleOptionGcBetweenTests(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:81
> +bool handleOptionPixelTests(Options& o, const char* opt, const char* arg)

ditto.

> Tools/WebKitTestRunner/Options.cpp:87
> +bool handleOptionPrintXSupportedFeatures(Options& o, const char* opt, const char* arg)

ditto.

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

ditto.

> Tools/WebKitTestRunner/Options.cpp:103
> +    add(Option("--timeout",                  "Sets long timeout to <param> and scales short timeout.", handleOptionTimeout, true));

There are too many whitespace before the option description, we should keep only one.

> Tools/WebKitTestRunner/Options.cpp:128
> +    for (int argCounter = 1; argCounter < argc; argCounter++) {

++argCounter instead of argCounter++.

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

don't use abbreviations.
'Option&' instead of 'Option &'

> Tools/WebKitTestRunner/Options.cpp:153
> +void OptionsHandler::help(FILE *channel)

'FILE*' instead of the 'FILE *'

> Tools/WebKitTestRunner/Options.cpp:170
> +void OptionsHandler::add(Option o)

Do we need this method at all? We only use it in the handler constructor. On the side note: Maybe we can use Option& here to avoid copy.

> Tools/WebKitTestRunner/Options.h:59
> +    const char* desc;

desc-> description.

> Tools/WebKitTestRunner/Options.h:74
> +    static const char * usagestr;
> +    static const char * helpstr;

maybe we can remove the str part from the variable name, and 'char*' instead of 'char *'.

> Tools/WebKitTestRunner/TestController.cpp:298
> +    m_longTimeout                 = wtrOptions.m_longTimeout;
> +    m_shortTimeout                = wtrOptions.m_shortTimeout;
> +    m_useWaitToDumpWatchdogTimer  = wtrOptions.m_useWaitToDumpWatchdogTimer;
> +    m_forceNoTimeout              = wtrOptions.m_forceNoTimeout;
> +    m_verbose                     = wtrOptions.m_verbose;
> +    m_gcBetweenTests              = wtrOptions.m_gcBetweenTests;
> +    m_shouldDumpPixelsForAllTests = wtrOptions.m_shouldDumpPixelsForAllTests;
> +    m_paths                       = wtrOptions.m_paths;

keep only one space before the = signs.
Comment 9 Tamas Gergely 2013-11-22 04:13:10 PST
Created attachment 217667 [details]
Options refactoring

Modified according to the comments.
Comment 10 EFL EWS Bot 2013-11-22 04:18:05 PST
Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/33868042
Comment 11 Build Bot 2013-11-22 04:46:31 PST
Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/33748041
Comment 12 Build Bot 2013-11-22 04:46:52 PST
Comment on attachment 217667 [details]
Options refactoring

Attachment 217667 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/33848056
Comment 13 Tamas Gergely 2013-11-22 05:17:43 PST
Created attachment 217672 [details]
Options refactoring (corrected)

Sorry, previous patch was not the final one. But I'm surprised that it passed two checks.
Comment 14 Tamas Gergely 2013-12-13 04:44:12 PST
Created attachment 219167 [details]
Updated patch.

The 3 new options are included.
Comment 15 Darin Adler 2013-12-13 11:04:53 PST
Comment on attachment 219167 [details]
Updated patch.

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.
Comment 16 Tamas Gergely 2013-12-15 07:59:23 PST
Created attachment 219277 [details]
Updated patch.

I renamed the identifiers, and left line Tools/WebKitTestRunner/TestController.cpp:731 unchanged (it does not belong to this change).
Comment 17 Darin Adler 2013-12-15 20:27:17 PST
Comment on attachment 219277 [details]
Updated patch.

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;
Comment 18 WebKit Commit Bot 2013-12-15 20:54:57 PST
Comment on attachment 219277 [details]
Updated patch.

Clearing flags on attachment: 219277

Committed r160627: <http://trac.webkit.org/changeset/160627>
Comment 19 WebKit Commit Bot 2013-12-15 20:55:00 PST
All reviewed patches have been landed.  Closing bug.