Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSupports border-style: double. #329
Conversation
|
@mephisto41 Thanks! Could you take a look over this please @changm ? |
| // The idea here is, offset pos by (1/3 * len) then mod it by len. | ||
| // And if this value is lesser than (2/3 * len) then we're in | ||
| // blank part. Thus, we can use only 1 check instead of 2. | ||
| float compared_pos = mod(pos + one_third_len, len); |
This comment has been minimized.
This comment has been minimized.
changm
Aug 8, 2016
Contributor
How does this work? Let's say I have a length of 100 and I want to see of pos 50 is in the blank part.
So 0- 33 - Filled
33 - 66 - Blank
66 - 100 filled
(1/3) * len = 33 here
So offset 50 would be:
50 + 33 = 83
83 % 100 = 83
This doesn't match the comment, but it does match the code. In the code you're checking that compared_pos is greater than 2/3, which if true, means its in the blank part. Please fix the comment. I think should be "greater than (2/3 * len).
nit: I'd rather do the most obvious thing here and just do two checks.
Also, I should fix the other borders too, but please do the calculations in local space rather than device space. Thanks!
This comment has been minimized.
This comment has been minimized.
mephisto41
Aug 9, 2016
Author
Contributor
I change my implementation so it is easier to understand. And also calculations in local space. Would you check it again? Thanks very much.
mephisto41
force-pushed the
mephisto41:border-style-double
branch
from
566f0f3
to
f194279
| return draw_double_edge_with_radius(); | ||
| } | ||
|
|
||
| bool is_vertical = (vBorderPart == PST_TOP_LEFT) ? vF < 0 : vF >= 0; |
This comment has been minimized.
This comment has been minimized.
changm
Aug 9, 2016
•
Contributor
Thanks for the other fixes!
How does this work for the bottom left corner? Seems like the vertical case shouldn't work here?
This comment has been minimized.
This comment has been minimized.
mephisto41
Aug 10, 2016
Author
Contributor
The vF calculation is quiet confuse now. For the vertical case, only the vF on top left corner is lesser than 0. All other 3 corners are greater than 0. I just tweak the vF calculation to make it more consistent. After my tweak, the vF stand for vertical edge if vF > 0 and for horizontal edge if vF < 0 for all corners, without exception. So this is_vertical check would be more clear.
mephisto41
force-pushed the
mephisto41:border-style-double
branch
from
f194279
to
ec96584
| @@ -863,8 +863,8 @@ impl Primitive { | |||
| details.tl_inner.x, | |||
| details.tl_inner.y), | |||
| }, | |||
| vertical_color: details.top_color, | |||
This comment has been minimized.
This comment has been minimized.
changm
Aug 10, 2016
Contributor
This will break inset borders. Let's just revert the colors back and keep the top left as is. Sorry for reverting the change. The previous commit works fine. Thanks again!
mephisto41
force-pushed the
mephisto41:border-style-double
branch
from
ec96584
to
76f20bc
|
I have reverted to previous commit. Thanks! |
|
LGTM! Thanks @glennw care to take a look or merge if you think it's good. Thanks! |
|
@bors-servo r+ |
|
|
Supports border-style: double. Add support for border-style: double. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/329) <!-- Reviewable:end -->
|
|
mephisto41 commentedAug 5, 2016
•
edited by larsbergstrom
Add support for border-style: double.
This change is