[webkit-reviews] review granted: [Bug 36244] [Qt] QtLauncher's FPS info should not be displayed on the terminal : [Attachment 51349] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Jesus Sanchez-Palencia
<jesus.palencia at openbossa.org>'s request for review:
Bug 36244: [Qt] QtLauncher's FPS info should not be displayed on the terminal
https://bugs.webkit.org/show_bug.cgi?id=36244

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 27d7fec..2d508cc 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,23 @@
> +2010-03-22  Jesus Sanchez-Palencia  <jesus.palencia at openbossa.org>
> +
> +	   Not displaying FPS info on the terminal. On S60 and Maemo the
> +	   Window title will be used and Status bar will used on desktop.
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [Qt] QtLauncher's FPS info should not be displayed on the terminal
> +	   https://bugs.webkit.org/show_bug.cgi?id=36244

It looks like the order of these three paragraphs is the wrong way around? :)

> +    if (!enable) {
> +#ifndef Q_WS_MAEMO_5 && !defined(Q_WS_S60)
> +	   statusBar()->clearMessage();
> +#else
> +	   setWindowTitle("");
> +#endif

I think it's more readable to express the preprocessor condition using positive
defines, i.e.

#if defined(Q_WS_MAEMO_5) || defined(Q_WS_S60)
    use the window title
#else
    use the status bar
#endif

> +void LauncherWindow::updateFPS(int fps)
> +{
> +    QString fpsStatusText;
> +    QTextStream fpsTextStream(&fpsStatusText);
> +
> +    fpsTextStream << "Current FPS: " << fps;

Isn't using QString::arg simpler?

QString statusText = QString("Current FPS: %1").arg(fps);


> -    bool frameRateMeasurementEnabled() { return m_measureFps; } 
> +    bool frameRateMeasurementEnabled() { return m_measureFps; }

You could make this getter const while you're at it :)
  
I'm going to say r+, but I think the above changes would lead to more readable
code, so consider doing them before landing.