[webkit-reviews] review granted: [Bug 57068] [GTK] [WebKit2] Implement a basic WebKitTestRunner : [Attachment 91330] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 57068: [GTK] [WebKit2] Implement a basic WebKitTestRunner
https://bugs.webkit.org/show_bug.cgi?id=57068

Attachment 91330: Patch
https://bugs.webkit.org/attachment.cgi?id=91330&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91330&action=review

> Tools/Scripts/webkitdirs.pm:636
> +	   foreach (@libraries) {
> +	       my $library = "$configurationProductDir/.libs/" . $_  .
$extension;

I think it's slightly more readable to use an explicit variable instead of the
implicit $_. "foreach my $libraryName (@libraries) {"

> Tools/Scripts/webkitdirs.pm:639
> +	       if (-e $library) {
> +		   return $library;
> +	       }

I'd write this as a one-liner: return $library if -e $library;

> Tools/Scripts/webkitdirs.pm:1996
> +	   return system "$productDir/Programs/WebKitTestRunner", @ARGV;

This won't do the right thing if $productDir contains a space and @ARGV is
empty. (The shell will split $productDir on the space and try to execute a
bogus path instead of WTR.) One way to avoid that is:

my @args = ("$productDir/Programs/WebKitTestRunner", @ARGV);
return system { $args[0] } @args;

> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:121
> +	   for (size_t path = 0; path < 2; path++) {
> +
> +	       if (g_file_test(fontPaths[font][path], G_FILE_TEST_EXISTS)) {

Extra newline in here.

You could use an early continue instead of nesting code inside this if.

> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:133
> +	   if (!found)
> +	       g_error("Could not find font at %s. Either install this font or
file a bug "
> +		       "at http://bugs.webkit.org if it is installed in another
location.",
> +		       fontPaths[font][0]);

This needs braces because it is multiple lines (even though it's only a single
statement).

> Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:39
> +}
> +
> +
> +void InjectedBundle::platformInitialize(WKTypeRef)

Extra newline in here.

> Tools/WebKitTestRunner/InjectedBundle/gtk/LayoutTestControllerGtk.cpp:66
> +JSRetainPtr<JSStringRef>
LayoutTestController::pathToLocalResource(JSStringRef url)
> +{
> +    return JSStringRetain(url); 
> +}

This causes a leak. If you remove the JSStringRetain call you should be fine.
(JSRetainPtr does it for you.)

> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:52
> +    gtk_widget_destroy(m_window);

Will this destroy m_view, too?

> Tools/WebKitTestRunner/gtk/main.cpp:33
> +    WTR::TestController controller(argc, const_cast<const char**>(argv));

I'm surprised the const_cast is required.