Bug 57068

Created attachment 87634 [details] Patch

Created attachment 88932 [details] Patch for ToT

Attachment 88932 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Tools/WebKitTestRunner/gtk/main.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:51: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Adding myself to the cc list

Comment on attachment 88932 [details] Patch for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=88932&action=review > Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:52 > + g_object_set(settings, "gtk-xft-rgba", "none", NULL); While calling g_object_set() again? you can just add "gtk-xft-rgba", "none" to the first g_object_set call. > Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:176 > + inititializeFontConfigSetting(); isn't this function X11 specific?

Comment on attachment 88932 [details] Patch for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=88932&action=review >> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:52 >> + g_object_set(settings, "gtk-xft-rgba", "none", NULL); > > While calling g_object_set() again? you can just add "gtk-xft-rgba", "none" to the first g_object_set call. Ah, right, this should move up into the previous block. For a particular test it needs to move, but I'm not sure if it's possible to get the test name at this point in the file. >> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:176 >> + inititializeFontConfigSetting(); > > isn't this function X11 specific? It's specific to ports that use FontConfig. As far as I know we are phasing out non-FontConfig builds.

Created attachment 89039 [details] Patch taking Carlos' suggestion

Attachment 89039 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Tools/WebKitTestRunner/gtk/main.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:52: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

View in context: https://bugs.webkit.org/attachment.cgi?id=88932&action=review Great Patch :) > Tools/WebKitTestRunner/GNUmakefile.am:24 > + -include Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h \ Are these Prefix headers now required to be included in makefiles since we are including config.h anyways in all files as first header? > Tools/WebKitTestRunner/GNUmakefile.am:44 > + $(WINMM_LIBS) Why do we require WINMM libs for GTK port? > Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:114 > + }; Instead of listing all fonts, can we use the env variable WEBKIT_TEST_FONTS, which is used in DRT as well as other WebKit2 ports? It simplifies the code here a lot. > Tools/WebKitTestRunner/InjectedBundle/gtk/LayoutTestControllerGtk.cpp:35 > +static gboolean waitToDumpWatchdogTimerIntervalCallback(gpointer) Can we name it as waitToDumpWatchdogTimerFired() itself? > Tools/WebKitTestRunner/InjectedBundle/gtk/LayoutTestControllerGtk.cpp:38 > + return TRUE; Shouldn't this be FALSE? Who will cancel it since it will be firing continuously? > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:43 > + gtk_widget_size_allocate(m_window, &size); Why are we not using gtk_widget_set_size_request() since it does both window_resize & size_allocate? > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:60 > + Ditto. > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:65 > + cancelTimeout(); Shouldn't the cancelTimeout() here based on the bool condition sent in platformRunUntil()? All other ports run until the bool goes false on which they cancel the timer. Isn't it so?

Comment on attachment 89039 [details] Patch taking Carlos' suggestion Looks good, still checking what happens and why it stalls when starting to test, but somme comments already. View in context: https://bugs.webkit.org/attachment.cgi?id=89039&action=review > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:-124 > -bool WebPage::platformCanHandleRequest(const WebCore::ResourceRequest&) > -{ > - // FIXME: Implement > - return true; > -} > - Why we should remove this function? > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:79 > + gint depth; > + gdk_window_get_geometry(gtk_widget_get_window(GTK_WIDGET(m_window)), > + &geometry.x, &geometry.y, &geometry.width, &geometry.height, &depth); Apparently gdk_window_get_geomtry does not include depth as a parameter in gtk+3.

Created attachment 89315 [details] Updated patch

Attachment 89315 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Tools/WebKitTestRunner/gtk/main.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:52: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #9) > View in context: https://bugs.webkit.org/attachment.cgi?id=88932&action=review Thanks for the comments! > > Tools/WebKitTestRunner/GNUmakefile.am:24 > > + -include Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h \ > Are these Prefix headers now required to be included in makefiles since we are including config.h anyways in all files as first header? I think it's good to set up the prefix header even if it isn't strictly necessary now. It might be useful later and we want the ports to be as similar as possible. > > Tools/WebKitTestRunner/GNUmakefile.am:44 > > + $(WINMM_LIBS) > Why do we require WINMM libs for GTK port? This is required for GTK+ builds on Windows. Perhaps this could be moved to the library itself, but that should happen in another patch. > > Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:114 > > + }; > Instead of listing all fonts, can we use the env variable WEBKIT_TEST_FONTS, which is used in DRT as well as other WebKit2 ports? It simplifies the code here a lot. It's possible that we could move some of this to a helper function and compile it into both the DRT and the WKTR, but some of it must still be different. I'd prefer to have as much of the logic be in the harness rather than the harness-script (old-run-webkit-tests), since we have to write it twice (for new-run-webkit-tests as well). > > Tools/WebKitTestRunner/InjectedBundle/gtk/LayoutTestControllerGtk.cpp:35 > > +static gboolean waitToDumpWatchdogTimerIntervalCallback(gpointer) > Can we name it as waitToDumpWatchdogTimerFired() itself? I've renamed this to waitToDumpWatchdogTimerCallback to simplify things. I prefer having the "Callback" suffix on GSignal callbacks to be more precise. :) > > Tools/WebKitTestRunner/InjectedBundle/gtk/LayoutTestControllerGtk.cpp:38 > > + return TRUE; > Shouldn't this be FALSE? Who will cancel it since it will be firing continuously? It should! Fixed. > > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:43 > > + gtk_widget_size_allocate(m_window, &size); > Why are we not using gtk_widget_set_size_request() since it does both window_resize & size_allocate? The documentation for gtk_widget_set_size_request says "Sets the minimum size of a widget; that is, the widget's size request will be width by height. " > > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:65 > > + cancelTimeout(); > Shouldn't the cancelTimeout() here based on the bool condition sent in platformRunUntil()? All other ports run until the bool goes false on which they cancel the timer. Isn't it so? The Qt port ignores the boolean parameter as well. I think it's just a nicety for ports that work more like the Mac port where the main loop runs directly in the platformRunUntil method.

