[webkit-reviews] review denied: [Bug 92383] [Qt] Dotted borders not drawn with rounded dots : [Attachment 156274] Patch with updated expected results

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 92383: [Qt] Dotted borders not drawn with rounded dots
https://bugs.webkit.org/show_bug.cgi?id=92383

Attachment 156274: Patch with updated expected results
https://bugs.webkit.org/attachment.cgi?id=156274&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=156274&action=review


Looks good, But GraphicsContext::drawLine is growing to epic proportions. Let's
try to split the dashing etc. to separate functions?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:376
> +	   if (isVerticalLine) {
> +	       p1.setY(p1.y() - width / 2);
> +	       p2.setY(p2.y() + width / 2);
> +	   } else {
> +	       p1.setX(p1.x() - width / 2);
> +	       p2.setX(p2.x() + width / 2);
> +	   }

Can we wrap this with some function with a descriptive name?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:415
> +	       // Draw circles circles in the corners too.

circles circles

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:419
> +	   if (style == DashedStroke) {
> +	       capStyle = Qt::FlatCap;
> +	       dashes << qreal(patWidth) / width << qreal(patWidth) / width;
> +
> +	       if (isVerticalLine) {
> +		   p->fillRect(FloatRect(p1.x() - width / 2, p1.y() - width,
width, width), QColor(color));
> +		   p->fillRect(FloatRect(p2.x() - width / 2, p2.y(), width,
width), QColor(color));
> +	       } else {
> +		   p->fillRect(FloatRect(p1.x() - width, p1.y() - width / 2,
width, width), QColor(color));
> +		   p->fillRect(FloatRect(p2.x(), p2.y() - width / 2, width,
width), QColor(color));
> +	       }
> +	   }
> +
> +	   // As per css spec a dotted stroke should be made of circles.
> +	   if (style == DottedStroke) {
> +	       capStyle = Qt::RoundCap;
> +	       // The actual length of one line element can not be set to zero
and at 0.1 the dots
> +	       // are still slightly elongated. Setting it to 0.01 will make it
look like the
> +	       // line endings are being stuck together, close enough to look
like a circle.
> +	       // For the distance of the line elements we subtract the small
amount again.
> +	       qreal lineElementLength = 0.01;
> +	       dashes << lineElementLength << qreal(2 * patWidth) / width -
lineElementLength;
> +
> +	       // Draw circles circles in the corners too.
> +	       p->setPen(Qt::NoPen);
> +	       p->setBrush(QColor(color));
> +	       p->drawEllipse(p1.x() - width / 2, p1.y() - width / 2, width,
width);
> +	       p->drawEllipse(p2.x() - width / 2, p2.y() - width / 2, width,
width);

Can we wrap this in a couple of functions with descriptive names?