[webkit-reviews] review requested: [Bug 23291] -webkit-box-shadow doesn't work on Qt-WebKit : [Attachment 38878] Patch proposal that addresses Eric's concerns (see comment)

[bugzilla-daemon at webkit.org]

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

Attachment 38878: Patch proposal that addresses Eric's concerns (see comment)
https://bugs.webkit.org/attachment.cgi?id=38878&action=review

------- Additional Comments from Carol Szabo <carol.szabo at nokia.com>
(In reply to comment #13)
> (From update of attachment 38714 [details])
> +++ LayoutTests/platform/qt/fast/box-shadow/basic-shadows-expected.png   
> (working copy)
> is not properly marked as binary, making the diff nasty.

I do not know how to "properly" mark the file as binary. I marked it as
svn:mime-type=image/png, which it is and as a result the patch showed the
content of the file as base64 or something similar.
To work around this issue after consulting with ariya I have removed the new
and modified png files for the pixel tests from the patch and I shall submit
them as a separate attachment.

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

This is necessary to avoid compiler "possible overflow" warning as "qreal" can
hold larger numbers than "int" can.


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

The explicit construction is not needed, but since it was used elsewhere in the
file I used it for consistency in the original patch.
Now it is removed.

> 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) {
> 

There is neither QPen nor QBrush to bool conversion (as far as I can tell).
PenStyle and BrushStyle to bool conversions are implemented by the compiler
(enum to bool) but the code readability will suffer if we use them:
if(pen.style()) ??? it is odd and not immediately obvious, while if(pen) looks
more acceptable but creates some confusion with checking for a NULL QPen
pointer.
> Again, explicit construction of the color at least is likely not needed:
>  599		   p->setBrush(QBrush(QColor(shadowColor)));
> Color has a QColor operator, no?
> 

Removed

> 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?
> 

I am not 100% sure I know what you mean here, but I see no reference to any pen
in the neighborhood of this code. Since it is a fill, it would use a brush.
I considered that for code readability, size and possibly speed also, it is
better to save the worldTransform (a 9 qreal matrix), change it and restore it
than to offset all points in the path twice although for short paths (4 points
or less definitely and maybe even for a little more points, changing the matrix
may be more costly).

> 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()
> 

Please do not blame me for the code commented on above, which I do not fully
understand. All I did was to indent it properly. I did not want to change its
behavior, as the current behavior is supposedly tested and running until a bug
is written against it and then fixing this code will be part of that bug fix
not of shadow implementation.

> 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.
> 

I have created shared implementation for fillPath and fillRect. I am not
particularly happy since the code is used in only 2 places in each case and
shadows are rarely, if ever, used, while the overhead of calculating parameters
and calling the function will be encountered always.
I could have written some tricky macros to take care of the repetition such as:

#define BEGIN_SHADOW ... if(getSahdow(...)) {
#define END_SHADOW }

but I felt they bring little value and make the code more cryptic than it needs
to be.

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

I am not sure I understand what Eric means here. More context lines maybe?