In RenderThemeQt.cpp there is this magic findFrameLineWidth() function, which queries the style for the frame line width: static int findFrameLineWidth(QStyle* style) { QLineEdit lineEdit; QStyleOptionFrameV2 opt; return style->pixelMetric(QStyle::PM_DefaultFrameWidth, &opt, &lineEdit); } It needs to provide the style with a widget so that the style will give the right width, but constructing it every time seems like an overkill. Making it static and keeping one instance in the memory should not hurt the memory consumption too much.
Created attachment 51163 [details] patch Additionally I included a very basic benchmark I used to measure the gain. I'm not sure if these results are trustworthy but they are the following. According to Valgrind, quite a bit of time is spent in QLineEditPrivate::init(). Before: ********* Start testing of tst_Painting ********* Config: Using QTest library 4.7.0, Qt 4.7.0 PASS : tst_Painting::initTestCase() RESULT : tst_Painting::textAreas(): 62.415 msecs per iteration (total: 62,416, iterations: 1000) PASS : tst_Painting::textAreas() PASS : tst_Painting::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped ********* Finished testing of tst_Painting ********* After: ********* Start testing of tst_Painting ********* Config: Using QTest library 4.7.0, Qt 4.7.0 PASS : tst_Painting::initTestCase() RESULT : tst_Painting::textAreas(): 46.996 msecs per iteration (total: 46,996, iterations: 1000) PASS : tst_Painting::textAreas() PASS : tst_Painting::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped ********* Finished testing of tst_Painting *********
Comment on attachment 51163 [details] patch Good catch Jakub!! It really sucks that we have to create a QLineEdit instance in the first place :-(, but I understand that this is a flaw in the QStyle API. Instead of creating a global QLineEdit that runs the risk of being destructed after the QApplication destructor, how about assocating it with the RenderTheme? Then it would be destroyed when the page dies. Alternatively you could _cache_ the frame width in the RenderThemeQt as a member variable and initialize it through a temporary QLineEdit instance the first time it's queried. It's not perfect as it wouldn't follow style changes, but it's a start :) I'm going to say r- because I believe we should destroy the QLineEdit instance earlier. But otherwise a great catch!
(In reply to comment #2) > (From update of attachment 51163 [details]) > Good catch Jakub!! > > It really sucks that we have to create a QLineEdit instance in the first place > :-(, but I understand that this is a flaw in the QStyle API. > > Instead of creating a global QLineEdit that runs the risk of being destructed > after the QApplication destructor, how about assocating it with the > RenderTheme? Then it would be destroyed when the page dies. With this patch, QLineEdit would not be destructed as such - DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not be called at the exit. > Alternatively you could _cache_ the frame width in the RenderThemeQt as a > member variable and initialize it through a temporary QLineEdit instance the > first time it's queried. Would this work with style changes during the application lifetime?
(In reply to comment #3) > Would this work with style changes during the application lifetime? Nevermind that. I just noticed your comment on that.
(In reply to comment #3) > > Instead of creating a global QLineEdit that runs the risk of being destructed > > after the QApplication destructor, how about assocating it with the > > RenderTheme? Then it would be destroyed when the page dies. > > With this patch, QLineEdit would not be destructed as such - > DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not > be called at the exit. True true, but that's evil, too, isn't it? :)
(In reply to comment #5) > (In reply to comment #3) > > > Instead of creating a global QLineEdit that runs the risk of being destructed > > > after the QApplication destructor, how about assocating it with the > > > RenderTheme? Then it would be destroyed when the page dies. > > > > With this patch, QLineEdit would not be destructed as such - > > DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not > > be called at the exit. > > True true, but that's evil, too, isn't it? :) Right. :) Another option would be to make the QApplication instance the parent of this object. This way we could avoid having one QLineEdit per page and could keep up with the style change. If that sounds reasonable, I'll try this tomorrow, otherwise I'll pick either of the ideas suggested by you.
Created attachment 51289 [details] patch OK, I went for having a QLineEdit per RenderTheme to simplify dealing with its lifetime. This could be further improved if needed.
Created attachment 51290 [details] patch Updated the ChangeLog in respect of the new approach.
Comment on attachment 51290 [details] patch Clearing flags on attachment: 51290 Committed r56343: <http://trac.webkit.org/changeset/56343>
All reviewed patches have been landed. Closing bug.
Cherry-picked into qtwebkit-4.6 with commit bd724fb2f716336a8a4b54cd2edc96851a5a26a4