Flatten the display list in the API #552

mrobinson · 2016-11-10T14:48:17Z

This changes brings the WebRender API more into line with the Servo
display list and also the frames generated inside WebRender itself.
Most intermediate display list representations are removed as well as
their place on the resource cache. Perhaps most notably, stacking
contexts are started via a PushStackingContext display item and ended
via a PopStackingContext item. Additionally, the flatten pass has been
significantly simplified to account for these changes.

This change is

mrobinson · 2016-11-10T14:48:25Z

@glennw r?

mrobinson · 2016-11-10T14:52:04Z

The Servo part of this patch is here, which can be ready to go soon after this lands: https://github.com/mrobinson/servo/tree/display-list

bors-servo · 2016-11-10T19:12:57Z

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

glennw · 2016-11-10T20:34:04Z

@mrobinson Wow, this is awesome! Will review today :)

emilio · 2016-11-10T21:54:38Z

@mrobinson I think you'll need to update the lockfiles, and potentially the callers in the other crates I guess, Travis is complaining :(

glennw · 2016-11-11T03:43:31Z

@mrobinson I've had a reasonable look over this - it looks great! I ran all the WPT tests on it locally, and I get the failures below. Once those are fixed up we'll try to finish up the review and get it merged before it bitrots too badly.


  ▶ FAIL [expected PASS] /_mozilla/css/iframe/hide_and_show.html
  └   → /_mozilla/css/iframe/hide_and_show.html 1b322aa260e8efcc96823b3616dec4048e764fd7
/_mozilla/css/iframe/hide_and_show_ref.html a588759663fe2c5184aba85f7ea89f48a5c2b38f
Testing 1b322aa260e8efcc96823b3616dec4048e764fd7 == a588759663fe2c5184aba85f7ea89f48a5c2b38f

  ▶ FAIL [expected PASS] /_mozilla/css/stacked_layers.html
  └   → /_mozilla/css/stacked_layers.html 47267c953cbf670c595a258334dff478f5c24369
/_mozilla/css/stacked_layers_ref.html 6955d0a11b580828a5e9a47926e22b577c48ee5f
Testing 47267c953cbf670c595a258334dff478f5c24369 == 6955d0a11b580828a5e9a47926e22b577c48ee5f

mrobinson · 2016-11-11T09:12:55Z

@glennw Ack! Thanks for taking a look. I'll try to get the ref tests sorted today. Sorry for letting those slip by.

mrobinson · 2016-11-11T14:20:02Z

@glennw Okay. Sorry for the failures. Things should be working now.

kvark

I don't know much about this area of WR, but the change looks good to me!

kvark · 2016-11-11T15:21:29Z

webrender/src/frame.rs

-            for item in &display_list.items {
-                result.push(item.clone());
-            }
+        match first_item.item {


nit: could combine these matches into one

Hrm. I was hoping to keep the nesting level a bit shallower to make it easier to read. Is there a way to do that while taking your suggestion?

That's really minor and doesn't need to be fixed! Could be done in a single match (like I specified in another comment), in nested matches, or even with and_then construct:

self.first().and_then(|item| match item.item {...})

Oh, that's a lot nicer! The latest version of the branch takes this suggestion.

kvark · 2016-11-11T15:22:22Z

webrender/src/frame.rs

@@ -190,6 +154,57 @@ impl StackingContextHelpers for StackingContext {
    }
 }

+pub struct DisplayListTraversal<'a> {
+    pub display_list: &'a Vec<DisplayItem>,


do these have to be public?

As a uber-nit: this can probably be &'a [DisplayItem] to save an extra level of indirection.

kvark · 2016-11-11T15:22:56Z

webrender/src/frame.rs

