Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(16)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 15804009: Fix border radius glitch. (Closed)

Created:
7 years, 6 months ago by kouhei (in TOK)
Modified:
7 years, 5 months ago
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, tasak (please_use_google.com), tkent, haraken
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Fix border radius glitch. This fixes the bug that the border radius partially disappeared or had wrong color when it was too steep or too shallow. Fix 1: Use the correct clipping quad, generated by intersecting border mitre lines to diagonal lines of the corner ellipse arc. RenderBoxModelObject::clipBorderSidePolygon was generating wrong clipping quad. It was based on a wrong assumption that rounded border would not cross {horizontal,vertical} line through the box center. Fix 2: Correctly split a quad into two quads. A clipping quad is split into "AND" of two quads to set anti-aliasing on both edges individually. Generate these quads properly by extending a vertex parallel to the opposite edge. TEST=LayoutTests/fast/borders/* BUG=160239 BUG=244748 R=jchaffraix@chromium.org, tasak@google.com Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153119

Patch Set 1 #

Patch Set 2 : remove unused func #

Patch Set 3 : handle triangle case / calc. errors #

Patch Set 4 : adding new LayoutTests #

Total comments: 1

Patch Set 5 : add expectations #

Patch Set 6 : fabsf #

Total comments: 13

Patch Set 7 : rebase / nitsfix #

Patch Set 8 : newexp #

Patch Set 9 : rebase #

Patch Set 10 : add more exp #

Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -39 lines) Patch
A LayoutTests/fast/borders/borderRadiusMultiColors01.html View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
A + LayoutTests/fast/borders/borderRadiusMultiColors01-expected.png View 1 2 3 4 5 6 7 8 Binary file 0 comments Download
A LayoutTests/fast/borders/borderRadiusMultiColors01-expected.txt View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/fast/borders/borderRadiusMultiColors02.html View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
A + LayoutTests/fast/borders/borderRadiusMultiColors02-expected.png View 1 2 3 4 7 8 Binary file 0 comments Download
A LayoutTests/fast/borders/borderRadiusMultiColors02-expected.txt View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/fast/borders/borderRadiusSlope.html View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
A + LayoutTests/fast/borders/borderRadiusSlope-expected.png View 1 2 3 4 7 8 Binary file 0 comments Download
A + LayoutTests/fast/borders/borderRadiusSlope-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/platform/chromium-linux/fast/borders/border-radius-wide-border-03-expected.png View 1 2 3 4 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/borders/border-styles-split-expected.png View 1 2 3 4 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/borders/borderRadiusDouble05-expected.png View 1 2 3 4 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/borders/borderRadiusDouble08-expected.png View 1 2 3 4 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/borders/mixed-border-styles-radius-expected.png View 1 2 3 4 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/writing-mode/border-styles-vertical-lr-expected.png View 1 2 3 4 5 6 7 9 Binary file 0 comments Download
M LayoutTests/platform/chromium-linux/fast/writing-mode/border-styles-vertical-rl-expected.png View 1 2 3 4 5 6 7 9 Binary file 0 comments Download
M Source/core/rendering/RenderBoxModelObject.cpp View 1 2 3 4 5 6 7 8 6 chunks +105 lines, -37 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
kouhei (in TOK)
WIP. fix both bugs, but no test needs layout test at least.
7 years, 6 months ago (2013-05-29 09:04:49 UTC) #1
kouhei (in TOK)
It is ready for review now. This fails some border-radius LayoutTests, but I think they ...
7 years, 6 months ago (2013-05-30 02:37:15 UTC) #2
kouhei (in TOK)
https://chromiumcodereview.appspot.com/15804009/diff/9001/Source/core/rendering/RenderBoxModelObject.cpp File Source/core/rendering/RenderBoxModelObject.cpp (right): https://chromiumcodereview.appspot.com/15804009/diff/9001/Source/core/rendering/RenderBoxModelObject.cpp#newcode2151 Source/core/rendering/RenderBoxModelObject.cpp:2151: if (bx < kEpsilon && by < kEpsilon) { ...
7 years, 6 months ago (2013-05-30 15:24:55 UTC) #3
tkent
I'm not familiar with this code. jchaffraix, tasak? We should not add tests without expectations. ...
7 years, 6 months ago (2013-05-30 23:16:11 UTC) #4
tasak
lgtm. The logic looks good.
7 years, 6 months ago (2013-05-31 08:07:38 UTC) #5
kouhei (in TOK)
jchaffraix: Would you take a look?
7 years, 6 months ago (2013-06-05 00:14:42 UTC) #6
Julien - ping for review
lgtm, assuming the test are passing as the try jobs couldn't apply the change. https://codereview.chromium.org/15804009/diff/26001/LayoutTests/fast/borders/borderRadiusMultiColors01.html ...
7 years, 6 months ago (2013-06-10 19:50:31 UTC) #7
kouhei (in TOK)
Thanks for review! https://codereview.chromium.org/15804009/diff/26001/LayoutTests/fast/borders/borderRadiusMultiColors01.html File LayoutTests/fast/borders/borderRadiusMultiColors01.html (right): https://codereview.chromium.org/15804009/diff/26001/LayoutTests/fast/borders/borderRadiusMultiColors01.html#newcode1 LayoutTests/fast/borders/borderRadiusMultiColors01.html:1: <html><body> On 2013/06/10 19:50:31, Julien Chaffraix ...
7 years, 6 months ago (2013-06-11 21:02:53 UTC) #8
kouhei (in TOK)
7 years, 5 months ago (2013-06-27 05:41:36 UTC) #9
Message was sent while issue was closed. 
Committed patchset #10 manually as r153119 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698