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.

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

Recurse to children of empty ClipScrollNodes #2249

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Recurse to children of empty ClipScrollNodes

This is important in order to clean up stale data during scrolling and
fixes a crash in Gecko. There is no test for this, because wrench does
not have the ability to easily test scrolling animation.
  • Loading branch information
mrobinson committed Jan 2, 2018
commit 95c5bbef82fcbd0565ec7e515dcd2c162f8ad07c
@@ -417,17 +417,18 @@ impl ClipScrollTree {

node.push_gpu_node_data(gpu_node_data);

if node.children.is_empty() || node.combined_clip_outer_bounds.is_empty() {

This comment has been minimized.

@kvark

kvark Jan 3, 2018

Member

so why can we not check for the outer bounds here?

This comment has been minimized.

@mrobinson

mrobinson Jan 3, 2018

Author Member

We need to keep recursing even in this case in order to properly set the GPU node data for all child nodes (otherwise it might be stale). Additionally, once we support non-hierarchical clip chains, this optimization has to be removed or reworked.

This comment has been minimized.

@kvark

kvark Jan 3, 2018

Member

Aren't child nodes going to be invisible anyway if the outer bounds are empty?

This comment has been minimized.

@mrobinson

mrobinson Jan 4, 2018

Author Member

Yes. The issue here is when they were previously marked as visible, we need to now recurse to mark them as invisible. If we do not, the other bits of WebRender will believe they are still visible, but they won't have any corresponding GPU data.

I think there's still a lot of adjustment we can do here. For instance, as I previously mentioned, we should further decouple the idea of a clip node and a clip chain in order to support complicated clipping situations. I'm working on this now. Additionally, the per-node GPU data is only holding transform information at the moment, so it makes sense that we only push this for nodes that adjust the transform (positioning nodes, but not clipping nodes).

These two things, along with allowing us to fix some Gecko issues, will also allow us to one day separate the clipping and scrolling/positioning trees completely.

return
if node.children.is_empty() {
return;
}


node.prepare_state_for_children(&mut state);
node.children.clone()
};

for child_layer_id in node_children {
for child_node_id in node_children {
self.update_node(
child_layer_id,
child_node_id,
&mut state,
next_coordinate_system_id,
device_pixel_scale,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.