+
+    pub fn current_stacking_context_empty(&self) -> bool {
+        match self.peek() {
+            Some(item) if item.item == SpecificDisplayItem::PopStackingContext => true,


nit: could do Some(item) => item.item == SpecificDisplayItem::PopStackingContext,

Oh nice. Done.

kvark · 2016-11-11T15:25:49Z

webrender/src/frame.rs

+
+        let iframe_scroll_layer_id = iframe_stacking_context.scroll_layer_id.unwrap();
+        self.layers.insert(iframe_scroll_layer_id,
+                           Layer::new(&iframe_rect,


nit: looks like the same layer as above, could clone() it

Okay. I've made Layer derive from Clone and done this. It made things quite a bit simpler.

kvark · 2016-11-11T15:28:04Z

webrender/src/scene.rs

+
+impl DisplayListHelpers for Vec<DisplayItem> {
+    fn starting_stacking_context<'a>(&'a self) -> Option<&'a StackingContext> {
+        let first_item = match self.first() {


nit: same as the other implementation, could combine the two matches into one:

Some(SomethingSomething { item: SpecificDisplayItem::PushStackingContext(ref item), .. }) => ...

This changes brings the WebRender API more into line with the Servo display list and also the frames generated inside WebRender itself. Most intermediate display list representations are removed as well as their place on the resource cache. Perhaps most notably, stacking contexts are started via a PushStackingContext display item and ended via a PopStackingContext item. Additionally, the flatten pass has been significantly simplified to account for these changes.

glennw · 2016-11-14T00:06:26Z

@mrobinson r=me. I haven't merged this yet to minimize the amount of time that we have this merged in WR without the corresponding Servo PR. But feel free to merge this when the Servo PR is ready to go!

mrobinson · 2016-11-14T07:52:34Z

@bors-servo r=glennw

bors-servo · 2016-11-14T07:52:35Z

📌 Commit 3b139d2 has been approved by glennw

bors-servo · 2016-11-14T07:52:37Z

⌛ Testing commit 3b139d2 with merge 087516e...

Flatten the display list in the API This changes brings the WebRender API more into line with the Servo display list and also the frames generated inside WebRender itself. Most intermediate display list representations are removed as well as their place on the resource cache. Perhaps most notably, stacking contexts are started via a PushStackingContext display item and ended via a PopStackingContext item. Additionally, the flatten pass has been significantly simplified to account for these changes.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/552)

mrobinson · 2016-11-14T07:52:45Z

@glennw Thanks. I'll get the Servo bit up ASAP.

bors-servo · 2016-11-14T08:02:39Z

☀️ Test successful - status-travis

mrobinson force-pushed the display-list branch from 9263903 to ccda8c1 Compare November 10, 2016 19:55

mrobinson force-pushed the display-list branch from ccda8c1 to dadb5c0 Compare November 11, 2016 13:46

kvark approved these changes Nov 11, 2016

View reviewed changes

mrobinson force-pushed the display-list branch from ffaa571 to 619f185 Compare November 11, 2016 16:18

mrobinson added 2 commits November 13, 2016 02:39

Increment minor version after to the API change

3b139d2

mrobinson force-pushed the display-list branch from 619f185 to 3b139d2 Compare November 13, 2016 01:40

bors-servo merged commit 3b139d2 into servo:master Nov 14, 2016

mrobinson deleted the display-list branch November 14, 2016 08:02

This was referenced Nov 14, 2016

[WIP] Add support for externally allocated images in the render backend #548

Closed

Off-screen clip mask generation #556

Merged

Introduce SourceTexture, an abstraction for location of source textures. #554

Merged

Implement Home end key scroll #540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten the display list in the API #552

Flatten the display list in the API #552

mrobinson commented Nov 10, 2016 •

edited by larsbergstrom

Loading

mrobinson commented Nov 10, 2016

mrobinson commented Nov 10, 2016

bors-servo commented Nov 10, 2016

glennw commented Nov 10, 2016

emilio commented Nov 10, 2016

glennw commented Nov 11, 2016

mrobinson commented Nov 11, 2016

mrobinson commented Nov 11, 2016

kvark left a comment

kvark Nov 11, 2016

mrobinson Nov 11, 2016

kvark Nov 11, 2016

mrobinson Nov 13, 2016

kvark Nov 11, 2016

emilio Nov 11, 2016

mrobinson Nov 11, 2016

kvark Nov 11, 2016

mrobinson Nov 11, 2016

kvark Nov 11, 2016

mrobinson Nov 11, 2016

kvark Nov 11, 2016

glennw commented Nov 14, 2016

mrobinson commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

mrobinson commented Nov 14, 2016

bors-servo commented Nov 14, 2016

Flatten the display list in the API #552

Flatten the display list in the API #552

Conversation

mrobinson commented Nov 10, 2016 • edited by larsbergstrom Loading

mrobinson commented Nov 10, 2016

mrobinson commented Nov 10, 2016

bors-servo commented Nov 10, 2016

glennw commented Nov 10, 2016

emilio commented Nov 10, 2016

glennw commented Nov 11, 2016

mrobinson commented Nov 11, 2016

mrobinson commented Nov 11, 2016

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennw commented Nov 14, 2016

mrobinson commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

mrobinson commented Nov 14, 2016

bors-servo commented Nov 14, 2016

mrobinson commented Nov 10, 2016 •

edited by larsbergstrom

Loading