Add support for API-defined ClipChains

[mrobinson]

API-defined clip chains allow constructing combinations of clips that
select any combination of clips from the ClipScrollTree without regard
to parents of clips. A ClipChain is defined by an optional ClipChain
parent and a vector of clips to use for that chain. A chain's clips are
combined with its parents clips to provide all the clips for display
items that set their clipping ClipId as the clip chain via
ClipId::ClipChain.

Fixes #1964.

---

This change is 

[mrobinson]

cc @staktrace

[mrobinson]

Here is the Gecko try run for this PR: https://treeherder.mozilla.org/#/jobs?repo=try&revision=192ed24cbbacbd7fd2807f3fc06e797377dee5f3

[glennw]

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

wrench/reftests/clip/custom-clip-chains.yaml, line 1 at r1 (raw file):

root:

Could we add a comment here describing what this is testing for?

---

Comments from Reviewable

[glennw]

Just one minor comment in reviewable, otherwise looks good to me!

[glennw]

@staktrace Do you want to take a look at the API here before it gets merged?

[mrobinson]

Review status: 10 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

wrench/reftests/clip/custom-clip-chains.yaml, line 1 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Could we add a comment here describing what this is testing for?

Done.

---

Comments from Reviewable

[staktrace]

I think this should provide sufficient expressiveness to deal with all the scenarios that we need. Thanks!

[glennw]

@bors-servo r+

[bors-servo]

📌 Commit 000d7a9 has been approved by glennw

[bors-servo]

🔒 Merge conflict

[bors-servo]

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

[glennw]

Needs a rebase.

[mrobinson]

@bors-servo r=glennw

[bors-servo]

📌 Commit f43028a has been approved by glennw

[bors-servo]

⌛ Testing commit f43028a with merge eb9e170...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing eb9e170 to master...

[kvark]

Reviewed 5 of 11 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

webrender/src/clip_scroll_tree.rs, line 470 at r2 (raw file):

            for clip_id in &descriptor.clips {
                if let Some(ref node_chain) = self.nodes[&clip_id].clip_chain {
                    if let Some(ref nodes) = node_chain.nodes {

nit: can do a within a parent pattern match

---

webrender/src/clip_scroll_tree.rs, line 476 at r2 (raw file):

            }

            self.clip_chains.insert(descriptor.id, chain);

I don't clearly understand what is going on here. We take chain as the parent clip chain of the descriptor. We populate the child nodes from descriptor.clips. But then we are inserting it as descriptor.id?

Ah, I guess it's actually simple: Current clip chain = Parent clip chain + all the current clip nodes. Makes sense now :) Perhaps, we could have a comment on that function?

Could we re-use the allocations within self.clip_chains[descriptor.id] instead of throwing it out?

---

webrender/src/clip_scroll_tree.rs, line 663 at r2 (raw file):

        match id {
            &ClipId::ClipChain(clip_chain_id) => Some(&self.clip_chains[&clip_chain_id]),
            _ => self.nodes[id].clip_chain.as_ref(),

do nodes contain ClipId::ClipChain items? If yes, can we just have this path followed here universally (perhaps, with a debug assert that the clip chain matches the one referred by clip_chain_id).

---

webrender_api/src/display_item.rs, line 435 at r2 (raw file):

    pub id: ClipChainId,
    pub parent: Option<ClipChainId>,
} // IMPLICIT stops: Vec<ClipId>

we can use stops: PhantomData<Vec<Clipid>>,

---

webrender_api/src/display_list.rs, line 1350 at r2 (raw file):

    fn generate_clip_chain_id(&mut self) -> ClipChainId {
        self.next_clip_chain_id += 1;
        ClipChainId(self.next_clip_chain_id - 1, self.pipeline_id)

nit: could probably not subtracting 1 here

---

Comments from Reviewable

[mrobinson]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

webrender/src/clip_scroll_tree.rs, line 470 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: can do a within a parent pattern match

I'm not sure how to do this without running past the end of the line...

---

webrender/src/clip_scroll_tree.rs, line 476 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't clearly understand what is going on here. We take chain as the parent clip chain of the descriptor. We populate the child nodes from descriptor.clips. But then we are inserting it as descriptor.id?

Ah, I guess it's actually simple: Current clip chain = Parent clip chain + all the current clip nodes. Makes sense now :) Perhaps, we could have a comment on that function?

Could we re-use the allocations within self.clip_chains[descriptor.id] instead of throwing it out?

I'm not sure exactly how to reuse the allocation here, but I will add a comment explaining what is going on. We do need to clone the parent node, but we are just cloning an Rc of the node.

---

webrender/src/clip_scroll_tree.rs, line 663 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

do nodes contain ClipId::ClipChain items? If yes, can we just have this path followed here universally (perhaps, with a debug assert that the clip chain matches the one referred by clip_chain_id).

Eventually, it might make sense to have all ClipChains represented by ClipChainDescriptors, but for the moment it is hard to unify the two code paths. ClipChainNode::update_clip_work_item does a few extra optimizations that only make sense for ClipScrollNodes.

---

webrender_api/src/display_item.rs, line 435 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we can use stops: PhantomData<Vec<Clipid>>,

Hrm. Does it make sense to change all of the // IMPLICIT lines together in a separate patch?

---

webrender_api/src/display_list.rs, line 1350 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could probably not subtracting 1 here

Is there a good way to do this without keeping an extra variable to record the previous number or the new clip id?

---

Comments from Reviewable

[mrobinson]

Sorry for taking so long to reply to these comments. I have replied above. I have a patch ready with the new comments which I'll post soon.

[kvark]

There is no ability to continue the review here, since the PR is merged and branch is deleted.

Hrm. Does it make sense to change all of the // IMPLICIT lines together in a separate patch?

Yep.