Bug 1372912 - Make the clip id more strongly typed. r?jrmuizel,mrobinson draft
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 15 Jun 2017 17:02:20 -0400
changeset 594985 e4ecb253636daec99b0574d42d099ef883d84670
parent 594984 37386a6d16f51351a0ab60a8c7ce0734b5ecf48b
child 594986 d8059a6da2900162a9f9a99c6ab54eac4572ee0c
push id64210
push userkgupta@mozilla.com
push dateThu, 15 Jun 2017 21:03:35 +0000
reviewersjrmuizel, mrobinson
bugs1372912
milestone56.0a1
Bug 1372912 - Make the clip id more strongly typed. r?jrmuizel,mrobinson This patch is not really needed, but it avoids accidental conversion between FrameMetrics::ViewID (which represents a scrolling clip) and a uint64_t id for a non-scrolling clip. MozReview-Commit-ID: JriIfpECHe7
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/webrender_bindings/WebRenderAPI.cpp
gfx/webrender_bindings/WebRenderAPI.h
gfx/webrender_bindings/WebRenderTypes.h
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -79,17 +79,17 @@ ScrollingLayersHelper::ScrollingLayersHe
   // don't want this content to scroll with any scroll layer on the stack up to
   // and including the scroll container, but we do want it to scroll with any
   // ancestor scroll layers. So we do a PushClipAndScrollInfo that maintains
   // the current non-scrolling clip stack, but resets the scrolling clip stack
   // to the ancestor of the scroll container.
   if (layer->GetIsFixedPosition()) {
     FrameMetrics::ViewID fixedFor = layer->GetFixedPositionScrollContainerId();
     Maybe<FrameMetrics::ViewID> scrollsWith = mBuilder->ParentScrollIdFor(fixedFor);
-    Maybe<uint64_t> clipId = mBuilder->TopmostClipId();
+    Maybe<wr::WrClipId> clipId = mBuilder->TopmostClipId();
     // Default to 0 if there is no ancestor, because 0 refers to the root scrollframe.
     mBuilder->PushClipAndScrollInfo(scrollsWith.valueOr(0), clipId.ptrOr(nullptr));
   }
 
   PushLayerLocalClip(aStackingContext);
 }
 
 void
--- a/gfx/webrender_bindings/WebRenderAPI.cpp
+++ b/gfx/webrender_bindings/WebRenderAPI.cpp
@@ -582,23 +582,23 @@ DisplayListBuilder::PopStackingContext()
 void
 DisplayListBuilder::PushClip(const WrRect& aClipRect,
                              const WrImageMask* aMask)
 {
   uint64_t clip_id = wr_dp_push_clip(mWrState, aClipRect, aMask);
   WRDL_LOG("PushClip id=%" PRIu64 " r=%s m=%p b=%s\n", clip_id,
       Stringify(aClipRect).c_str(), aMask,
       aMask ? Stringify(aMask->rect).c_str() : "none");
-  mClipIdStack.push_back(clip_id);
+  mClipIdStack.push_back(WrClipId { clip_id });
 }
 
 void
 DisplayListBuilder::PopClip()
 {
-  WRDL_LOG("PopClip id=%" PRIu64 "\n", mClipIdStack.back());
+  WRDL_LOG("PopClip id=%" PRIu64 "\n", mClipIdStack.back().id);
   mClipIdStack.pop_back();
   wr_dp_pop_clip(mWrState);
 }
 
 void
 DisplayListBuilder::PushBuiltDisplayList(BuiltDisplayList dl)
 {
   wr_dp_push_built_display_list(mWrState,
@@ -622,21 +622,22 @@ DisplayListBuilder::PopScrollLayer()
 {
   WRDL_LOG("PopScrollLayer id=%" PRIu64 "\n", mScrollIdStack.back());
   mScrollIdStack.pop_back();
   wr_dp_pop_scroll_layer(mWrState);
 }
 
 void
 DisplayListBuilder::PushClipAndScrollInfo(const layers::FrameMetrics::ViewID& aScrollId,
-                                          const uint64_t* aClipId)
+                                          const WrClipId* aClipId)
 {
   WRDL_LOG("PushClipAndScroll s=%" PRIu64 " c=%s\n", aScrollId,
-      aClipId ? Stringify(*aClipId).c_str() : "none");
-  wr_dp_push_clip_and_scroll_info(mWrState, aScrollId, aClipId);
+      aClipId ? Stringify(aClipId->id).c_str() : "none");
+  wr_dp_push_clip_and_scroll_info(mWrState, aScrollId,
+      aClipId ? &(aClipId->id) : nullptr);
 }
 
 void
 DisplayListBuilder::PopClipAndScrollInfo()
 {
   WRDL_LOG("PopClipAndScroll\n");
   wr_dp_pop_clip_and_scroll_info(mWrState);
 }
@@ -886,17 +887,17 @@ DisplayListBuilder::PushClipRegion(const
       (int)aComplex.Length(), aMask,
       aMask ? Stringify(aMask->rect).c_str() : "none");
   return wr_dp_push_clip_region(mWrState,
                                 aMain,
                                 aComplex.Elements(), aComplex.Length(),
                                 aMask);
 }
 
-Maybe<uint64_t>
+Maybe<WrClipId>
 DisplayListBuilder::TopmostClipId()
 {
   if (mClipIdStack.empty()) {
     return Nothing();
   }
   return Some(mClipIdStack.back());
 }
 
--- a/gfx/webrender_bindings/WebRenderAPI.h
+++ b/gfx/webrender_bindings/WebRenderAPI.h
@@ -163,17 +163,17 @@ public:
   void PushBuiltDisplayList(wr::BuiltDisplayList dl);
 
   void PushScrollLayer(const layers::FrameMetrics::ViewID& aScrollId,
                        const WrRect& aContentRect, // TODO: We should work with strongly typed rects
                        const WrRect& aClipRect);
   void PopScrollLayer();
 
   void PushClipAndScrollInfo(const layers::FrameMetrics::ViewID& aScrollId,
-                             const uint64_t* aClipId);
+                             const WrClipId* aClipId);
   void PopClipAndScrollInfo();
 
   void PushRect(const WrRect& aBounds,
                 const WrClipRegionToken aClip,
                 const WrColor& aColor);
 
   void PushLinearGradient(const WrRect& aBounds,
                           const WrClipRegionToken aClip,
@@ -287,33 +287,33 @@ public:
                                    const WrImageMask* aMask = nullptr);
   WrClipRegionToken PushClipRegion(const WrRect& aMain,
                                    const nsTArray<WrComplexClipRegion>& aComplex,
                                    const WrImageMask* aMask = nullptr);
 
   // Returns the clip id that was most recently pushed with PushClip and that
   // has not yet been popped with PopClip. Return Nothing() if the clip stack
   // is empty.
-  Maybe<uint64_t> TopmostClipId();
+  Maybe<WrClipId> TopmostClipId();
   // Returns the scroll id that was pushed just before the given scroll id.
   // If the given scroll id is not in the stack of active scrolled layers, or if
   // it is the rootmost scroll id (and therefore has no ancestor), this function
   // returns Nothing().
   Maybe<layers::FrameMetrics::ViewID> ParentScrollIdFor(layers::FrameMetrics::ViewID aScrollId);
 
   // Try to avoid using this when possible.
   WrState* Raw() { return mWrState; }
 protected:
   WrState* mWrState;
 
   // Track the stack of clip ids and scroll layer ids that have been pushed
   // (by PushClip and PushScrollLayer, respectively) and are still active.
   // This is helpful for knowing e.g. what the ancestor scroll id of a particular
   // scroll id is, and doing other "queries" of current state.
-  std::vector<uint64_t> mClipIdStack;
+  std::vector<WrClipId> mClipIdStack;
   std::vector<layers::FrameMetrics::ViewID> mScrollIdStack;
 
   friend class WebRenderAPI;
 };
 
 Maybe<WrImageFormat>
 SurfaceFormatToWrImageFormat(gfx::SurfaceFormat aFormat);
 
--- a/gfx/webrender_bindings/WebRenderTypes.h
+++ b/gfx/webrender_bindings/WebRenderTypes.h
@@ -579,12 +579,20 @@ static inline WrFilterOpType ToWrFilterO
 
 static inline WrFilterOp ToWrFilterOp(const layers::CSSFilter& filter) {
   return {
     ToWrFilterOpType(filter.type),
     filter.argument,
   };
 }
 
+// Corresponds to an "internal" webrender clip id. That is, a
+// ClipId::Clip(x,pipeline_id) maps to a WrClipId{x}. We use a struct wrapper
+// instead of a typedef so that this is a distinct type from FrameMetrics::ViewID
+// and the compiler will catch accidental conversions between the two.
+struct WrClipId {
+  uint64_t id;
+};
+
 } // namespace wr
 } // namespace mozilla
 
 #endif /* GFX_WEBRENDERTYPES_H */