Bug 29362 – Qt build error in GraphicsContextQt.cpp (qt 4.4.0)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp: In member function 
‘void WebCore::GraphicsContext::drawRect(const WebCore::IntRect&)’:
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:425: error: no match
ing function for call to ‘QPainter::fillRect(WebCore::IntRect&, WebCore::Color&)
’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp: In member function ‘void WebCore::GraphicsContext::drawLine(const WebCore::IntPoint&, const WebCore::IntPoint&)’:
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:485: error: no matching function for call to ‘QPainter::fillRect(WebCore::FloatRect, WebCore::Color&)’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:486: error: no matching function for call to ‘QPainter::fillRect(WebCore::FloatRect, WebCore::Color&)’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:488: error: no matching function for call to ‘QPainter::fillRect(WebCore::FloatRect, WebCore::Color&)’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:489: error: no matching function for call to ‘QPainter::fillRect(WebCore::FloatRect, WebCore::Color&)’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp: In function ‘void WebCore::drawBorderlessRectShadow(WebCore::GraphicsContext*, QPainter*, const WebCore::FloatRect&)’:
../../../WebCore/platform/graphics/qt/GraphicsContextQt.cpp:721: error: no matching function for call to ‘QPainter::fillRect(WebCore::FloatRect&, WebCore::Color&)’
/usr/include/qt4/QtGui/qpainter.h:382: note: candidates are: void QPainter::fillRect(const QRectF&, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:714: note:                 void QPainter::fillRect(int, int, int, int, const QBrush&)
/usr/include/qt4/QtGui/qpainter.h:384: note:                 void QPainter::fillRect(const QRect&, const QBrush&)
make[1]: *** [obj/release/GraphicsContextQt.o] Error 1

[Fumitoshi Ukai]

gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu4)

[Fumitoshi Ukai]

Created attachment 39752 [details]
Qt build fix


---
 2 files changed, 18 insertions(+), 6 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 39752 [details]
Qt build fix

This looks fine, but you should explain why this is needed in the ChangeLog.  It looks like QBrush(QPen) was marked as explicit in 4.4?

Please add some explanation to the ChangeLog on landing.

[Fumitoshi Ukai]

Created attachment 40040 [details]
Qt build fix

[Eric Seidel (no email)]

I have no idea what this sentence means:

 8         In Qt 4.4, QBrush constructor that takes QColor was added additional
 9         default parameter Qt::BrushStyle, so that WebCore::Color isn't
 10         converted as QBrush implicitly.

[Eric Seidel (no email)]

Comment on attachment 39752 [details]
Qt build fix

Clearing r+ on this old patch now that a new one has been posted.

[Ariya Hidayat]

Comment on attachment 40040 [details]
Qt build fix

The idea is good, the implementation needs some tweak.

> +        In Qt 4.4, QBrush constructor that takes QColor was added additional
> +        default parameter Qt::BrushStyle, so that WebCore::Color isn't
> +        converted as QBrush implicitly.

Actually, it's more like "In Qt 4.5, a new function QPainter::fillRect(QRect, QColor) is introduced".
The idea is to speed up solid color fill, because we can avoid the rather expensive construction of QBrush.

> -        p->fillRect(shadowRect, shadowColor);
> +        p->fillRect(shadowRect, QBrush(shadowColor));

And therefore the right fix should be QColor(shadowColor). Otherwise, we keep falling back to use QBrush, thereby cancelling out the intended optimization.

[Fumitoshi Ukai]

Created attachment 40222 [details]
Use QColor(shadowColor) instead of QBrush(shadowColor)

[Fumitoshi Ukai]

Thanks for review!

(In reply to comment #7)
> (From update of attachment 40040 [details])
> The idea is good, the implementation needs some tweak.
> 
> > +        In Qt 4.4, QBrush constructor that takes QColor was added additional
> > +        default parameter Qt::BrushStyle, so that WebCore::Color isn't
> > +        converted as QBrush implicitly.
> 
> Actually, it's more like "In Qt 4.5, a new function QPainter::fillRect(QRect,
> QColor) is introduced".
> The idea is to speed up solid color fill, because we can avoid the rather
> expensive construction of QBrush.

Ah, I see.

> 
> > -        p->fillRect(shadowRect, shadowColor);
> > +        p->fillRect(shadowRect, QBrush(shadowColor));
> 
> And therefore the right fix should be QColor(shadowColor). Otherwise, we keep
> falling back to use QBrush, thereby cancelling out the intended optimization.

Fixed to use QColor(shadowColor) instead of QBrush(shadowColor).

[Eric Seidel (no email)]

Comment on attachment 40222 [details]
Use QColor(shadowColor) instead of QBrush(shadowColor)

I'm confused.

http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/Color.h#L111

Transparently converts to QColor, and not to QBrush, right?  So why is an explicit conversion necessary here?  Shouldn't operator QColor automatically be called?  Or maybe I'm incorrect in my understanding of the QColor operator().

I still don't understand why this is necessary?  Won't the existing code already do the right thing on either Qt 4.4 or 4.5?

[Eric Seidel (no email)]

Clearly it's already correctly calling "QRect IntRect::operator()" correctly, no?
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/IntRect.h#L145

I'm confused as to why we need to be explicit about calling QColor() here for C++ to understand this.

[Fumitoshi Ukai]

(In reply to comment #11)
> Clearly it's already correctly calling "QRect IntRect::operator()" correctly,
> no?
> http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/IntRect.h#L145
> 
> I'm confused as to why we need to be explicit about calling QColor() here for
> C++ to understand this.

Here, fillRect wants a QBrush and we passes a WebCore::Color. So compiler needs to find QColor to convert from WebCore::Color to QBrush, which is different situation from IntRect to QRect (no intermediate class needed).
(In Qt 4.5, fillRect takes a QColor, so it works fine?)
I'm not sure, but this is the explanation?

[Eric Seidel (no email)]

I could believe that the compiler wouldn't understand a two-level "cast" like that.  But it seems it already is, no?  Isn't the QColor variant new?  Or is this bug about a build error in versions of Qt before the Qcolor version of the function existed?

[Fumitoshi Ukai]

(In reply to comment #13)
> I could believe that the compiler wouldn't understand a two-level "cast" like
> that.  But it seems it already is, no?  Isn't the QColor variant new?  Or is
> this bug about a build error in versions of Qt before the Qcolor version of the
> function existed?

Hmm. I encountered this bug when I first tried to compile for Qt.

It seems webkit r48220 removed QColor, and broke build on Qt 4.4?
Does Qt port requires Qt 4.5 or later now?

[Ariya Hidayat]

Eric: it's likely because there is no conversion from WebCore::Color to QBrush (needed for fillRect in Qt < 4.5), and for which the compiler seems unable to find by itself.

Another alternative patch will be to add WebCore::Color to QBrush explicit conversion. But then, I'm not sure which code path the compiler will take in 4.5 (either it will be confused due to the ambiguity or take the "slower" one, i.e. QBrush).

[WebKit Commit Bot]

Comment on attachment 40222 [details]
Use QColor(shadowColor) instead of QBrush(shadowColor)

Clearing flags on attachment: 40222

Committed r49165: <http://trac.webkit.org/changeset/49165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.