[webkit-reviews] review denied: [Bug 23291] -webkit-box-shadow doesn't work on Qt-WebKit : [Attachment 38714] This is the second phase of the implementation of shadows on QtWebKit

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Carol Szabo <carol.szabo at nokia.com>'s
request for review:
Bug 23291: -webkit-box-shadow doesn't work on Qt-WebKit
https://bugs.webkit.org/show_bug.cgi?id=23291

Attachment 38714: This is the second phase of the implementation of shadows on
QtWebKit
https://bugs.webkit.org/attachment.cgi?id=38714&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
+++ LayoutTests/platform/qt/fast/box-shadow/basic-shadows-expected.png 
(working copy)
is not properly marked as binary, making the diff nasty.

I'm surprised the (int) is needed here:
 424	     shadowRect.inflate((int)p->pen().widthF());
also, normally we use static_cast<int>() for c++

I'm surprised explicit construction is needed here:
 566	     pen.setColor(QColor(shadowColor));

I'm a bit surprised these don't have a bool conversion operator:
 598	     if (p->brush().style() != Qt::NoBrush)
 601	     if (pen.style() != Qt::NoPen) {

Again, explicit construction of the color at least is likely not needed:
 599		 p->setBrush(QBrush(QColor(shadowColor)));
Color has a QColor operator, no?

I don't understand why this gets pulled into a local first:
 636		 QTransform t(p->worldTransform());
pen's don't have a save/retore?  Or I guess that would be more expensive?

Seems strange that platformGradient() doesn't return a gradient including the
transform?
 652		 QBrush
brush(*m_common->state.fillGradient->platformGradient());
 653		
brush.setTransform(m_common->state.fillGradient->gradientSpaceTransform());

Really?
	     TransformationMatrix affine;
 733		 p->fillRect(rect,
QBrush(m_common->state.fillPattern->createPlatformPattern(affine)));
In other platforms at least the passed in affine transform is supposed to be
the ctm()

Locally defined functions sure would make this easier, as we could pass them to
some generic "draw shadow" routine.  A shame that gcc disabled them.

Seems there is at least one instance of copy/paste fillPath() w/ shadow code
which we could easily share implementations of.

r- for the confused patch and for the questions above.	Would be great to share
more code here.