Bug 1778013 - Remove ClipId-style parenting from image clip masks public API r=gfx-reviewers,lsalzman

Also make the parent in ClipTemplate an Option, so that the
semantics are a bit clearer (follow up patches will remove this
parent field entirely).

Differential Revision: https://phabricator.services.mozilla.com/D150980

gfx/webrender_bindings/WebRenderAPI.cpp

@@ -1043,7 +1043,7 @@ wr::WrClipId DisplayListBuilder::DefineImageMaskClip(
   CancelGroup();
 
   WrClipId clipId = wr_dp_define_image_mask_clip_with_parent_clip_chain(
-      mWrState, &mCurrentSpaceAndClipChain, aMask, aPoints.Elements(),
+      mWrState, mCurrentSpaceAndClipChain.space, aMask, aPoints.Elements(),
       aPoints.Length(), aFillRule);
 
   return clipId;

gfx/webrender_bindings/src/bindings.rs

@@ -2661,7 +2661,7 @@ pub extern "C" fn wr_dp_define_clipchain(
 #[no_mangle]
 pub extern "C" fn wr_dp_define_image_mask_clip_with_parent_clip_chain(
     state: &mut WrState,
-    parent: &WrSpaceAndClipChain,
+    space: WrSpatialId,
     mask: ImageMask,
     points: *const LayoutPoint,
     point_count: usize,
@@ -2673,7 +2673,7 @@ pub extern "C" fn wr_dp_define_image_mask_clip_with_parent_clip_chain(
     let points: Vec<LayoutPoint> = c_points.iter().copied().collect();
 
     let clip_id = state.frame_builder.dl_builder.define_clip_image_mask(
-        &parent.to_webrender(state.pipeline_id),
+        space.to_webrender(state.pipeline_id),
         mask,
         &points,
         fill_rule,

gfx/wr/examples/basic.rs

@@ -68,7 +68,7 @@ impl Example for App {
             ClipMode::Clip
         );
         let mask_clip_id = builder.define_clip_image_mask(
-            &root_space_and_clip,
+            root_space_and_clip.spatial_id,
             mask,
             &vec![],
             FillRule::Nonzero,

gfx/wr/webrender/src/clip.rs

@@ -161,7 +161,7 @@ pub struct SceneClipInstance {
 #[cfg_attr(feature = "capture", derive(Serialize))]
 pub struct ClipTemplate {
     /// Parent of this clip, in terms of the public clip API
-    pub parent: ClipId,
+    pub parent: Option<ClipId>,
     /// Range of instances that define this clip template
     pub clips: ops::Range<u32>,
 }
@@ -266,19 +266,26 @@ impl ClipChainBuilder {
             clip_chain_id = new_clip_chain_id;
         }
 
-        // The ClipId parenting is terminated when we reach the root ClipId
-        if clip_id == template.parent {
-            return clip_chain_id;
-        }
+        match template.parent {
+            Some(parent) => {
+                // The ClipId parenting is terminated when we reach the root ClipId
+                if clip_id == parent {
+                    return clip_chain_id;
+                }
 
-        ClipChainBuilder::add_new_clips_to_chain(
-            template.parent,
-            clip_chain_id,
-            existing_clips,
-            clip_chain_nodes,
-            templates,
-            clip_instances,
-        )
+                ClipChainBuilder::add_new_clips_to_chain(
+                    parent,
+                    clip_chain_id,
+                    existing_clips,
+                    clip_chain_nodes,
+                    templates,
+                    clip_instances,
+                )
+            }
+            None => {
+                clip_chain_id
+            }
+        }
     }
 
     /// Return true if any of the clips in the hierarchy from clip_id to the
@@ -301,17 +308,24 @@ impl ClipChainBuilder {
             }
         }
 
-        // The ClipId parenting is terminated when we reach the root ClipId
-        if clip_id == template.parent {
-            return false;
-        }
+        match template.parent {
+            Some(parent) => {
+                // The ClipId parenting is terminated when we reach the root ClipId
+                if clip_id == parent {
+                    return false;
+                }
 
-        // Recurse into parent clip template to also check those
-        self.has_complex_clips(
-            template.parent,
-            templates,
-            instances,
-        )
+                // Recurse into parent clip template to also check those
+                self.has_complex_clips(
+                    parent,
+                    templates,
+                    instances,
+                )
+            }
+            None => {
+                false
+            }
+        }
     }
 
     /// This is the main method used to get a clip chain for a primitive. Given a
@@ -1036,7 +1050,7 @@ impl ClipStore {
     pub fn register_clip_template(
         &mut self,
         clip_id: ClipId,
-        parent: ClipId,
+        parent: Option<ClipId>,
         clips: &[SceneClipInstance],
     ) {
         let start = self.instances.len() as u32;

gfx/wr/webrender/src/hit_test.rs

@@ -494,13 +494,15 @@ fn add_clips(
     }
 
     // The ClipId parenting is terminated when we reach the root ClipId
-    if clip_id != template.parent {
-        add_clips(
-            template.parent,
-            clip_store,
-            clip_nodes,
-            seen_clips,
-            interners,
-        );
+    if let Some(parent) = template.parent {
+        if clip_id != parent {
+            add_clips(
+                parent,
+                clip_store,
+                clip_nodes,
+                seen_clips,
+                interners,
+            );
+        }
     }
 }

gfx/wr/webrender/src/scene_building.rs

@@ -767,12 +767,12 @@ impl<'a> SceneBuilder<'a> {
         let invalid_clip_chain_id = ClipId::ClipChain(api::ClipChainId::INVALID);
         self.clip_store.register_clip_template(
             invalid_clip_chain_id,
-            invalid_clip_chain_id,
+            None,
             &[],
         );
 
         let root_clip_id = ClipId::root(root_pipeline.pipeline_id);
-        self.clip_store.register_clip_template(root_clip_id, root_clip_id, &[]);
+        self.clip_store.register_clip_template(root_clip_id, None, &[]);
         self.clip_store.push_clip_root(Some(root_clip_id), false);
         self.id_to_index_mapper_stack.push(NodeIdToIndexMapper::default());
 
@@ -884,7 +884,7 @@ impl<'a> SceneBuilder<'a> {
                         continue 'outer;
                     }
                     _ => {
-                        self.build_item(item, bc.pipeline_id);
+                        self.build_item(item);
                     }
                 };
             }
@@ -1248,7 +1248,6 @@ impl<'a> SceneBuilder<'a> {
     fn build_item<'b>(
         &'b mut self,
         item: DisplayItemRef,
-        pipeline_id: PipelineId,
     ) {
         match *item.item() {
             DisplayItem::Image(ref info) => {
@@ -1678,7 +1677,7 @@ impl<'a> SceneBuilder<'a> {
 
                 self.add_image_mask_clip_node(
                     info.id,
-                    &info.parent_space_and_clip,
+                    info.spatial_id,
                     &info.image_mask,
                     info.fill_rule,
                     item.points(),
@@ -1705,7 +1704,7 @@ impl<'a> SceneBuilder<'a> {
             DisplayItem::ClipChain(ref info) => {
                 profile_scope!("clip_chain");
 
-                let parent = info.parent.map_or(ClipId::root(pipeline_id), |id| ClipId::ClipChain(id));
+                let parent = info.parent.map(|id| ClipId::ClipChain(id));
                 let mut clips: SmallVec<[SceneClipInstance; 4]> = SmallVec::new();
 
                 for clip_item in item.clip_chain_items() {
@@ -2680,12 +2679,12 @@ impl<'a> SceneBuilder<'a> {
     fn add_image_mask_clip_node(
         &mut self,
         new_node_id: ClipId,
-        space_and_clip: &SpaceAndClipInfo,
+        spatial_id: SpatialId,
         image_mask: &ImageMask,
         fill_rule: FillRule,
         points_range: ItemRange<LayoutPoint>,
     ) {
-        let spatial_node_index = self.get_space(space_and_clip.spatial_id);
+        let spatial_node_index = self.get_space(spatial_id);
 
         let snapped_mask_rect = self.snap_rect(
             &image_mask.rect,
@@ -2727,7 +2726,7 @@ impl<'a> SceneBuilder<'a> {
 
         self.clip_store.register_clip_template(
             new_node_id,
-            space_and_clip.clip_id,
+            None,
             &[instance],
         );
     }
@@ -2767,7 +2766,7 @@ impl<'a> SceneBuilder<'a> {
 
         self.clip_store.register_clip_template(
             new_node_id,
-            space_and_clip.clip_id,
+            Some(space_and_clip.clip_id),
             &[instance],
         );
     }
@@ -2810,7 +2809,7 @@ impl<'a> SceneBuilder<'a> {
 
         self.clip_store.register_clip_template(
             new_node_id,
-            space_and_clip.clip_id,
+            Some(space_and_clip.clip_id),
             &[instance],
         );
     }

gfx/wr/webrender_api/src/display_item.rs

@@ -239,7 +239,7 @@ pub enum DebugDisplayItem {
 #[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Serialize, PeekPoke)]
 pub struct ImageMaskClipDisplayItem {
     pub id: ClipId,
-    pub parent_space_and_clip: SpaceAndClipInfo,
+    pub spatial_id: SpatialId,
     pub image_mask: ImageMask,
     pub fill_rule: FillRule,
 } // IMPLICIT points: Vec<LayoutPoint>

gfx/wr/webrender_api/src/display_list.rs

@@ -1936,14 +1936,14 @@ impl DisplayListBuilder {
 
     pub fn define_clip_image_mask(
         &mut self,
-        parent_space_and_clip: &di::SpaceAndClipInfo,
+        spatial_id: di::SpatialId,
         image_mask: di::ImageMask,
         points: &[LayoutPoint],
         fill_rule: di::FillRule,
     ) -> di::ClipId {
         let id = self.generate_clip_index();
 
-        let current_offset = self.current_offset(parent_space_and_clip.spatial_id);
+        let current_offset = self.current_offset(spatial_id);
 
         let image_mask = di::ImageMask {
             rect: image_mask.rect.translate(current_offset),
@@ -1952,7 +1952,7 @@ impl DisplayListBuilder {
 
         let item = di::DisplayItem::ImageMaskClip(di::ImageMaskClipDisplayItem {
             id,
-            parent_space_and_clip: *parent_space_and_clip,
+            spatial_id,
             image_mask,
             fill_rule,
         });

gfx/wr/wrench/src/yaml_frame_reader.rs

@@ -1904,7 +1904,7 @@ impl YamlFrameReader {
 
         if let Some(image_mask) = self.as_image_mask(&yaml["image-mask"], wrench) {
             space_and_clip.clip_id = dl.define_clip_image_mask(
-                &space_and_clip,
+                space_and_clip.spatial_id,
                 image_mask,
                 &[],
                 FillRule::Nonzero,