Closed Bug 1178971 Opened 4 years ago Closed 4 years ago

Change drawing path of dotted/dashed lines of table borders

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kyle_fung, Assigned: kyle_fung)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

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.
Assignee: nobody → kfung
Blocks: paint-fast
Attached patch fast-dash-borders.patch (obsolete) — Splinter Review
Attachment #8629487 - Flags: review?(mstange)
Builds for profiling purposes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c2c0ad5668a
Attached patch fast-dash-borders-v2.patch (obsolete) — Splinter Review
Forgot to change a variable to Float.
Attachment #8629487 - Attachment is obsolete: true
Attachment #8629487 - Flags: review?(mstange)
Attachment #8629491 - Flags: review?(mstange)
Attached file table.html
Summary: Change drawing path of dotted/dashed lines → Change drawing path of dotted/dashed lines of table borders
Attached patch round-snapping.patch (obsolete) — Splinter Review
Changed the way line snapping rounds depending on the even/odd-ness of the stroke width.
Attachment #8630545 - Flags: review?(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 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.
Attached patch fast-dash-borders-v3.patch (obsolete) — Splinter Review
Fixed some styling issues. This patch replaces the loop previously used to draw dashed lines with a single call.
Attachment #8629491 - Attachment is obsolete: true
Attachment #8629491 - Flags: review?(mstange)
Attachment #8630629 - Flags: review?(mstange)
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.
Attachment #8630545 - Attachment is obsolete: true
Attachment #8630545 - Flags: review?(mstange)
Attachment #8630631 - Flags: review?(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
Attachment #8630629 - Flags: review?(mstange) → review+
Attachment #8630631 - Flags: review?(mstange) → review+
Fixed styling issue.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fb8bb508dd
Attachment #8630629 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4823385c1005
https://hg.mozilla.org/mozilla-central/rev/c1c2fe57cf91
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1196180
You need to log in before you can comment on or make changes to this bug.