Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clamp the blur radius #2076

Merged
merged 1 commit into from Nov 22, 2017
Merged

Clamp the blur radius #2076

merged 1 commit into from Nov 22, 2017

Conversation

ncarrillo
Copy link
Contributor

@ncarrillo ncarrillo commented Nov 21, 2017 •
edited by larsbergstrom

Fixes #2061
Still needs a reftest

This change is Reviewable

@ncarrillo
Copy link
Contributor Author

I added a reftest, it fails on the OSX build bot with the following:

REFTEST TEST-UNEXPECTED-FAIL | reftests/boxshadow/box-shadow-large-border-radius.yaml == reftests/boxshadow/box-shadow-large-border-radius.png | image comparison, max difference: 1, number of differing pixels: 2638

Maybe it needs to be fuzzed?

@glennw
Copy link
Member

glennw commented Nov 22, 2017

Did you generate the reference image with ./headless.py <yaml>? That should in theory make the reference consistent with the CI machines, which all run tests with OSMesa software rasterizer.

@ncarrillo
Copy link
Contributor Author

Oh nope, I did it using wrench itself. I'll regenerate.

@glennw
Copy link
Member

glennw commented Nov 22, 2017

r? @mstange

@mstange
Copy link
Contributor

mstange commented Nov 22, 2017

Looks good, but I think the test should be called box-shadow-large-blur-radius.yaml instead of box-shadow-large-border-radius.yaml. Would it be better to make the reference a YAML file that has a blur radius of 300, rather than a PNG?

@ncarrillo
Copy link
Contributor Author

I didn't know you could have YAML files on both sides of the ref tests. Cool!
Updated.

r? @mstange

@glennw
Copy link
Member

glennw commented Nov 22, 2017

Looks good, thank you!

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 9d19d39 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 9d19d39 with merge fb25304...

bors-servo pushed a commit that referenced this pull request Nov 22, 2017
Auto merge of #2076 - nc4rrillo:clamp-blur-radius, r=glennw
fb25304 
Clamp the blur radius

Fixes #2061
Still needs a reftest

<!-- 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/2076)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing fb25304 to master...

@bors-servo bors-servo merged commit 9d19d39 into servo:master Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ncarrillo @glennw @mstange @bors-servo