34874 2010-02-11 19:58:58 -0800 [Qt] Unnecessary QBrush construction for doing a solid color stroke 2010-02-12 19:07:57 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ariya.hidayat webkit-unassigned oldest_to_newest 189906 0 48614 ariya.hidayat 2010-02-11 19:58:58 -0800 Created attachment 48614 Patch Use the similar trick like in r54347, i.e. use the special brush for solid color to avoid expensive QBrush constructor. 189908 1 48616 ariya.hidayat 2010-02-11 20:07:11 -0800 Created attachment 48616 2010-02-11 Ariya Hidayat <ariya.hidayat@gmail.com> 190009 2 48616 kenneth 2010-02-12 04:31:50 -0800 Comment on attachment 48616 2010-02-11 Ariya Hidayat <ariya.hidayat@gmail.com> > +++ b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp > @@ -1112,7 +1112,8 @@ void GraphicsContext::setPlatformStrokeColor(const Color& color, ColorSpace colo > return; > QPainter* p = m_data->p(); > QPen newPen(p->pen()); > - newPen.setColor(color); > + m_data->solidColor.setColor(color); > + newPen.setBrush(m_data->solidColor); > p->setPen(newPen); > } > For me this code is not so clear, so maybe a comment would help? 190035 3 ariya.hidayat 2010-02-12 06:44:02 -0800 (In reply to comment #2) > (From update of attachment 48616 [details]) > > > +++ b/WebCore/platform/graphics/qt/GraphicsContextQt.cpp > > @@ -1112,7 +1112,8 @@ void GraphicsContext::setPlatformStrokeColor(const Color& color, ColorSpace colo > > return; > > QPainter* p = m_data->p(); > > QPen newPen(p->pen()); > > - newPen.setColor(color); > > + m_data->solidColor.setColor(color); > > + newPen.setBrush(m_data->solidColor); > > p->setPen(newPen); > > } > > > > For me this code is not so clear, so maybe a comment would help? This is common QBrush problem,please see http://trac.webkit.org/changeset/54347 which refers also to http://trac.webkit.org/changeset/37421. Basically it avoids QBrush::QBrush() because the said constructor is expensive, involving allocating QTransform from the heap, necessary for transformed gradient/pattern brush but completely useless for a solid color brush. So we keep our own brush for the solid color which we reuse for this purpose (just change the color, but not construct a new QBrush). 190059 4 48616 kenneth 2010-02-12 08:17:31 -0800 Comment on attachment 48616 2010-02-11 Ariya Hidayat <ariya.hidayat@gmail.com> I would have liked the comment in the actual code, or an inline method with a self described name 190231 5 ariya.hidayat 2010-02-12 19:07:57 -0800 (Added one line comment per Kenneth's request) Manually committed in r54746: http://trac.webkit.org/changeset/54746 48614 2010-02-11 19:58:58 -0800 2010-02-11 20:07:07 -0800 Patch 20100211195856.patch text/plain 582 ariya.hidayat ZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvR3JhcGhpY3NDb250ZXh0 UXQuY3BwIGIvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5j cHAKaW5kZXggMTA1ZDg2Ni4uMWI4NjgyYiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAKKysrIGIvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAKQEAgLTExMTIsNyArMTExMiw4IEBAIHZv aWQgR3JhcGhpY3NDb250ZXh0OjpzZXRQbGF0Zm9ybVN0cm9rZUNvbG9yKGNvbnN0IENvbG9yJiBj b2xvciwgQ29sb3JTcGFjZSBjb2xvCiAgICAgICAgIHJldHVybjsKICAgICBRUGFpbnRlciogcCA9 IG1fZGF0YS0+cCgpOwogICAgIFFQZW4gbmV3UGVuKHAtPnBlbigpKTsKLSAgICBuZXdQZW4uc2V0 Q29sb3IoY29sb3IpOworICAgIG1fZGF0YS0+c29saWRDb2xvci5zZXRDb2xvcihjb2xvcik7Cisg ICAgbmV3UGVuLnNldEJydXNoKG1fZGF0YS0+c29saWRDb2xvcik7CiAgICAgcC0+c2V0UGVuKG5l d1Blbik7CiB9CiAK 48616 2010-02-11 20:07:11 -0800 2010-02-12 08:17:31 -0800 2010-02-11 Ariya Hidayat <ariya.hidayat@gmail.com> bug-34874-20100211200710.patch text/plain 1307 ariya.hidayat ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZjNlNDFkMy4uNWMxMDkyZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAxMC0wMi0xMSAgQXJpeWEgSGlkYXlh dCAgPGFyaXlhLmhpZGF5YXRAZ21haWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gVW5uZWNlc3NhcnkgUUJydXNoIGNvbnN0cnVjdGlv biBmb3IgZG9pbmcgYSBzb2xpZCBjb2xvciBzdHJva2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM0ODc0CisKKyAgICAgICAgVXNlIHRoZSBzaW1pbGFy IHRyaWNrIGxpa2UgaW4gcjU0MzQ3LCBpLmUuIHVzZSB0aGUgc3BlY2lhbCBicnVzaCBmb3IKKyAg ICAgICAgc29saWQgY29sb3IgdG8gYXZvaWQgZXhwZW5zaXZlIFFCcnVzaCBjb25zdHJ1Y3Rvci4K KworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBoaWNzQ29udGV4dFF0LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkdyYXBoaWNzQ29udGV4dDo6c2V0UGxhdGZvcm1TdHJva2VDb2xv cik6CisKIDIwMTAtMDItMTEgIFBhdmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBjaHJvbWl1bS5vcmc+ CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgVGltb3RoeSBIYXRjaGVyLgpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAgYi9XZWJDb3Jl L3BsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBoaWNzQ29udGV4dFF0LmNwcAppbmRleCA3MzYwNjJj Li5kNjFmZTBkIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBo aWNzQ29udGV4dFF0LmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0L0dyYXBo aWNzQ29udGV4dFF0LmNwcApAQCAtMTExMiw3ICsxMTEyLDggQEAgdm9pZCBHcmFwaGljc0NvbnRl eHQ6OnNldFBsYXRmb3JtU3Ryb2tlQ29sb3IoY29uc3QgQ29sb3ImIGNvbG9yLCBDb2xvclNwYWNl IGNvbG8KICAgICAgICAgcmV0dXJuOwogICAgIFFQYWludGVyKiBwID0gbV9kYXRhLT5wKCk7CiAg ICAgUVBlbiBuZXdQZW4ocC0+cGVuKCkpOwotICAgIG5ld1Blbi5zZXRDb2xvcihjb2xvcik7Cisg ICAgbV9kYXRhLT5zb2xpZENvbG9yLnNldENvbG9yKGNvbG9yKTsKKyAgICBuZXdQZW4uc2V0QnJ1 c2gobV9kYXRhLT5zb2xpZENvbG9yKTsKICAgICBwLT5zZXRQZW4obmV3UGVuKTsKIH0KIAo=