Trim the render work done after a scene build

[staktrace]

Fixes #2807. I'm not totally sure that I'm including all the work that is necessary for hit-testing but it seems to pass my ad-hoc testing and CI. But still, please review with care to make sure I'm not violating assumptions or leaving behind inconsistent state by just doing the bits of the clip scroll tree update that don't touch the resource cache, gpu cache, and transform palette.

---

This change is 

[gw3583]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @staktrace)

---

webrender/src/frame_builder.rs, line 307 at r1 (raw file):

    pub fn update_clip_scroll_tree(
        &mut self,
        resource_cache: Option<&mut ResourceCache>,

Perhaps pass a ClipRenderContext here, rather than the gpu_cache.unwrap() etc below?

[gw3583]

Apart from the review comment above, I have a couple of questions:

• What are the situation(s) where we'd want to build the hit tester but not do a render anyway (for my own understanding, more than anything else). I vaguely recall complexities around needing to copy the hit-tester data at some point, but the details escape me.

• The bug being fixed refers to needing to call build_layer_screen_rects_and_cull_layers but not the batching and task generation of that code. However, it seems this patch only does the clip_scroll_tree.update, and not the culling code. There's a couple subtleties here I'm unsure of:

  • During build_layer_screen_rects_and_cull_layers we can optionally update the local rect of a primitive. I think we only do this for Picture types, and picture primitives should never have an ItemTag associated with them - so this shouldn't affect hit testing.
  • We only update the visibility of a primitive during build_layer_screen_rects_and_cull_layers. This includes checking whether a primitive is considered backface visible. Updating the clip-scroll tree may result in a change to whether a primitive is backface visible, but this patch would not update the hit-test code. I'm not actually sure what the specifications say regarding how hit testing interacts with backface visibility culling - is this relevant here?

If we can get away with just updating the clip scroll tree, that's great - it should be a decent win. However, if we need to also call build_layer_screen_rects_and_cull_layers but not do batching / task generation, this may be slightly more involved (and we might be better to consider refactoring how the visibility test code works, to make this simpler).

[bors-servo]

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

[staktrace]

What are the situation(s) where we'd want to build the hit tester but not do a render anyway (for my own understanding, more than anything else).

There are none - we want to rebuild the hit tester if we get a new scene or do an async scroll, and in both of those cases we want to do a render anyway. The issue is one of timing. The render that we do upon getting a new scene happens at the next vsync, triggered from one of these two callbacks. However in between the scene build happening and that next vsync, we would have an inconsistent hit-test state unless we rebuild the hit tester. So currently, we are doing two renders for each scene build - one is at the next vsync as linked above, and the other is triggered from here. That's the one that I'm trying to trim.

During build_layer_screen_rects_and_cull_layers we can optionally update the local rect of a primitive.

But do we actually use the local rect of a primitive when doing a hit-test? I was looking at the code in hit_test.rs and I didn't see anywhere we use that, it looks like a totally separate data structure. Maybe I missed something?

We only update the visibility of a primitive during build_layer_screen_rects_and_cull_layers. This includes checking whether a primitive is considered backface visible. Updating the clip-scroll tree may result in a change to whether a primitive is backface visible, but this patch would not update the hit-test code.

Likewise, from my reading of the hit-testing code, the backface visibility check uses the flag on the HitTestingItem which is copied from the LayoutPrimitiveInfo so I'm not sure how running build_layer_screen_rects_and_cull_layers would affect that. @mrobinson maybe you know - is calling this function necessary for a correct hit-test?

I'm not actually sure what the specifications say regarding how hit testing interacts with backface visibility culling - is this relevant here?

I'm not sure if there is actually a spec that covers hit-testing, but I believe that with respect to backface-visibility, the item is hit-testable if and only if it is visible.

[staktrace]

I updated the patches to address the review comment. And it looks like Martin isn't working on WR now so we'll have to figure this out without him :(

[bors-servo]

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

[nical]

I rebased this PR in this branch. I'll do some testing before submitting the pull request.

try-push with talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f0a0093d96456c101aeced4d78f5f6077869a1d

[nical]

Replaced by #3052