Bug 34874 - [Qt] Unnecessary QBrush construction for doing a solid color stroke
Summary: [Qt] Unnecessary QBrush construction for doing a solid color stroke
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-11 19:58 PST by Ariya Hidayat
Modified: 2010-02-12 19:07 PST (History)
0 users

See Also:


Attachments
Patch (582 bytes, patch)
2010-02-11 19:58 PST, Ariya Hidayat
no flags Details | Formatted Diff | Diff
2010-02-11 Ariya Hidayat <ariya.hidayat@gmail.com> (1.28 KB, patch)
2010-02-11 20:07 PST, Ariya Hidayat
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ariya Hidayat 2010-02-11 19:58:58 PST
Created attachment 48614 [details]
Patch

Use the similar trick like in r54347, i.e. use the special brush for
solid color to avoid expensive QBrush constructor.
Comment 1 Ariya Hidayat 2010-02-11 20:07:11 PST
Created attachment 48616 [details]
2010-02-11  Ariya Hidayat  <ariya.hidayat@gmail.com>
Comment 2 Kenneth Rohde Christiansen 2010-02-12 04:31:50 PST
Comment on attachment 48616 [details]
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?
Comment 3 Ariya Hidayat 2010-02-12 06:44:02 PST
(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).
Comment 4 Kenneth Rohde Christiansen 2010-02-12 08:17:31 PST
Comment on attachment 48616 [details]
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
Comment 5 Ariya Hidayat 2010-02-12 19:07:57 PST
(Added one line comment per Kenneth's request)

Manually committed in r54746: http://trac.webkit.org/changeset/54746