Bug 141967 – [Cairo] Incorrect dashed and dotted border painting after r177686.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hunseop Jeong]

Fix the incorrect dashed/dotted border painting in cairo.
This patch referred to r177686.

[Hunseop Jeong]

Created attachment 247227 [details]
Patch

[Hunseop Jeong]

Created attachment 248584 [details]
Patch

[Hunseop Jeong]

The tests below are passed on EFL after applying this patch.

fast/borders/0px-borders.html [ ImageOnlyFailure ]
fast/backgrounds/background-opaque-clipped-gradients.html [ ImageOnlyFailure ]

But, tests below are still failed.
fast/borders/border-painting-correctness-dashed.html [ ImageOnlyFailure ]
fast/borders/border-painting-correctness-dotted.html [ ImageOnlyFailure ]

Some pixels are different from the expected result.
Need to investigate why it is different with CG.

[Hunseop Jeong]

Created attachment 255990 [details]
Patch

[Hunseop Jeong]

Now EFL, gtk+ port paint the dashed and dotted border incorrectly.
So I changed the code to draw the correct border.

[Martin Robinson]

Thanks for the patch. Are the tests you mentioned skipped on EFL and GTK+? If you would you unskip them in your patch?

[Brent Fulgham]

Comment on attachment 255990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255990&action=review

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:302
> +        int numberOfSegments = floorf(strokeWidth / patternWidth);

Maybe this should be std::floor(strokeWidth / patternWidth)?

Is truncation right for the segment count? We don't want to round to the nearest integer?

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306
> +            patternOffset -= remainder / 2;

Should this be "remainder / 2.0f" to avoid possible truncation? Maybe the compiler does the right thing and doesn't do an integer divide...

Or better yet, why not multiply by 0.5f, since that's a lower-cost operation?

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:309
> +                patternOffset += patternOffset - (patternWidth + remainder)  / 2;

Ditto. Looks like an extra space before the slash.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:311
> +                patternOffset += patternWidth  / 2;

Ditto.

[Gyuyoung Kim]

*** Bug 149948 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

