Add support for hit testing

[mrobinson]

This adds a new API point for doing hit testing against the display
list. This version supports the features Servo needs to do hist
testing, except for text index support (which will come in a later PR).

Fixes #1575.

---

This change is 

[mrobinson]

cc @staktrace

[glennw]

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

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

        clip_store: &ClipStore
    ) -> bool {
        match cache.get(node_id) {

if let

---

webrender/src/ellipse.rs, line 78 at r1 (raw file):

    /// Find the signed distance from this ellipse given a point.
    /// Taken from http://www.iquilezles.org/www/articles/ellipsedist/ellipsedist.htm
    fn signed_distance(&self, point: LayerVector2D) -> f32 {

We should probably add some unit tests for this - as a low priority follow up.

---

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

        }

        result.items.dedup();

dedup() will only work if the items are sorted - that won't be the case here, right?

---

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

    pub pipeline: PipelineId,
    pub tag: ItemTag,
    pub point_in_viewport: LayoutPoint,

Could we add a comment here with more details on the coordinate space here?

---

webrender_api/src/display_item.rs, line 46 at r1 (raw file):

/// second is used to select the cursor that should be used during mouse
/// movement.
pub type ItemTag = (u64, u8);

The size of this tuple seems quite implementation specific. I imagine that struct alignment will mean there is probably 7 bytes of padding added to the type anyway (although I haven't checked this). Is there any way we could make this a single u64 (or 2x u32) and pack the tags a bit tighter on the Servo side?

---

Comments from Reviewable

[glennw]

This looks great! I added a few minor comments / fixups in reviewable.

Hit testing is something that regularly gets broken in Servo due to lack of test coverage. It would be great to find a way to auto test this stuff in wrench, although that can be done as a follow up to this initial patch.

[glennw]

Also - CI is failing, due to warnings as errors. So we'll need to fix those up before landing this too.

[bors-servo]

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

[mrobinson]

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

---

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

Previously, glennw (Glenn Watson) wrote…

if let

Done.

---

webrender/src/ellipse.rs, line 78 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

We should probably add some unit tests for this - as a low priority follow up.

Okay. I will create unit tests in a followup PR.

---

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

Previously, glennw (Glenn Watson) wrote…

dedup() will only work if the items are sorted - that won't be the case here, right?

This is to avoid having a bunch of repeated items with the same information (for instance the border and background of the same div). In this case, the items with equal tags will almost certainly be together (CSS stacking works per Flow and not per display item). Deduplicating here should leave one entry per Flow (if the cursor is equal).

---

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

Previously, glennw (Glenn Watson) wrote…

Could we add a comment here with more details on the coordinate space here?

Done.

---

webrender_api/src/display_item.rs, line 46 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

The size of this tuple seems quite implementation specific. I imagine that struct alignment will mean there is probably 7 bytes of padding added to the type anyway (although I haven't checked this). Is there any way we could make this a single u64 (or 2x u32) and pack the tags a bit tighter on the Servo side?

I took a look at this and having metadata stored in the layout process introduces a race condition. The WebRender thread is the only thread with display list information for all pipelines. If we need to make a second request to a pipeline's layout thread to fetch the metadata, conceivably the layout thread may have produced a new display list(s) in the meantime. If the ids that we use to locate the metadata are produced by a simple counter we are likely to produce an incorrect hit test result.

My idea here is to make a future patch which turns the ItemTag into a simple u32 index and allows the display list to include a side table of display item metadata. This metadata structure will be defined in some way by the client, but importantly, it will stay with the display list to avoid race conditions. The tag will be used to access the data from the side table and return it to the compositor.

If you are okay with this idea, I'd like to land this version and do the "fix" later.

---

Comments from Reviewable

[glennw]

@bors-servo r+

[bors-servo]

📌 Commit 79cba94 has been approved by glennw

[bors-servo]

⌛ Testing commit 79cba94 with merge 29bd2aa...

[bors-servo]

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