[Webkit-unassigned] [Bug 57068] [GTK] [WebKit2] Implement a basic WebKitTestRunner

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57068





--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2011-04-12 17:41:37 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.