1178971 - Change drawing path of dotted/dashed lines of table borders

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kyle Fung]

Currently, dotted and dashed table borders are drawn by drawing multiple segments through a loop in nsCSSRendering::DrawTableBorderSegment :

>        for (int32_t spaceX = 0; spaceX < numDashSpaces; spaceX++) {
>          rect.x += rect.width + dashLength;
>          rect.width = (spaceX == (numDashSpaces - 1)) ? endDashLength : dashLength;
>          DrawSolidBorderSegment(aContext, rect, aBorderColor, aAppUnitsPerDevPixel, twipsPerPixel);
>        }

This is not practical since a dotted line can have many dots, so we should try to just draw the entire line in one call.

Comment 1

[Kyle Fung]

Attachment: fast-dash-borders.patch

Comment 2

[Kyle Fung]

Builds for profiling purposes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c2c0ad5668a

Comment 3

[Kyle Fung]

Attachment: fast-dash-borders-v2.patch

Forgot to change a variable to Float.

Comment 4

[Kyle Fung]

Attachment: table.html

Comment 5

[Markus Stange [:mstange]]

Attachment: treeherder-like testcase

Comment 6

[Kyle Fung]

Attachment: round-snapping.patch

Changed the way line snapping rounds depending on the even/odd-ness of the stroke width.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8630545 [details] [diff] [review]
round-snapping.patch

You also need to modify aP1 and aP2 in the even case. Right now your patch leaves them unchanged, but we want to snap them to integer pixels.

The lineWidth argument should be called aLineWidth (mozilla style) and should have the type Float, otherwise the Float -> int conversion isn't obvious enough. You could make the cast explicit like this:
bool lineWidthIsOdd = int(aLineWidth) % 2 == 1;

Some lines need to be rewrapped so that they don't exceed 80 characters.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8629491 [details] [diff] [review]
fast-dash-borders-v2.patch

Review of attachment 8629491 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +3692,5 @@
> +                  nscoord              aDashLength,
> +                  nscolor              aColor,
> +                  int32_t              aAppUnitsPerDevPixel,
> +                  nscoord              aTwipsPerPixel,
> +                  bool                 horizontal)

aHorizontal

@@ +3710,5 @@
> +
> +  nsPoint left   = (aRect.TopLeft()    + aRect.BottomLeft() ) / 2;
> +  nsPoint top    = (aRect.TopLeft()    + aRect.TopRight()   ) / 2;
> +  nsPoint right  = (aRect.TopRight()   + aRect.BottomRight()) / 2;
> +  nsPoint bottom = (aRect.BottomLeft() + aRect.BottomRight()) / 2;

Please move the calculation of these values inside the if branches that need them.
And don't bother with the alignment. We don't do it anywhere else, as far as I know. It won't look as tidy but it'll still be very obvious what's happening.

@@ +3713,5 @@
> +  nsPoint right  = (aRect.TopRight()   + aRect.BottomRight()) / 2;
> +  nsPoint bottom = (aRect.BottomLeft() + aRect.BottomRight()) / 2;
> +
> +  if (horizontal) {
> +    strokeOptions.mLineWidth = Float(aRect.height)/aAppUnitsPerDevPixel;

Add spaces around the /, please.

@@ +3894,5 @@
>          DrawSolidBorderSegment(aContext, rect, aBorderColor, aAppUnitsPerDevPixel, twipsPerPixel);
> +
> +        rect.x += startDashLength + dashLength;
> +        rect.width = aBorder.width - (startDashLength + endDashLength + dashLength);
> +        DrawDashedSegment(aContext, rect, dashLength, aBorderColor, aAppUnitsPerDevPixel, twipsPerPixel, horizontal);

Please wrap after "aAppUnitsPerDevPixel,", in all four occurrences of this.

Comment 9

[Kyle Fung]

Attachment: fast-dash-borders-v3.patch

Fixed some styling issues. This patch replaces the loop previously used to draw dashed lines with a single call.

Comment 10

[Kyle Fung]

Attachment: round-snapping-v2.patch

Fixed minor issues from review. This patches fixes the way snapping is done when it is used for a stroked line. Before, a 0.5px offset was added to each of the two points of the line, but this causes certain backends to render a line slightly too wide. Adding this offset only when the width is odd fixes this problem.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8630629 [details] [diff] [review]
fast-dash-borders-v3.patch

Review of attachment 8630629 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: layout/base/nsCSSRendering.cpp
@@ +3715,5 @@
> +    StrokeLineWithSnapping(left, right,
> +                           aAppUnitsPerDevPixel, *drawTarget,
> +                           color, strokeOptions, drawOptions);
> +  }
> +  else {

} else {

in one line please

Comment 12

[Kyle Fung]

Attachment: fast-dash-borders-v4.patch

Fixed styling issue.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fb8bb508dd

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4823385c1005
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c2fe57cf91

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4823385c1005
https://hg.mozilla.org/mozilla-central/rev/c1c2fe57cf91