[Qt] QtLauncher's FPS info should be displayed on an overlay text item
Created attachment 50963 [details] Patch
(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);
(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. :)
Created attachment 51062 [details] Patch
(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 on attachment 51062 [details] Patch LGTM
Comment on attachment 51062 [details] Patch Clearing flags on attachment: 51062 Committed r56183: <http://trac.webkit.org/changeset/56183>
All reviewed patches have been landed. Closing bug.
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.
agree with Simon, for the same reason, in yberbrowser, the fps counter is placed on a toolbar instead of the webview area.
(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.
(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.
(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, can you roll it out please?! patch rolled out in http://trac.webkit.org/changeset/56335
Created attachment 51349 [details] Patch
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.
Antonio landed the patch after I fixed what Simon asked. Commit r56553. Closing the bug as fixed. Thanks!
Landed in r56553
Revision r56553 cherry-picked into qtwebkit-2.0 with commit 61b094cc1b2a0e2a15058e87f1c61230fd326c29
Guys maybe this comes in a bit too late, but Symbian does have a terminal...