[Qt] Don't construct a QLineEdit every time when painting a text field

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

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 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]

(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]

(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]

(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]

(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]

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]

Created attachment 51290 [details]
patch

Updated the ChangeLog in respect of the new approach.

[Jakub Wieczorek]

Comment on attachment 51290 [details]
patch

Clearing flags on attachment: 51290

Committed r56343: <http://trac.webkit.org/changeset/56343>

[Jakub Wieczorek]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Cherry-picked into qtwebkit-4.6 with commit bd724fb2f716336a8a4b54cd2edc96851a5a26a4