[webkit-reviews] review denied: [Bug 22037] QtLauncher robotization for testing purposes : [Attachment 24844] proposed patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Kelemen Balázs
<Kelemen.Balazs.3 at stud.u-szeged.hu>'s request for review:
Bug 22037: QtLauncher robotization for testing purposes
https://bugs.webkit.org/show_bug.cgi?id=22037

Attachment 24844: proposed patch
https://bugs.webkit.org/attachment.cgi?id=24844&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 37444)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> +2008-10-09  System User  <set EMAIL_ADDRESS environment variable>

Please add your real name and email address.

> --- WebKit/qt/QtLauncher/QtLauncher.pro	(revision 37432)
> +++ WebKit/qt/QtLauncher/QtLauncher.pro	(working copy)
> @@ -3,6 +3,9 @@ SOURCES += main.cpp
>  CONFIG -= app_bundle
>  CONFIG += uitools
>  DESTDIR = ../../../bin
> +INCPATH += $$PWD/../../../JavaScriptCore/wtf
> +LIBS += -L../../../JavaScriptCore/ 
> +LIBS += -lJavaScriptCore

I don't think we should use the WTF types outside of WebCore, so this should
not be necessary.

> +public slots:
> +    void loadNext()
> +    {
> +	   QString qstr;
> +	   if (getUrl(qstr)) {
> +	       QUrl url(qstr, QUrl::StrictMode);
> +	       if (url.isValid()) {
> +		   m_stdOut<<"Loading "<<qstr<<" ......"<<endl;

Coding style, please put spaces between the arguments.

> +private:
> +    WTF::Vector<QString> m_urls;

I suggest the use of QStringList instead. This way we do not have to have to
pull in WTF or the static JavaScriptCore library.

The rest of the patch looks good, heads up :)