[webkit-reviews] review granted: [Bug 197133] Make it possible to control the renderTreeAsText output by setting options on testRunner : [Attachment 367988] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 197133: Make it possible to control the renderTreeAsText output by setting
options on testRunner
https://bugs.webkit.org/show_bug.cgi?id=197133

Attachment 367988: Patch

https://bugs.webkit.org/attachment.cgi?id=367988&action=review




--- Comment #28 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 367988
  --> https://bugs.webkit.org/attachment.cgi?id=367988
Patch

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

> Tools/DumpRenderTree/TestRunner.cpp:115
> +    double options = JSValueToNumber(context, arguments[0], exception);
> +    ASSERT(!*exception);

Seems like this could throw if the value passed in is not convertible to a
number. Seems like checking for the exception and returning early would be
clearer to test writers than crashing. That may not be what we've done
elsewhere, but seems like a better pattern to me.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:95
> +    # Underscores to camel case.
> +    $constantName =~ s{(\w+)}{
> +	       ($a = lc $1) =~ s<(^[a-z]|_[a-z])><
> +		   ($b = uc $1) =~ s/^_//;
> +		   $b;
> +	       >eg;
> +	       $a;
> +	   }eg;

This seems unnecessary. I'd just keep it using underscores, which are totally
legal. This is generated code and doesn't have to conform to the normal rules.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:621
> +    my $attributes = [];

Can you call this $attributesAndConstants to make me happier :).

> LayoutTests/fast/harness/render-tree-as-text-options.html:36
> +		       testRunner.RENDER_TREE_SHOW_ALL_LAYERS + 
> +		       testRunner.RENDER_TREE_SHOW_LAYER_NESTING + 

Why '+' rather than the more traditional '|'? Is this the syntax you want, or
would rather it be TestRunner.RENDER_TREE_FOO, e.g. on the constructor?