Fix the incorrect dashed/dotted border painting in cairo. This patch referred to r177686.
Created attachment 247227 [details] Patch
Created attachment 248584 [details] Patch
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.
Created attachment 255990 [details] Patch
Now EFL, gtk+ port paint the dashed and dotted border incorrectly. So I changed the code to draw the correct border.
Thanks for the patch. Are the tests you mentioned skipped on EFL and GTK+? If you would you unskip them in your patch?
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.
*** Bug 149948 has been marked as a duplicate of this bug. ***
(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.
Created attachment 263066 [details] Patch
(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 ].
(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.
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 ?
Comment on attachment 263066 [details] Patch rs=me. I think this implementation is based on Mac port's one.
Comment on attachment 263066 [details] Patch Clearing flags on attachment: 263066 Committed r191658: <http://trac.webkit.org/changeset/191658>
All reviewed patches have been landed. Closing bug.
Alex, Per: You should double-check the WinCairo output to see if this creates a progression, or if we need to revise test expectations.
Does this fix the broken border on Bugzilla pages?
The patch broke the SolidStroke when drawing a line, 2 mathml tests failed after it. Check bug 151385.
(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.
(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.
(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.