Bug 1395212 - Hoist the scroll layer deduplication code out of bindings.rs into wr::DisplayListBuilder. r=mstange

This also splits the wr_dp_push_scroll_layer function in bindings.rs into two
separate functions. This makes the API consistent with clipping, and also allows
for optimizations in the upcoming patches.

MozReview-Commit-ID: IXnOZK0dZm

--HG--
extra : rebase_source : aa28875433a03ee9d6c388750f022958958d05e9

gfx/layers/wr/ScrollingLayersHelper.cpp

@@ -177,7 +177,7 @@ ScrollingLayersHelper::DefineAndPushScrollLayers(nsDisplayItem* aItem,
   DefineAndPushChain(asrClippedBy, aBuilder, aStackingContext,
       aAppUnitsPerDevPixel, aCache);
   // Finally, push the ASR itself as a scroll layer. Note that the
-  // implementation of wr_push_scroll_layer in bindings.rs makes sure the
+  // implementation of PushScrollLayer in DisplayListBuilder makes sure the
   // scroll layer doesn't get defined multiple times so we don't need to worry
   // about that here.
   if (PushScrollLayer(metadata->GetMetrics(), aStackingContext)) {

gfx/webrender_bindings/WebRenderAPI.cpp

@@ -685,14 +685,21 @@ DisplayListBuilder::PushScrollLayer(const layers::FrameMetrics::ViewID& aScrollI
 {
   WRDL_LOG("PushScrollLayer id=%" PRIu64 " co=%s cl=%s\n", mWrState,
       aScrollId, Stringify(aContentRect).c_str(), Stringify(aClipRect).c_str());
-  wr_dp_push_scroll_layer(mWrState, aScrollId, aContentRect, aClipRect);
+
-  if (!mScrollIdStack.empty()) {
+  Maybe<layers::FrameMetrics::ViewID> parent =
-    auto it = mScrollParents.insert({aScrollId, mScrollIdStack.back()});
+      mScrollIdStack.empty() ? Nothing() : Some(mScrollIdStack.back());
-    if (!it.second) { // aScrollId was already a key in mScrollParents
+  auto it = mScrollParents.insert({aScrollId, parent});
-                      // so check that the parent value is the same.
+  if (it.second) {
-      MOZ_ASSERT(it.first->second == mScrollIdStack.back());
+    // An insertion took place, which means we haven't defined aScrollId before.
-    }
+    // So let's define it now.
+    wr_dp_define_scroll_layer(mWrState, aScrollId, aContentRect, aClipRect);
+  } else {
+    // aScrollId was already a key in mScrollParents so check that the parent
+    // value is the same.
+    MOZ_ASSERT(it.first->second == parent);
   }
+
+  wr_dp_push_scroll_layer(mWrState, aScrollId);
   mScrollIdStack.push_back(aScrollId);
 }
 
@@ -1011,7 +1018,7 @@ Maybe<layers::FrameMetrics::ViewID>
 DisplayListBuilder::ParentScrollIdFor(layers::FrameMetrics::ViewID aScrollId)
 {
   auto it = mScrollParents.find(aScrollId);
-  return (it == mScrollParents.end() ? Nothing() : Some(it->second));
+  return (it == mScrollParents.end() ? Nothing() : it->second);
 }
 
 } // namespace wr

gfx/webrender_bindings/WebRenderAPI.h

@@ -348,8 +348,10 @@ protected:
   std::vector<wr::WrClipId> mClipIdStack;
   std::vector<layers::FrameMetrics::ViewID> mScrollIdStack;
 
-  // Track the parent scroll id of each scroll id that we encountered.
+  // Track the parent scroll id of each scroll id that we encountered. A
-  std::unordered_map<layers::FrameMetrics::ViewID, layers::FrameMetrics::ViewID> mScrollParents;
+  // Nothing() value indicates a root scroll id. We also use this structure to
+  // ensure that we don't define a particular scroll layer multiple times.
+  std::unordered_map<layers::FrameMetrics::ViewID, Maybe<layers::FrameMetrics::ViewID>> mScrollParents;
 
   friend class WebRenderAPI;
 };

gfx/webrender_bindings/src/bindings.rs

@@ -1,4 +1,3 @@
-use std::collections::HashSet;
 use std::ffi::CString;
 use std::{mem, slice};
 use std::path::PathBuf;
@@ -926,7 +925,6 @@ pub unsafe extern "C" fn wr_api_get_namespace(dh: &mut DocumentHandle) -> WrIdNa
 pub struct WebRenderFrameBuilder {
     pub root_pipeline_id: WrPipelineId,
     pub dl_builder: webrender_api::DisplayListBuilder,
-    pub scroll_clips_defined: HashSet<ClipId>,
 }
 
 impl WebRenderFrameBuilder {
@@ -935,7 +933,6 @@ impl WebRenderFrameBuilder {
         WebRenderFrameBuilder {
             root_pipeline_id: root_pipeline_id,
             dl_builder: webrender_api::DisplayListBuilder::new(root_pipeline_id, content_size),
-            scroll_clips_defined: HashSet::new(),
         }
     }
 }
@@ -1102,21 +1099,22 @@ pub extern "C" fn wr_dp_pop_clip(state: &mut WrState) {
 }
 
 #[no_mangle]
-pub extern "C" fn wr_dp_push_scroll_layer(state: &mut WrState,
+pub extern "C" fn wr_dp_define_scroll_layer(state: &mut WrState,
                                             scroll_id: u64,
                                             content_rect: LayoutRect,
                                             clip_rect: LayoutRect) {
     assert!(unsafe { is_in_main_thread() });
     let clip_id = ClipId::new(scroll_id, state.pipeline_id);
-    // Avoid defining multiple scroll clips with the same clip id, as that
-    // results in undefined behaviour or assertion failures.
-    if !state.frame_builder.scroll_clips_defined.contains(&clip_id) {
-
     state.frame_builder.dl_builder.define_scroll_frame(
         Some(clip_id), content_rect, clip_rect, vec![], None,
         ScrollSensitivity::Script);
-        state.frame_builder.scroll_clips_defined.insert(clip_id);
 }
+
+#[no_mangle]
+pub extern "C" fn wr_dp_push_scroll_layer(state: &mut WrState,
+                                          scroll_id: u64) {
+    assert!(unsafe { is_in_main_thread() });
+    let clip_id = ClipId::new(scroll_id, state.pipeline_id);
     state.frame_builder.dl_builder.push_clip_id(clip_id);
 }
 

gfx/webrender_bindings/webrender_ffi_generated.h

@@ -860,6 +860,13 @@ uint64_t wr_dp_define_clip(WrState *aState,
                            const WrImageMask *aMask)
 WR_FUNC;
 
+WR_INLINE
+void wr_dp_define_scroll_layer(WrState *aState,
+                               uint64_t aScrollId,
+                               LayoutRect aContentRect,
+                               LayoutRect aClipRect)
+WR_FUNC;
+
 WR_INLINE
 void wr_dp_end(WrState *aState)
 WR_FUNC;
@@ -1027,9 +1034,7 @@ WR_FUNC;
 
 WR_INLINE
 void wr_dp_push_scroll_layer(WrState *aState,
-                             uint64_t aScrollId,
+                             uint64_t aScrollId)
-                             LayoutRect aContentRect,
-                             LayoutRect aClipRect)
 WR_FUNC;
 
 WR_INLINE