Improved Local Combined Clip Rectangle

[kvark]

This PR brings a number of improvements and clean ups related to clipping:

• new footprint calculation for reference frame's LCCR
• reworked rectangular region handling for ClipRegion
• clip regions are included into LCCR
• primitive bounding rect is computed with the clip stack taken into account
• clip stack gets filtered based on which clips are not trivial with regards to the current bounding box

TODO: testing, benchmarking, rebasing
Based on #1305

r? @glennw @mrobinson

---

This change is 

[glennw]

I started doing some testing on this, but it seems to be breaking the scrolling on HN. If you load up https://news.ycombinator.com in Servo with this patch, and scroll to the bottom - parts of the background (and text?) seem to get incorrectly clipped out.

[kvark]

I guess it might be related to the new NodeType, hence the breakage after rebase. Will investigate later tonight.

…

On Jun 13, 2017, at 18:58, Glenn Watson ***@***.***> wrote: I started doing some testing on this, but it seems to be breaking the scrolling on HN. If you load up https://news.ycombinator.com in Servo with this patch, and scroll to the bottom - parts of the background (and text?) seem to get incorrectly clipped out. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[glennw]

Reviewed 5 of 11 files at r1, 10 of 14 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

webrender/src/clip_scroll_node.rs, line 92 at r3 (raw file):

    /// This is in the coordinate system of the node origin.
    /// Precisely, it combines the local clipping rectangles of all the parent
    /// nodes on the way to the root, including those of `ClipRegion`.

This is now a conservative local-space rect, is that right?

---

webrender/src/mask_cache.rs, line 144 at r3 (raw file):

                ClipSource::Complex(..) => {
                    complex_clip_count += 1;
                },

nit: extra comma

---

webrender/src/mask_cache.rs, line 307 at r3 (raw file):

    }

    /// Check if this `MaskCacheInfo` actually carries any masks, excluding the layer rectangles.

This comment doesn't seem to match the code below, which does check the layer clip range?

---

webrender/src/prim_store.rs, line 934 at r3 (raw file):

        if let Some(ref mut clip_info) = metadata.clip_cache_info {
            let bounds = clip_info.update(&metadata.clips,

This seems to be unused.

---

webrender/src/render_backend.rs, line 71 at r3 (raw file):

    vr_compositor_handler: Arc<Mutex<Option<Box<VRCompositorHandler>>>>,

    // A helper gate to prevent any frames rendering triggered by scrolling

Perhaps expand on what problem this causes if it occurs?

---

Comments from Reviewable

[glennw]

Wow, this is complex stuff! It looks good to me though, apart from the few minor issues above.

I've done a bit of local testing - manual page tests seem fine, and the test timings seem as good or better. I did hit a few panics in the CSS tests below - from the test names, I'm guessing we're not correctly handling cases with non-invertible matrices, perhaps?

/css-transforms-1_dev/html/transform3d-scale-004.htm
/css-transforms-1_dev/html/transform-singular-001.htm
/css-transforms-1_dev/html/scale-zero-001.htm
/css-transforms-1_dev/html/css-scale-nested-001.htm

It'd also be good for @mrobinson to take a look over this and see if he can spot anything.

[glennw]

@mrobinson @kvark We should definitely find a few hours in SF to sit down and talk about clips, masks, scroll nodes etc and work out what the next steps are for this part of the code :)

[kvark]

Thanks for the review, @glennw!
All comments are addressed, CSS tests are passing now, plus some icing on the cake:

PASS [expected FAIL] /css-transforms-1_dev/html/transform-table-006.htm

---

Review status: 6 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

webrender/src/clip_scroll_node.rs, line 92 at r3 (raw file):

Previously, glennw (Glenn Watson) wrote…

This is now a conservative local-space rect, is that right?

extended the comment, hopefully it's more clear now

---

Comments from Reviewable

[kvark]

Hmm, css-transforms1-dev passed but test-wpt has some failures now:

▶ FAIL [expected PASS] /_mozilla/css/iframe/simple_inline_height.html

Looking...

Edit: appears to be (harmlessly?) intermittent, similar to famous hide_and_show.html

[glennw]

@bors-servo r+

🚢

[bors-servo]

📌 Commit 22b7072 has been approved by glennw

[bors-servo]

⌛ Testing commit 22b7072 with merge bb36847...

[bors-servo]

☀️ Test successful - status-travis
Approved by: glennw
Pushing bb36847 to master...

[staktrace]

This PR regressed the dom/html/reftests/bug1228601-video-rotation-90.html reftest in gecko. Reftest analyzer link. It might be the same problem as #1408, not sure.

[kvark]

@staktrace very possible. I'm looking at it.

[staktrace]

@kvark FYI the reftest failure seems to have gone away with #1412 (although now there are a whole bunch of other reftest failures, so it might be that fixing those will restore this failure, not sure)

[staktrace]

I did a one-line revert of a change in #1412 which fixed the async-scrolling reftest failures. The reftest failure from this PR was not restored as a result, so that's good.