Remove external scroll ids from ClipId

[mrobinson]

Move the concept of external scroll ids to ScrollFrames themselves. It
was probably a mistake to integrate this into ClipId in the first place.
Moving it out allows us to simplify a few code paths and will allow us
to remove user defined ClipIds entirely, making things simpler and more
consistent. This will also allow fixing Servo bugs servo/servo#19287,
servo/servo#17176, and servo/servo#19648.

---

This change is 

[staktrace]

Please don't merge this until we have the gecko-side patch to go with it, as by itself it will block WR updating into gecko. Right now I'm backlogged with regressions and other stuff so I won't be able to get to writing one until next week at least. (Feel free to write it yourself though)

[mrobinson]

@staktrace Okay. I'll try to get the Gecko patch ready tomorrow. The WebRender bit is done (just needs cleanup and a bit more testing) and is here: https://github.com/mrobinson/servo/tree/remove-external-clip-id

[bors-servo]

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

[mrobinson]

I think it makes sense to include in this PR the removal of specifying external ClipIds completely. I'll do this as well as finish the Gecko bits of this change.

[mrobinson]

Here's the Gecko try job which includes the Gecko changes for this PR: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f31bc33d18361f78b041620f0dd174edd19e3e6

@staktrace, what is the best way to provide the Gecko patch?

[staktrace]

The try push is fine. I'll put out the gecko stuff and apply it to my local queue from where I do WR update try pushes, and then eventually that patch will go onto bug 1432789 as part of the patch queue for the next WR update. I'll keep you as the patch author and flag myself for review when the patches go up.

[staktrace]

(Just to be clear the gecko stuff is good now - thanks for the patch - so feel free to merge this whenever you're ready)

[glennw]

@mrobinson Sorry, I didn't realize this one was ready for review. Will review first thing in the morning!

[kvark]

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

---

webrender/src/clip_scroll_tree.rs, line 259 at r1 (raw file):

            match old_node.node_type {
                NodeType::ScrollFrame(info) if info.external_id.is_some() => {

could be shorter in order to avoid is_some() followed by unwrap():
NodeType::ScrollFrame(ScrollFrameInfo{ external_id: Some(id), .. }) => {...}
should probably be turned into if let as well

---

webrender/src/clip_scroll_tree.rs, line 490 at r1 (raw file):

        for (clip_id, node) in &mut self.nodes {
            let external_id = match node.node_type {
                NodeType::ScrollFrame(info) if info.external_id.is_some() => info.external_id,

is_some check is not needed here

---

webrender/src/clip_scroll_tree.rs, line 505 at r1 (raw file):

            }

            if let Some(external_id) = external_id {

can this be combined with the earlier check for external_id? if yes, it might be easier to go with if let again

---

webrender/src/clip_scroll_tree.rs, line 508 at r1 (raw file):

                let id = IdType::ExternalScrollId(external_id);
                if let Some((offset, clamping)) = self.pending_scroll_offsets.remove(&id) {
                    node.set_scroll_origin(&offset, clamping);

what happens if the node has pending scroll offsets for both ClipId and ExternalScrollId?

---

webrender_api/src/api.rs, line 395 at r1 (raw file):

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
pub enum IdType {

Perhaps, this name is too generic?

---

webrender_api/src/api.rs, line 397 at r1 (raw file):

pub enum IdType {
    ExternalScrollId(ExternalScrollId),
    ClipId(ClipId),

we could implement From<ClipId> and From<ExternalScrollId> for it

---

Comments from Reviewable

[glennw]

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

webrender_api/src/api.rs, line 395 at r1 (raw file):

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
pub enum IdType {

Maybe ClipScrollIdType ?

---

Comments from Reviewable

[glennw]

Looks good to me, once the comments from @kvark are addressed. Thanks!

[bors-servo]

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

[mrobinson]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

webrender/src/clip_scroll_tree.rs, line 259 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could be shorter in order to avoid is_some() followed by unwrap():
NodeType::ScrollFrame(ScrollFrameInfo{ external_id: Some(id), .. }) => {...}
should probably be turned into if let as well

I gave this a shot, but it ultimately didn't work because I need info and info.external_id inside the body of the match. I'm not sure of a way to do the deep pattern match while still maintaining a variable with info in it.

---

webrender/src/clip_scroll_tree.rs, line 490 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is_some check is not needed here

Nice catch!

---

webrender/src/clip_scroll_tree.rs, line 505 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can this be combined with the earlier check for external_id? if yes, it might be easier to go with if let again

It looks like we can combine these blocks by applying ExternalId pending scroll offsets before ClipId ones. Not sure how I missed this.

---

webrender/src/clip_scroll_tree.rs, line 508 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what happens if the node has pending scroll offsets for both ClipId and ExternalScrollId?

With these changes the ClipId version will take precedence. I think that a later change will make it possible to remove one of the two modes, so this situation should only be temporary.

---

webrender_api/src/api.rs, line 395 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Maybe ClipScrollIdType ?

I have gone with ScrollNodeIdType, because this should only apply to scrolling nodes. Hopefully we can remove this entirely in an upcoming PR once Gecko and Servo both use the same id type to scroll.

---

webrender_api/src/api.rs, line 397 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we could implement From<ClipId> and From<ExternalScrollId> for it

Sure. That makes the code a bit nicer.

---

Comments from Reviewable

[mrobinson]

@bors-servo r=glennw,kvark

[bors-servo]

📌 Commit aa91e78 has been approved by glennw,kvark

[bors-servo]

⌛ Testing commit aa91e78 with merge e772c3c...

[bors-servo]

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