(In reply to comment #10) > (From update of attachment 89039 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89039&action=review Thanks for your comments! > > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:-124 > > -bool WebPage::platformCanHandleRequest(const WebCore::ResourceRequest&) > > -{ > > - // FIXME: Implement > > - return true; > > -} > > - > > Why we should remove this function? Sorry, this snuck in from a previous version of the patch. I've removed this change. > > > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:79 > > + gint depth; > > + gdk_window_get_geometry(gtk_widget_get_window(GTK_WIDGET(m_window)), > > + &geometry.x, &geometry.y, &geometry.width, &geometry.height, &depth); > > Apparently gdk_window_get_geomtry does not include depth as a parameter in gtk+3. I've #ifdeffed this section for GTK+ 3.x. Nice find.

View in context: https://bugs.webkit.org/attachment.cgi?id=89315&action=review Looks all set for r+. Just couple of minor comments. > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:61 > +typedef ::GModule* PlatformBundle; Can't we define both the typedef here itself instead of 2 separate blocks? > Tools/WebKitTestRunner/gtk/TestInvocationGtk.cpp:32 > +{ I guess the argument should be WKImageRef. :)

Created attachment 90371 [details] Patch using WKImageRef

(In reply to comment #15) > View in context: https://bugs.webkit.org/attachment.cgi?id=89315&action=review > > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:61 > > +typedef ::GModule* PlatformBundle; > Can't we define both the typedef here itself instead of 2 separate blocks? I attempted this, but GCC was unhappy with it. > > Tools/WebKitTestRunner/gtk/TestInvocationGtk.cpp:32 > > +{ > I guess the argument should be WKImageRef. :) Fixed!

Attachment 90371 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:52: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

r+ for me. :) (In reply to comment #17) > (In reply to comment #15) > > View in context: https://bugs.webkit.org/attachment.cgi?id=89315&action=review > > > > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:61 > > > +typedef ::GModule* PlatformBundle; > > Can't we define both the typedef here itself instead of 2 separate blocks? > > I attempted this, but GCC was unhappy with it. > > > > Tools/WebKitTestRunner/gtk/TestInvocationGtk.cpp:32 > > > +{ > > I guess the argument should be WKImageRef. :) > > Fixed!

Comment on attachment 90371 [details] Patch using WKImageRef 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.

Created attachment 91330 [details] Patch

Attachment 91330 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:51: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #20) Thank you very much for the review! I've attached an updated patch. > (From update of attachment 90371 [details]) > 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. Sadly, it is. Attempts to remove it have been unsuccessful. > > 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? Closing the module will make the symbols unavailable. It stays open much like the Mac implementation. It seems that the lifetime of InjectedBundle is for the extent of the WebProcess? > > 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. Fixed! > Do you want to modify readSkippedFiles, too, to fall back to mac-wk2 like win-wk2 and qt-wk2 do? I think we are still exploring how our test situation looks. Hopefully we can share the skipped list in the future like the Qt port. > > 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. I added because the old-run-webkit-tests script executes run-launcher to show the results. Right now this produces an error because we build WebKit1 separately. In the future we'd like to build them together, so maybe this change is not needed in the end. It can certainly be in a separate patch though. I've removed it. > > Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:43 > > +{ > > + > > + GtkSettings* settings = gtk_settings_get_default(); > Extra newline in here. Fixed! > > 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? It's actually non-trivial to find the binary path in a cross-platform way. We've punted on this, much like Qt. > I think you're leaking m_injectedBundlePath. You're right! I've fixed it in my patch and I will try to write a patch to fix the Qt code. > > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:86 > > + // This is called after initializeInjectedBundlePath. > > + m_testPluginDirectory = m_injectedBundlePath; > You could assert that m_injectedBundlePath is initialized. Done.

Comment on attachment 91330 [details] Patch 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.

(In reply to comment #24) > (From update of attachment 91330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91330&action=review Thanks for the 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) {" Okay. I've fixed this. > > > Tools/Scripts/webkitdirs.pm:639 > > + if (-e $library) { > > + return $library; > > + } > > I'd write this as a one-liner: return $library if -e $library; Fixed. > > > 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; Fixed. Note that this bug exists for the Mac port too. > > 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. Fixed. > > 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). Fixed. > > Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:39 > > +} > > + > > + > > +void InjectedBundle::platformInitialize(WKTypeRef) > > Extra newline in here. Fixed. > > 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.) Fixed. Nice catch. > > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:52 > > + gtk_widget_destroy(m_window); > > Will this destroy m_view, too? Yep! > > Tools/WebKitTestRunner/gtk/main.cpp:33 > > + WTR::TestController controller(argc, const_cast<const char**>(argv)); > > I'm surprised the const_cast is required. Yep, the compile fails without it. :( ../../Tools/WebKitTestRunner/gtk/main.cpp: In function ‘int main(int, char**)’: ../../Tools/WebKitTestRunner/gtk/main.cpp:34:46: error: invalid conversion from ‘char**’ to ‘const char**’ ../../Tools/WebKitTestRunner/gtk/main.cpp:34:46: error: initializing argument 2 of ‘WTR::TestController::TestController(int, const char**)’

Committed r87760: <http://trac.webkit.org/changeset/87760>