Lots of wrench fixes and features #660

vvuk · 2016-12-22T03:21:24Z

Support for many new yaml display items, clips, and other bits.

This change is

glennw

Just a couple of nits.

glennw · 2016-12-22T03:30:38Z

webrender_traits/src/display_item.rs

+           self.top_right == uniform_radius &&
+           self.bottom_left == uniform_radius &&
+           self.bottom_right == uniform_radius
+        {


nit: brace placement

glennw · 2016-12-22T03:30:38Z

webrender_traits/src/display_item.rs

+
+    pub fn is_uniform(&self) -> Option<f32> {
+        let uniform_radius = LayoutSize::new(self.top_left.width, self.top_left.width);
+        if self.top_left == uniform_radius &&


nit: don't need to check top_left here.

vvuk · 2016-12-22T13:51:56Z

Fixed.

kvark

A lot of great stuff here!

kvark · 2016-12-22T16:18:31Z

webrender/src/record.rs

@@ -18,6 +17,9 @@ lazy_static! {
    static ref WEBRENDER_RECORDING_DETOUR: Arc<Mutex<Option<Box<ApiRecordingReceiver>>>> = Arc::new(Mutex::new(None));
 }

+pub static WEBRENDER_RECORDING_HEADER: u64 = 0xbeefbeefbeefbe01u64;
+static mut CURRENT_FRAME_NUMBER: u32 = 0xffffffffu32;


could do ~0, easier than to count f

kvark · 2016-12-22T16:18:31Z

wrench/src/main.rs

+    let pct = pct_int as f32 / 100.;
+    let index_f = (values.len()-1) as f32 * pct;
+    let index = f32::floor(index_f) as usize;
+    if index == index_f as usize {


any reason of not going with integer-only arithmetic?

let index_big = (values.len() - 1) * (pct_int as usize); let index = index_big / 100; if index * 100 != index_big {..} else {..}

kvark · 2016-12-22T16:18:31Z

wrench/src/main.rs


    let mut min_time = time::Duration::max_value();
    let mut min_min_time = time::Duration::max_value();
    let mut max_time = time::Duration::min_value();
    let mut max_max_time = time::Duration::min_value();
    let mut sum_time = time::Duration::zero();

+    let mut block_avg_ms = vec![];


would better be called block_avg_durations, since ms here is incorrect

kvark · 2016-12-22T16:18:31Z

wrench/src/main.rs

        if let Some(limit) = limit_seconds {
            if (time::SteadyTime::now() - time_start) >= limit {
+                let mut block_avg_ms = block_avg_ms.iter().map(|v| as_ms(*v)).collect::<Vec<f64>>();
+                block_avg_ms.sort_by(|a, b| a.partial_cmp(b).unwrap());
+                let avg_ms = block_avg_ms.iter().fold(0., |sum, v| sum + v) / block_avg_ms.len() as f64;


nit: could use sum

kvark · 2016-12-22T16:18:31Z

wrench/src/wrench.rs

            frame_start_sender: timing_sender,
        };

        wrench.set_title("start");
+        // there's a "frame 0" that webrender itself renders; push this to
+        // not confuse our notifier
+        wrench.frame_start_sender.push(time::SteadyTime::now());


nit: use wrench.begin_frame() instead?
perhaps, frame_start_sender doesn't even need to be public

kvark · 2016-12-22T16:18:31Z

wrench/src/yaml_frame_reader.rs

+        let start = item["start"].as_point().expect("gradient must have start");
+        let end = item["end"].as_point().expect("gradient must have end");
+        let stops = if let Some(stops) = item["stops"].as_vec() {
+            // FIXME(vlad): I can't find the right way to do this with iterators;


Chunks come to rescue:

let g_stops = stops.chunks(2).map(|chunk| GradientStop { offset: chunk[0].as_force_f32().expect("gradient stop offset is not f32"), color: chunk[1].as_colorf().expect("gradient stop color is not color"), }).collect::<Vec<_>>();

kvark · 2016-12-22T16:18:31Z

wrench/src/yaml_frame_reader.rs

+            }
+        }
+    }
+
    fn handle_rect(&mut self, wrench: &mut Wrench, clip_region: &ClipRegion, item: &Yaml)
    {


opening brackets on the new line is against the rust style guidelines

kvark · 2016-12-22T16:18:31Z

wrench/src/yaml_frame_reader.rs

+        let colors = item["color"].as_vec_colorf().expect("borders must have color(s)");
+        let styles = item["style"].as_vec_string().expect("borders must have style(s)");
+        let styles = styles.iter().map(|s| match s {
+            s if s == "none" => BorderStyle::None,


You should be able to just do "something" => BorderStyle::Something without all the if in the matching

kvark · 2016-12-22T16:18:31Z

wrench/src/yaml_frame_reader.rs

+        let border_radius = item["border_radius"].as_force_f32().unwrap_or(0.0);
+        let clip_mode = if let Some(mode) = item.as_str() {
+            match mode {
+                s if s == "none" => BoxShadowClipMode::None,


same here. Just make sure you are matching &str and not something else

kvark · 2016-12-22T16:18:31Z

wrench/src/yaml_frame_writer.rs

@@ -14,6 +13,7 @@ use std::path::{Path, PathBuf};
 use webrender;
 use webrender_traits::*;
 use yaml_rust::{Yaml, YamlEmitter};
+use time;

 use super::CURRENT_FRAME_NUMBER;


any reason we can't just store the frame number locally and pass it around?

bors-servo · 2016-12-28T23:37:36Z

☔ The latest upstream changes (presumably #649) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-01-04T19:20:34Z

☔ The latest upstream changes (presumably #677) made this pull request unmergeable. Please resolve the merge conflicts.


        Add support for a bunch of new display item types to YAML

add flag to render only certain element types from YAML Output summary stats at end of time-limited run handle borders, box shadow, gradients in YAML Add binary recording header before TypeId, and check for it; clean up binary replay Fix yaml border stuff print 10th/90th percentiles add time prefix to resource filenames to avoid collisions Add --play option to binary frame reader, and fix window title setting Implement YAML clips properly Fix StackingContext bounds to record the proper rect Add 720p 1080p 4k size shorthand Add support for complex clips to wrench


        Use "image_mask" instead of "mask" to match reader

The field is called 'image_mask' so we might as well use that name.


        Fix up some review comments

kvark · 2017-01-05T19:02:22Z

Thanks @vvuk ! I think it's in good shape now. There are still minor issues to be addressed, but we can do it as a follow up, especially considering that someone is blocked by this PR (see #669).
@glennw - if you happy and you know it... merge this thing

glennw · 2017-01-05T19:05:39Z

@bors-servo r+

bors-servo · 2017-01-05T19:05:41Z

📌 Commit 7ead5ff has been approved by glennw

bors-servo · 2017-01-05T19:05:44Z

⌛ Testing commit 7ead5ff with merge 69d1e15...


        Auto merge of #660 - vvuk:wrench, r=glennw

Lots of wrench fixes and features Support for many new yaml display items, clips, and other bits.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/660)

bors-servo · 2017-01-05T20:21:25Z

☀️ Test successful - status-travis

vvuk force-pushed the vvuk:wrench branch from 59368a6 to 9cb51b8 Dec 22, 2016

glennw reviewed Dec 22, 2016

View changes

vvuk force-pushed the vvuk:wrench branch from 9cb51b8 to 9f4dce3 Dec 22, 2016

kvark reviewed Dec 22, 2016

View changes

kvark mentioned this pull request Jan 4, 2017

Fix `wrench` compile warnings #669

Closed

vvuk force-pushed the vvuk:wrench branch from 9f4dce3 to beee954 Jan 4, 2017

vvuk force-pushed the vvuk:wrench branch from beee954 to 66c519f Jan 4, 2017

jrmuizel and others added 2 commits Jan 5, 2017

Use "image_mask" instead of "mask" to match reader

Loading status checks…

c848b0f

The field is called 'image_mask' so we might as well use that name.

Fix up some review comments

Loading status checks…

7ead5ff

bors-servo merged commit 7ead5ff into servo:master Jan 5, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / webrender

Lots of wrench fixes and features #660

Lots of wrench fixes and features #660

vvuk commented Dec 22, 2016 •

edited by larsbergstrom

glennw left a comment

This comment has been minimized.

This comment has been minimized.

vvuk commented Dec 22, 2016

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 28, 2016

bors-servo commented Jan 4, 2017

kvark commented Jan 5, 2017 •

edited

glennw commented Jan 5, 2017

bors-servo commented Jan 5, 2017

bors-servo commented Jan 5, 2017

bors-servo commented Jan 5, 2017

servo / webrender

Join GitHub today

Lots of wrench fixes and features #660

Lots of wrench fixes and features #660

Conversation

vvuk commented Dec 22, 2016 • edited by larsbergstrom

glennw left a comment

This comment has been minimized.

This comment has been minimized.

vvuk commented Dec 22, 2016

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 28, 2016

bors-servo commented Jan 4, 2017

kvark commented Jan 5, 2017 • edited

glennw commented Jan 5, 2017

bors-servo commented Jan 5, 2017

bors-servo commented Jan 5, 2017

bors-servo commented Jan 5, 2017

vvuk commented Dec 22, 2016 •

edited by larsbergstrom

kvark commented Jan 5, 2017 •

edited