43416 2010-08-03 06:09:48 -0700 [Qt] Implement GraphicsContext::clipOut more efficiently 2010-08-18 22:20:51 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) All All RESOLVED FIXED Performance, Qt, QtTriaged P3 Normal --- 1 hausmann hausmann ariya.hidayat benjamin commit-queue kling oldest_to_newest 259413 0 hausmann 2010-08-03 06:09:48 -0700 The current implementation of the function uses QPainter::clipRect().boundingRect(), which is very slow and with complex content it can make text selection very slow or even cause crashes in Qt due to complex regions. http://bugreports.qt.nokia.com/browse/QTBUG-12618 in Qt tracks adding support for a faster method in Qt that would allow to quickly retrieve the clip bounding rect in device coordinates. Once that function is implemented, then we can use it in GraphicsContexQt and transform the return value into logical coordinates. 263178 1 64096 hausmann 2010-08-11 04:50:33 -0700 Created attachment 64096 Patch 263184 2 64096 ariya.hidayat 2010-08-11 04:55:57 -0700 Comment on attachment 64096 Patch > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index efc27d6e5a204eaeba7775d0a099627bc29adaaf..6b790604d12511157df404aeaad32e0b434add46 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,21 @@ > +2010-08-11 Simon Hausmann <simon.hausmann@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Implement GraphicsContext::clipOut more efficiently > + https://bugs.webkit.org/show_bug.cgi?id=43416 > + > + The current implementation of clipOut uses QPainter::clipRegion().boundingRect(), > + which is a very slow function because it converts the entire clip region - which > + may potentially contain a complex path - into a set of QRegion rectangles, just > + to throw away all that information and use the bounding rect. > + > + QTBUG-12618 implements a faster function in QPainter: QPainter::clipBoundingRect(). > + > + * platform/graphics/qt/GraphicsContextQt.cpp: > + (WebCore::GraphicsContext::clipOut): Use QPainter::clipBoundingRect() with Qt >= 4.8. > + (WebCore::GraphicsContext::clipOutEllipseInRect): Ditto. > + > 2010-08-11 Adam Barth <abarth@webkit.org> > > Reviewed by Eric Seidel. > diff --git a/WebCore/platform/graphics/qt/GraphicsContextQt.cpp b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp > index d4a145f9f1e875c4bf012fd24fba24d5c1771257..43d0f7eb66e69240312d6e6a7c4a902619a6125a 100644 > --- a/WebCore/platform/graphics/qt/GraphicsContextQt.cpp > +++ b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp > @@ -1121,7 +1121,11 @@ void GraphicsContext::clipOut(const Path& path) > QPainterPath newClip; > newClip.setFillRule(Qt::OddEvenFill); > if (p->hasClipping()) { > +#if QT_VERSION >= QT_VERSION_CHECK(4, 8, 0) > + newClip.addRect(p->clipBoundingRect()); > +#else > newClip.addRect(p->clipRegion().boundingRect()); > +#endif > newClip.addPath(clippedOut); > p->setClipPath(newClip, Qt::IntersectClip); > } else { > @@ -1191,7 +1195,11 @@ void GraphicsContext::clipOut(const IntRect& rect) > QPainterPath newClip; > newClip.setFillRule(Qt::OddEvenFill); > if (p->hasClipping()) { > +#if QT_VERSION >= QT_VERSION_CHECK(4, 8, 0) > + newClip.addRect(p->clipBoundingRect()); > +#else > newClip.addRect(p->clipRegion().boundingRect()); > +#endif > newClip.addRect(QRect(rect)); > p->setClipPath(newClip, Qt::IntersectClip); > } else { > @@ -1213,7 +1221,11 @@ void GraphicsContext::clipOutEllipseInRect(const IntRect& rect) > QPainterPath newClip; > newClip.setFillRule(Qt::OddEvenFill); > if (p->hasClipping()) { > +#if QT_VERSION >= QT_VERSION_CHECK(4, 8, 0) > + newClip.addRect(p->clipBoundingRect()); > +#else > newClip.addRect(p->clipRegion().boundingRect()); > +#endif > newClip.addEllipse(QRect(rect)); > p->setClipPath(newClip, Qt::IntersectClip); > } else { 266527 3 64096 kling 2010-08-18 20:36:50 -0700 Comment on attachment 64096 Patch Great stuff :) Setting cq+ since Simon is traveling this week. 266544 4 64096 commit-queue 2010-08-18 22:20:47 -0700 Comment on attachment 64096 Patch Clearing flags on attachment: 64096 Committed r65650: <http://trac.webkit.org/changeset/65650> 266545 5 commit-queue 2010-08-18 22:20:51 -0700 All reviewed patches have been landed. Closing bug. 64096 2010-08-11 04:50:33 -0700 2010-08-18 22:20:46 -0700 Patch bug-43416-20100811135031.patch text/plain 2846 hausmann ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZWZjMjdkNmU1YTIwNGVhZWJhNzc3NWQwYTA5OTYyN2JjMjlhZGFhZi4uNmI3OTA2MDRkMTI1MTEx NTdkZjQwNGFlYWFkMzJlMGI0MzRhZGQ0NiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMSBAQAorMjAxMC0wOC0xMSAgU2lt b24gSGF1c21hbm4gIDxzaW1vbi5oYXVzbWFubkBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1F0XSBJbXBsZW1lbnQgR3JhcGhpY3ND b250ZXh0OjpjbGlwT3V0IG1vcmUgZWZmaWNpZW50bHkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQzNDE2CisKKyAgICAgICAgVGhlIGN1cnJlbnQgaW1w bGVtZW50YXRpb24gb2YgY2xpcE91dCB1c2VzIFFQYWludGVyOjpjbGlwUmVnaW9uKCkuYm91bmRp bmdSZWN0KCksCisgICAgICAgIHdoaWNoIGlzIGEgdmVyeSBzbG93IGZ1bmN0aW9uIGJlY2F1c2Ug aXQgY29udmVydHMgdGhlIGVudGlyZSBjbGlwIHJlZ2lvbiAtIHdoaWNoCisgICAgICAgIG1heSBw b3RlbnRpYWxseSBjb250YWluIGEgY29tcGxleCBwYXRoIC0gaW50byBhIHNldCBvZiBRUmVnaW9u IHJlY3RhbmdsZXMsIGp1c3QKKyAgICAgICAgdG8gdGhyb3cgYXdheSBhbGwgdGhhdCBpbmZvcm1h dGlvbiBhbmQgdXNlIHRoZSBib3VuZGluZyByZWN0LgorCisgICAgICAgIFFUQlVHLTEyNjE4IGlt cGxlbWVudHMgYSBmYXN0ZXIgZnVuY3Rpb24gaW4gUVBhaW50ZXI6IFFQYWludGVyOjpjbGlwQm91 bmRpbmdSZWN0KCkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0Nv bnRleHRRdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpHcmFwaGljc0NvbnRleHQ6OmNsaXBPdXQp OiBVc2UgUVBhaW50ZXI6OmNsaXBCb3VuZGluZ1JlY3QoKSB3aXRoIFF0ID49IDQuOC4KKyAgICAg ICAgKFdlYkNvcmU6OkdyYXBoaWNzQ29udGV4dDo6Y2xpcE91dEVsbGlwc2VJblJlY3QpOiBEaXR0 by4KKwogMjAxMC0wOC0xMSAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IEVyaWMgU2VpZGVsLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9y bS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAgYi9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL3F0L0dyYXBoaWNzQ29udGV4dFF0LmNwcAppbmRleCBkNGExNDVmOWYxZTg3NWM0YmYw MTJmZDI0ZmJhMjRkNWMxNzcxMjU3Li40M2QwZjdlYjY2ZTY5MjQwMzEyZDZlNmE3YzRhOTAyNjE5 YTYxMjVhIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBoaWNz Q29udGV4dFF0LmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBoaWNz Q29udGV4dFF0LmNwcApAQCAtMTEyMSw3ICsxMTIxLDExIEBAIHZvaWQgR3JhcGhpY3NDb250ZXh0 OjpjbGlwT3V0KGNvbnN0IFBhdGgmIHBhdGgpCiAgICAgUVBhaW50ZXJQYXRoIG5ld0NsaXA7CiAg ICAgbmV3Q2xpcC5zZXRGaWxsUnVsZShRdDo6T2RkRXZlbkZpbGwpOwogICAgIGlmIChwLT5oYXND bGlwcGluZygpKSB7CisjaWYgUVRfVkVSU0lPTiA+PSBRVF9WRVJTSU9OX0NIRUNLKDQsIDgsIDAp CisgICAgICAgIG5ld0NsaXAuYWRkUmVjdChwLT5jbGlwQm91bmRpbmdSZWN0KCkpOworI2Vsc2UK ICAgICAgICAgbmV3Q2xpcC5hZGRSZWN0KHAtPmNsaXBSZWdpb24oKS5ib3VuZGluZ1JlY3QoKSk7 CisjZW5kaWYKICAgICAgICAgbmV3Q2xpcC5hZGRQYXRoKGNsaXBwZWRPdXQpOwogICAgICAgICBw LT5zZXRDbGlwUGF0aChuZXdDbGlwLCBRdDo6SW50ZXJzZWN0Q2xpcCk7CiAgICAgfSBlbHNlIHsK QEAgLTExOTEsNyArMTE5NSwxMSBAQCB2b2lkIEdyYXBoaWNzQ29udGV4dDo6Y2xpcE91dChjb25z dCBJbnRSZWN0JiByZWN0KQogICAgIFFQYWludGVyUGF0aCBuZXdDbGlwOwogICAgIG5ld0NsaXAu c2V0RmlsbFJ1bGUoUXQ6Ok9kZEV2ZW5GaWxsKTsKICAgICBpZiAocC0+aGFzQ2xpcHBpbmcoKSkg eworI2lmIFFUX1ZFUlNJT04gPj0gUVRfVkVSU0lPTl9DSEVDSyg0LCA4LCAwKQorICAgICAgICBu ZXdDbGlwLmFkZFJlY3QocC0+Y2xpcEJvdW5kaW5nUmVjdCgpKTsKKyNlbHNlCiAgICAgICAgIG5l d0NsaXAuYWRkUmVjdChwLT5jbGlwUmVnaW9uKCkuYm91bmRpbmdSZWN0KCkpOworI2VuZGlmCiAg ICAgICAgIG5ld0NsaXAuYWRkUmVjdChRUmVjdChyZWN0KSk7CiAgICAgICAgIHAtPnNldENsaXBQ YXRoKG5ld0NsaXAsIFF0OjpJbnRlcnNlY3RDbGlwKTsKICAgICB9IGVsc2UgewpAQCAtMTIxMyw3 ICsxMjIxLDExIEBAIHZvaWQgR3JhcGhpY3NDb250ZXh0OjpjbGlwT3V0RWxsaXBzZUluUmVjdChj b25zdCBJbnRSZWN0JiByZWN0KQogICAgIFFQYWludGVyUGF0aCBuZXdDbGlwOwogICAgIG5ld0Ns aXAuc2V0RmlsbFJ1bGUoUXQ6Ok9kZEV2ZW5GaWxsKTsKICAgICBpZiAocC0+aGFzQ2xpcHBpbmco KSkgeworI2lmIFFUX1ZFUlNJT04gPj0gUVRfVkVSU0lPTl9DSEVDSyg0LCA4LCAwKQorICAgICAg ICBuZXdDbGlwLmFkZFJlY3QocC0+Y2xpcEJvdW5kaW5nUmVjdCgpKTsKKyNlbHNlCiAgICAgICAg IG5ld0NsaXAuYWRkUmVjdChwLT5jbGlwUmVnaW9uKCkuYm91bmRpbmdSZWN0KCkpOworI2VuZGlm CiAgICAgICAgIG5ld0NsaXAuYWRkRWxsaXBzZShRUmVjdChyZWN0KSk7CiAgICAgICAgIHAtPnNl dENsaXBQYXRoKG5ld0NsaXAsIFF0OjpJbnRlcnNlY3RDbGlwKTsKICAgICB9IGVsc2Ugewo=