(In reply to comment #7)
> Comment on attachment 255990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255990&action=review
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:302
> > +        int numberOfSegments = floorf(strokeWidth / patternWidth);
> 
> Maybe this should be std::floor(strokeWidth / patternWidth)?
> 
> Is truncation right for the segment count? We don't want to round to the
> nearest integer?
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306
> > +            patternOffset -= remainder / 2;
> 
> Should this be "remainder / 2.0f" to avoid possible truncation? Maybe the
> compiler does the right thing and doesn't do an integer divide...
> 
> Or better yet, why not multiply by 0.5f, since that's a lower-cost operation?
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:309
> > +                patternOffset += patternOffset - (patternWidth + remainder)  / 2;
> 
> Ditto. Looks like an extra space before the slash.
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:311
> > +                patternOffset += patternWidth  / 2;
> 
> Ditto.

Hunseop, any update ? We need to fix this issue soon.

[Hunseop Jeong]

Created attachment 263066 [details]
Patch

[Hunseop Jeong]

(In reply to comment #6)
> Thanks for the patch. Are the tests you mentioned skipped on EFL and GTK+?
> If you would you unskip them in your patch?

I only unskip the fast/borders/0px-borders.html because some pixels are different with expected result in fast/borders/border-painting-correctness-foo tests.
So I remain the tests with [ ImageOnlyFailure ].

[Hunseop Jeong]

(In reply to comment #9)
> (In reply to comment #7)
> > Comment on attachment 255990 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255990&action=review
> > 
> > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:302
> > > +        int numberOfSegments = floorf(strokeWidth / patternWidth);
> > 
> > Maybe this should be std::floor(strokeWidth / patternWidth)?
> > 
> > Is truncation right for the segment count? We don't want to round to the
> > nearest integer?
> > 
> > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306
> > > +            patternOffset -= remainder / 2;
> > 
> > Should this be "remainder / 2.0f" to avoid possible truncation? Maybe the
> > compiler does the right thing and doesn't do an integer divide...
> > 
> > Or better yet, why not multiply by 0.5f, since that's a lower-cost operation?
> > 
> > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:309
> > > +                patternOffset += patternOffset - (patternWidth + remainder)  / 2;
> > 
> > Ditto. Looks like an extra space before the slash.
> > 
> > > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:311
> > > +                patternOffset += patternWidth  / 2;
> > 
> > Ditto.

This patch is based on r177686.
Anyway I changed the codes by following your comments.
Thanks.

[Gyuyoung Kim]

Comment on attachment 263066 [details]
Patch

This patch is implemented based on GraphicsContextCG.cpp implementation. I think this bug is critical issue in EFL and GTK ports. Hope to fix this issue as soon as possible. Any other review comment ?

[Gyuyoung Kim]

Comment on attachment 263066 [details]
Patch

rs=me. I think this implementation is based on Mac port's one.

[WebKit Commit Bot]

Comment on attachment 263066 [details]
Patch

Clearing flags on attachment: 263066

Committed r191658: <http://trac.webkit.org/changeset/191658>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Alex, Per: You should double-check the WinCairo output to see if this creates a progression, or if we need to revise test expectations.

[Michael Catanzaro]

Does this fix the broken border on Bugzilla pages?

[Alejandro G. Castro]

The patch broke the SolidStroke when drawing a line, 2 mathml tests failed after it. Check bug 151385.

[Gyuyoung Kim]

(In reply to comment #18)
> Does this fix the broken border on Bugzilla pages?

This patch was to fix incorrect dashed|dotted line drawing. Dotted line was drawn on EFL and GTK MiniBrowser incorrectly.

- https://bug-149948-attachments.webkit.org/attachment.cgi?id=262755
- https://bug-149948-attachments.webkit.org/attachment.cgi?id=262756

How can I see the broken border on Bugzilla pages ? I'd like to check if the issue still occurs.

[Michael Catanzaro]

(In reply to comment #20)
> (In reply to comment #18)
> > Does this fix the broken border on Bugzilla pages?
> 
> This patch was to fix incorrect dashed|dotted line drawing. Dotted line was
> drawn on EFL and GTK MiniBrowser incorrectly.
> 
> - https://bug-149948-attachments.webkit.org/attachment.cgi?id=262755
> - https://bug-149948-attachments.webkit.org/attachment.cgi?id=262756
> 
> How can I see the broken border on Bugzilla pages ? I'd like to check if the
> issue still occurs.

It's the dotted line underneath the final comment in a Bugzilla bug, before the text entry, when you have set the text entry to appear at the bottom of the page in preferences. It's supposed to be a straight line, but recently it was broken such that the second half of the line was drawn slightly below the first half of the line. I cannot reproduce the issue anymore -- the line is completely straight now -- so this patch did indeed fix that bug.

[Gyuyoung Kim]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Does this fix the broken border on Bugzilla pages?
> > 
> > This patch was to fix incorrect dashed|dotted line drawing. Dotted line was
> > drawn on EFL and GTK MiniBrowser incorrectly.
> > 
> > - https://bug-149948-attachments.webkit.org/attachment.cgi?id=262755
> > - https://bug-149948-attachments.webkit.org/attachment.cgi?id=262756
> > 
> > How can I see the broken border on Bugzilla pages ? I'd like to check if the
> > issue still occurs.
> 
> It's the dotted line underneath the final comment in a Bugzilla bug, before
> the text entry, when you have set the text entry to appear at the bottom of
> the page in preferences. It's supposed to be a straight line, but recently
> it was broken such that the second half of the line was drawn slightly below
> the first half of the line. I cannot reproduce the issue anymore -- the line
> is completely straight now -- so this patch did indeed fix that bug.

Thanks, I also think this patch fixed it definitely.