Bug 1616422 - Part 1: Refactor WR display item caching and add additional checks r=jrmuizel

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

--HG--
extra : moz-landing-system : lando

gfx/wr/webrender_api/src/display_item_cache.rs

@@ -5,6 +5,9 @@
 use crate::display_item::*;
 use crate::display_list::*;
 
+#[cfg(debug_assertions)]
+use std::collections::HashSet;
+
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 pub struct CachedDisplayItem {
     item: DisplayItem,
@@ -30,19 +33,33 @@ impl From<DisplayItemRef<'_, '_>> for CachedDisplayItem {
                 item: *item,
                 data: item_ref.glyphs().bytes().to_vec(),
             },
-            DisplayItem::Rectangle(..) |
-            DisplayItem::Image(..) => CachedDisplayItem {
+            _ => CachedDisplayItem {
                 item: *item,
                 data: Vec::new(),
             },
-            _ => { unimplemented!("Unsupported display item type"); }
         }
     }
 }
 
+fn key_from_item(item: &DisplayItem) -> ItemKey {
+    let key = match item {
+        DisplayItem::Rectangle(ref info) => info.common.item_key,
+        DisplayItem::ClearRectangle(ref info) => info.common.item_key,
+        DisplayItem::HitTest(ref info) => info.common.item_key,
+        DisplayItem::Text(ref info) => info.common.item_key,
+        DisplayItem::Image(ref info) => info.common.item_key,
+        _ => unimplemented!("Unexpected item: {:?}", item)
+    };
+
+    key.expect("Cached item without a key")
+}
+
 #[derive(Clone, Deserialize, Serialize)]
 pub struct DisplayItemCache {
-    items: Vec<Option<CachedDisplayItem>>
+    items: Vec<Option<CachedDisplayItem>>,
+
+    #[cfg(debug_assertions)]
+    keys: HashSet<ItemKey>,
 }
 
 impl DisplayItemCache {
@@ -59,11 +76,10 @@ impl DisplayItemCache {
 
     fn add_item(
         &mut self,
-        key: Option<ItemKey>,
-        item: DisplayItemRef
+        item: CachedDisplayItem,
+        key: ItemKey,
     ) {
-        let index = usize::from(key.expect("Cached item without key"));
-        self.items[index] = Some(CachedDisplayItem::from(item));
+        self.items[key as usize] = Some(item);
     }
 
     pub fn get_item(
@@ -75,7 +91,11 @@ impl DisplayItemCache {
 
     pub fn new() -> Self {
         Self {
-            items: Vec::new()
+            items: Vec::new(),
+
+            #[cfg(debug_assertions)]
+            /// Used to check that there is only one item per key.
+            keys: HashSet::new(),
         }
     }
 
@@ -87,26 +107,26 @@ impl DisplayItemCache {
 
         let mut iter = display_list.extra_data_iter();
 
+        #[cfg(debug_assertions)]
+        {
+            self.keys.clear();
+        }
+
         loop {
             let item = match iter.next() {
                 Some(item) => item,
                 None => break,
             };
 
-            match item.item() {
-                DisplayItem::Rectangle(ref info) => {
-                    self.add_item(info.common.item_key, item);
-                }
-                DisplayItem::Text(ref info) => {
-                    self.add_item(info.common.item_key, item);
-                }
-                DisplayItem::Image(ref info) => {
-                    self.add_item(info.common.item_key, item);
-                }
-                item @ _ => {
-                    unimplemented!("Unexpected item in extra data: {:?}", item);
-                }
+            let item_key = key_from_item(item.item());
+            let cached_item = CachedDisplayItem::from(item);
+
+            #[cfg(debug_assertions)]
+            {
+                debug_assert!(self.keys.insert(item_key));
             }
+
+            self.add_item(cached_item, item_key);
         }
     }
 }

gfx/wr/webrender_api/src/display_list.rs

@@ -658,8 +658,8 @@ impl<'a> BuiltDisplayListIter<'a> {
     pub fn as_ref<'b>(&'b self) -> DisplayItemRef<'a, 'b> {
         let cached_item = match self.cur_item {
             di::DisplayItem::ReuseItem(key) => {
-                debug_assert!(self.cache.is_some(), "Cache marker without cache!");
-                self.cache.and_then(|c| c.get_item(key))
+                let cache = self.cache.expect("Cache marker without cache!");
+                cache.get_item(key)
             }
             _ => None
         };