Bug 141967

Summary: [Cairo] Incorrect dashed and dotted border painting after r177686.
Product: WebKit Reporter: Hunseop Jeong <hs85.jeong>
Component: WebCore Misc.Assignee: Hunseop Jeong <hs85.jeong>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex, bfulgham, clopez, commit-queue, d-r, gyuyoung.kim, jinwoo7.song, mcatanzaro, mrobinson, ossy, peavo, simon.fraser, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 139872    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Hunseop Jeong 2015-02-24 05:20:32 PST 
Fix the incorrect dashed/dotted border painting in cairo.
This patch referred to r177686.
Comment 1 Hunseop Jeong 2015-02-24 05:25:56 PST 
Created attachment 247227 [details]
Patch
Comment 2 Hunseop Jeong 2015-03-13 04:35:25 PDT 
Created attachment 248584 [details]
Patch
Comment 3 Hunseop Jeong 2015-03-13 04:57:58 PDT 
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.
Comment 4 Hunseop Jeong 2015-07-01 21:44:48 PDT 
Created attachment 255990 [details]
Patch
Comment 5 Hunseop Jeong 2015-07-08 23:08:34 PDT 
Now EFL, gtk+ port paint the dashed and dotted border incorrectly.
So I changed the code to draw the correct border.
Comment 6 Martin Robinson 2015-07-13 08:15:59 PDT 
Thanks for the patch. Are the tests you mentioned skipped on EFL and GTK+? If you would you unskip them in your patch?
Comment 7 Brent Fulgham 2015-08-13 09:39:55 PDT 
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.
Comment 8 Gyuyoung Kim 2015-10-13 23:16:47 PDT 
*** Bug 149948 has been marked as a duplicate of this bug. ***
Comment 9 Gyuyoung Kim 2015-10-13 23:19:35 PDT 
(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.
Comment 10 Hunseop Jeong 2015-10-14 02:04:51 PDT 
Created attachment 263066 [details]
Patch
Comment 11 Hunseop Jeong 2015-10-14 02:16:40 PDT 
(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 ].
Comment 12 Hunseop Jeong 2015-10-14 02:24:21 PDT 
(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 13 Gyuyoung Kim 2015-10-14 19:05:55 PDT 
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 14 Gyuyoung Kim 2015-10-27 21:28:43 PDT 
Comment on attachment 263066 [details]
Patch

rs=me. I think this implementation is based on Mac port's one.
Comment 15 WebKit Commit Bot 2015-10-27 22:20:55 PDT 
Comment on attachment 263066 [details]
Patch

Clearing flags on attachment: 263066

Committed r191658: <http://trac.webkit.org/changeset/191658>
Comment 16 WebKit Commit Bot 2015-10-27 22:21:00 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 17 Brent Fulgham 2015-10-28 09:02:34 PDT 
Alex, Per: You should double-check the WinCairo output to see if this creates a progression, or if we need to revise test expectations.
Comment 18 Michael Catanzaro 2015-10-28 11:13:49 PDT 
Does this fix the broken border on Bugzilla pages?
Comment 19 Alejandro G. Castro 2015-11-18 04:16:48 PST 
The patch broke the SolidStroke when drawing a line, 2 mathml tests failed after it. Check bug 151385.
Comment 20 Gyuyoung Kim 2015-12-08 02:04:29 PST 
(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.
Comment 21 Michael Catanzaro 2015-12-08 02:20:40 PST 
(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.
Comment 22 Gyuyoung Kim 2015-12-08 02:46:27 PST 
(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.