Support filters in wrench #801
Conversation
|
The reftest is failing on CI - I haven't looked into why yet. |
|
☔ The latest upstream changes (presumably #806) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Okay, I got run_tests.py/headless working locally on my machine. The reftest failure is in the opacity test. Using 50% opacity on rgb(0,255,0) is giving rgb(127,255,127), while I hardcoded the reference to rgb(128,255,128). It was passing on my machine so I'm guessing it's hardware/software related. I don't know of any easy fix besides adding reftest fuzzing, so I'll remove the reftest for now. |
|
The tests are always run on the OSMesa software rasterizer, and we compile a specific version of that to be used on the CI machines. So it should be fine to just update the reftest to expect the value that you're seeing on the CI machines. |
|
(Or to be more specific, when running the tests locally, we should be using the headless mode - this will mean tests run locally should always match the results on CI). |
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Got a few styling notes, nothing major. Looks good.
| } | ||
|
|
||
| fn as_filter_op(&self) -> Option<FilterOp> { | ||
| if let Some(s) = self.as_str() { |
There was a problem hiding this comment.
this function body is overly verbose and could be reduced by some refactoring
not required for this PR though
There was a problem hiding this comment.
Yeah I definitely agree. It'd be nice to have something like define-enum-conversion that works with more general enums.
wrench/src/yaml_frame_reader.rs
Outdated
|
|
||
| // FIXME handle these | ||
| let filters: Vec<FilterOp> = Vec::new(); | ||
| let filters: Vec<FilterOp> = yaml["filters"].as_vec_filter_op().unwrap_or(vec![]); |
There was a problem hiding this comment.
nit: doesn't need explicit type here
wrench/src/yaml_helper.rs
Outdated
| if let Some(v) = self.as_vec() { | ||
| Some(v.iter().map(|x| x.as_filter_op().unwrap()).collect()) | ||
| } else { | ||
| if let Some(op) = self.as_filter_op() { |
There was a problem hiding this comment.
this asks for self.as_filter_op().map(|op| vec![op])
|
Investigating the opacity reftest further, it looks like it's not just the color that's off but there is a one pixel border on the bottom and right edges that is a different color than the rest of the rect. For now I think it's easiest to remove the test and maybe add it in later. |
|
@bors-servo r+ |
|
📌 Commit c9aced4 has been approved by |
Support filters in wrench @jrmuizel <!-- 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/801) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
5 participants
@jrmuizel
This change is