Bug 36244 – [Qt] QtLauncher's FPS info should not be displayed on the terminal

• Component: Tools / Tests
• Status: CLOSED FIXED

[Jesus Sanchez-Palencia]

[Qt] QtLauncher's FPS info should be displayed on an overlay text item

[Jesus Sanchez-Palencia]

Created attachment 50963 [details]
Patch

[Antonio Gomes]

(In reply to comment #1)
> Created an attachment (id=50963) [details]
> Patch

hi Jeez. In the patch, I see you new'ing, but not deleting it. Is that correct?

>  m_fpsTextItem = new QGraphicsTextItem("[FPS]", m_item);

[Jesus Sanchez-Palencia]

(In reply to comment #2)
> hi Jeez. In the patch, I see you new'ing, but not deleting it. Is that correct?
> 
> >  m_fpsTextItem = new QGraphicsTextItem("[FPS]", m_item);

It will be deleted when its parent (m_item) is.
Thanks for the comment. :)

[Jesus Sanchez-Palencia]

Created attachment 51062 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #4)
> Created an attachment (id=51062) [details]
> Patch

Now with a background and appearing on Launcher's bottom right corner.

[Kenneth Rohde Christiansen]

Comment on attachment 51062 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 51062 [details]
Patch

Clearing flags on attachment: 51062

Committed r56183: <http://trac.webkit.org/changeset/56183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Can we please re-consider this?

Adding an overlay item for an FPS counter is a pretty thing to have, but I have two concerns:

1) The overlay changes the way QtLauncher receives updates. It may have potential side-effects, which we don't want. QtLauncher should remain as pristine as possible when it comes to the way it tests WebKit behaviour.

2) The extra item is certainly not going to _improve_ the performance, instead I'm afraid it might actually product worse numbers.

I like having an FPS counter in QtLauncher, it's a useful tool to have. But printing the results in a place where it does not have any side-effects sounds like better way of implementing the feature - to me that means printing it on the terminal.

[zalan]

agree with Simon, for the same reason, in yberbrowser, the fps counter is placed on a toolbar instead of the webview area.

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> agree with Simon, for the same reason, in yberbrowser, the fps counter is
> placed on a toolbar instead of the webview area.

I actually had some similar concerns, but neglected them as my version of yberbrowser also painted it on screen (you must have changed that later then :)).

I suggest we just use the title bar / status bar for writing the FPS.

Feel free to roll out.

[Antonio Gomes]

(In reply to comment #11)
> (In reply to comment #10)
> > agree with Simon, for the same reason, in yberbrowser, the fps counter is
> > placed on a toolbar instead of the webview area.
> 
> I actually had some similar concerns, but neglected them as my version of
> yberbrowser also painted it on screen (you must have changed that later then
> :)).
> 
> I suggest we just use the title bar / status bar for writing the FPS.
> 
> Feel free to roll out.

If the final decision is to roll it out. I can help w/ that. I am heading back to office, and should be there in 30 min. Let me know, kenneth, pls.

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> I like having an FPS counter in QtLauncher, it's a useful tool to have. But
> printing the results in a place where it does not have any side-effects sounds
> like better way of implementing the feature - to me that means printing it on
> the terminal.

Yes, we had this in mind... We changed it just because of Symbian devices, where we don't have a terminal.


(In reply to comment #11)
> (In reply to comment #10)
> > agree with Simon, for the same reason, in yberbrowser, the fps counter is
> > placed on a toolbar instead of the webview area.
> 
> I actually had some similar concerns, but neglected them as my version of
> yberbrowser also painted it on screen (you must have changed that later then
> :)).

Yes, it was like this when I saw it!

> 
> I suggest we just use the title bar / status bar for writing the FPS.
> 
> Feel free to roll out.


Ok, I'll change the patch later.
I'll edit the bug's title since we are not going to use overlay text items anymore. 

Antonio, can you roll it out please?!

Thanks! :)

[Antonio Gomes]

> Antonio, can you roll it out please?!

patch rolled out in http://trac.webkit.org/changeset/56335

[Jesus Sanchez-Palencia]

Created attachment 51349 [details]
Patch

[Simon Hausmann]

Comment on attachment 51349 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 27d7fec..2d508cc 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,23 @@
> [phone]  Jesus Sanchez-Palencia  <[e-mail]>
> +
> +        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.

[Jesus Sanchez-Palencia]

Antonio landed the patch after I fixed what Simon asked. Commit r56553.

Closing the bug as fixed. Thanks!

[Simon Hausmann]

Landed in r56553

[Simon Hausmann]

Revision r56553 cherry-picked into qtwebkit-2.0 with commit 61b094cc1b2a0e2a15058e87f1c61230fd326c29

[Janne Koskinen]

Guys maybe this comes in a bit too late, but Symbian does have a terminal...