Bug 36244

Summary: [Qt] QtLauncher's FPS info should not be displayed on the terminal
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: Tools / TestsAssignee: Jesus Sanchez-Palencia <jesus>
Status: CLOSED FIXED    
Severity: Normal CC: benjamin, commit-queue, hausmann, kenneth, koshuin, tonikitoo, zalan
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 35303, 35784    
Attachments:
Description Flags
Patch
none
Patch
none
Patch hausmann: review+, hausmann: commit-queue-

Description Jesus Sanchez-Palencia 2010-03-17 15:02:18 PDT
[Qt] QtLauncher's FPS info should be displayed on an overlay text item
Comment 1 Jesus Sanchez-Palencia 2010-03-17 15:04:09 PDT
Created attachment 50963 [details]
Patch
Comment 2 Antonio Gomes 2010-03-17 20:23:17 PDT
(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);
Comment 3 Jesus Sanchez-Palencia 2010-03-18 07:05:47 PDT
(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. :)
Comment 4 Jesus Sanchez-Palencia 2010-03-18 11:29:42 PDT
Created attachment 51062 [details]
Patch
Comment 5 Jesus Sanchez-Palencia 2010-03-18 11:33:49 PDT
(In reply to comment #4)
> Created an attachment (id=51062) [details]
> Patch

Now with a background and appearing on Launcher's bottom right corner.
Comment 6 Kenneth Rohde Christiansen 2010-03-18 11:38:36 PDT
Comment on attachment 51062 [details]
Patch

LGTM
Comment 7 WebKit Commit Bot 2010-03-18 13:07:20 PDT
Comment on attachment 51062 [details]
Patch

Clearing flags on attachment: 51062

Committed r56183: <http://trac.webkit.org/changeset/56183>
Comment 8 WebKit Commit Bot 2010-03-18 13:07:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Simon Hausmann 2010-03-19 02:58:31 PDT
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.
Comment 10 zalan 2010-03-19 03:03:53 PDT
agree with Simon, for the same reason, in yberbrowser, the fps counter is placed on a toolbar instead of the webview area.
Comment 11 Kenneth Rohde Christiansen 2010-03-19 10:17:37 PDT
(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.
Comment 12 Antonio Gomes 2010-03-19 10:29:04 PDT
(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.
Comment 13 Jesus Sanchez-Palencia 2010-03-22 06:05:42 PDT
(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! :)
Comment 14 Antonio Gomes 2010-03-22 08:28:10 PDT
> Antonio, can you roll it out please?!

patch rolled out in http://trac.webkit.org/changeset/56335
Comment 15 Jesus Sanchez-Palencia 2010-03-22 14:01:15 PDT
Created attachment 51349 [details]
Patch
Comment 16 Simon Hausmann 2010-03-25 07:21:17 PDT
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 @@
> +2010-03-22  Jesus Sanchez-Palencia  <jesus.palencia@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.
Comment 17 Jesus Sanchez-Palencia 2010-03-25 10:29:00 PDT
Antonio landed the patch after I fixed what Simon asked. Commit r56553.

Closing the bug as fixed. Thanks!
Comment 18 Simon Hausmann 2010-03-26 05:15:25 PDT
Landed in r56553
Comment 19 Simon Hausmann 2010-03-26 07:04:06 PDT
Revision r56553 cherry-picked into qtwebkit-2.0 with commit 61b094cc1b2a0e2a15058e87f1c61230fd326c29
Comment 20 Janne Koskinen 2010-04-12 01:29:32 PDT
Guys maybe this comes in a bit too late, but Symbian does have a terminal...