WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
36373
[Qt] Don't construct a QLineEdit every time when painting a text field
https://bugs.webkit.org/show_bug.cgi?id=36373
Summary
[Qt] Don't construct a QLineEdit every time when painting a text field
Jakub Wieczorek
Reported
2010-03-19 09:40:25 PDT
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.
Attachments
patch
(3.11 KB, patch)
2010-03-19 09:52 PDT
,
Jakub Wieczorek
hausmann
: review-
Details
Formatted Diff
Diff
patch
(4.50 KB, patch)
2010-03-22 08:18 PDT
,
Jakub Wieczorek
no flags
Details
Formatted Diff
Diff
patch
(4.50 KB, patch)
2010-03-22 08:21 PDT
,
Jakub Wieczorek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jakub Wieczorek
Comment 1
2010-03-19 09:52:41 PDT
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 *********
Simon Hausmann
Comment 2
2010-03-21 15:40:43 PDT
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!
Jakub Wieczorek
Comment 3
2010-03-21 15:48:33 PDT
(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?
Jakub Wieczorek
Comment 4
2010-03-21 15:49:18 PDT
(In reply to
comment #3
)
> Would this work with style changes during the application lifetime?
Nevermind that. I just noticed your comment on that.
Simon Hausmann
Comment 5
2010-03-21 16:09:17 PDT
(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? :)
Jakub Wieczorek
Comment 6
2010-03-21 16:37:06 PDT
(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.
Jakub Wieczorek
Comment 7
2010-03-22 08:18:34 PDT
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.
Jakub Wieczorek
Comment 8
2010-03-22 08:21:13 PDT
Created
attachment 51290
[details]
patch Updated the ChangeLog in respect of the new approach.
Jakub Wieczorek
Comment 9
2010-03-22 10:42:55 PDT
Comment on
attachment 51290
[details]
patch Clearing flags on attachment: 51290 Committed
r56343
: <
http://trac.webkit.org/changeset/56343
>
Jakub Wieczorek
Comment 10
2010-03-22 10:43:04 PDT
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 11
2010-03-29 04:06:12 PDT
Cherry-picked into qtwebkit-4.6 with commit bd724fb2f716336a8a4b54cd2edc96851a5a26a4
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug