[webkit-reviews] review denied: [Bug 57068] [GTK] [WebKit2] Implement a basic WebKitTestRunner : [Attachment 90371] Patch using WKImageRef

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied 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 90371: Patch using WKImageRef
https://bugs.webkit.org/attachment.cgi?id=90371&action=review

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:61
> -typedef void* PlatformBundle;
> +typedef ::GModule* PlatformBundle;

I'm surprised the :: is needed.

> Source/WebKit2/WebProcess/InjectedBundle/gtk/InjectedBundleGtk.cpp:41
> +    m_platformBundle =
g_module_open(fileSystemRepresentation(m_path).data(), G_MODULE_BIND_LOCAL);

Do we need to close this ever?

> Tools/Scripts/old-run-webkit-tests:1710
> +    } elsif (isGtk()) {
> +	   push @extraPlatforms, "gtk" if $platform eq "gtk-wk2";
>      }

This doesn't look right. You should modify buildPlatformHierarchy instead,
similar to what is done for win-wk2 and mac-wk2.

Do you want to modify readSkippedFiles, too, to fall back to mac-wk2 like
win-wk2 and qt-wk2 do?

> Tools/Scripts/run-launcher:73
>      if (isGtk()) {
> -	   $launcherPath = catdir($launcherPath, "Programs", "GtkLauncher");
> +	   if (isWK2()) {
> +	       $launcherPath = catdir($launcherPath, "Programs",
"MiniBrowser");
> +	   } else {
> +	       $launcherPath = catdir($launcherPath, "Programs",
"GtkLauncher");
> +	   }
>      }

This seems somewhat orthogonal.

> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:43
> +{
> +
> +    GtkSettings* settings = gtk_settings_get_default();

Extra newline in here.

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:80
> +    const char* bundlePath =
g_getenv("TEST_RUNNER_INJECTED_BUNDLE_FILENAME");
> +    if (!bundlePath) {
> +	   fprintf(stderr, "TEST_RUNNER_INJECTED_BUNDLE_FILENAME environment
variable not found\n");
> +	   exit(0);
> +    }
> +
> +    gsize bytesWritten;
> +    GOwnPtr<char> utf8BundlePath(g_filename_to_utf8(bundlePath, -1, 0,
&bytesWritten, 0));
> +    m_injectedBundlePath =
WKStringCreateWithUTF8CString(utf8BundlePath.get());

Is it not possible to find the injected bundle using a path relative to the
test runner?

I think you're leaking m_injectedBundlePath.

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:86
> +    // This is called after initializeInjectedBundlePath.
> +    m_testPluginDirectory = m_injectedBundlePath;

You could assert that m_injectedBundlePath is initialized.