Bug 1422057 - Remove now-unnecessary ::Equals checks. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Sat, 06 Jan 2018 09:52:59 -0500
changeset 716819 6e4c39178689687f0e9145cd3674e2d2cdd42b56
parent 716818 667637f3cde67ad6a5f374d3347944c33c33e01f
child 745098 2ae176e6d86b00b6e7c85531c351e23a87de582b
push id94505
push userkgupta@mozilla.com
push dateSat, 06 Jan 2018 16:27:17 +0000
reviewersmstange
bugs1422057, 1421054
milestone59.0a1
Bug 1422057 - Remove now-unnecessary ::Equals checks. r?mstange Part of this is a backout of bug 1421054 which is no longer needed now that we can assume equivalent clip items have identical pointer values. MozReview-Commit-ID: BhnLVmVr4TX
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/layers/wr/ScrollingLayersHelper.h
layout/painting/FrameLayerBuilder.cpp
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -32,28 +32,26 @@ ScrollingLayersHelper::BeginBuild(WebRen
                                   wr::DisplayListBuilder& aBuilder)
 {
   MOZ_ASSERT(!mManager);
   mManager = aManager;
   MOZ_ASSERT(!mBuilder);
   mBuilder = &aBuilder;
   MOZ_ASSERT(mCacheStack.empty());
   mCacheStack.emplace_back();
-  MOZ_ASSERT(mScrollParents.empty());
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
 ScrollingLayersHelper::EndBuild()
 {
   mBuilder = nullptr;
   mManager = nullptr;
   mCacheStack.pop_back();
   MOZ_ASSERT(mCacheStack.empty());
-  mScrollParents.clear();
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
 ScrollingLayersHelper::BeginList(const StackingContextHelper& aStackingContext)
 {
   if (aStackingContext.IsReferenceFrame()) {
     mCacheStack.emplace_back();
@@ -361,54 +359,30 @@ ScrollingLayersHelper::RecurseAndDefineA
   FrameMetrics::ViewID scrollId = aAsr->GetViewId();
   if (mBuilder->IsScrollLayerDefined(scrollId)) {
     // If we've already defined this scroll layer before, we can early-exit
     ids.first = Some(scrollId);
     if (aChain) {
       if (mBuilder->HasExtraClip()) {
         ids.second = mBuilder->GetCacheOverride(aChain);
       } else {
-        // Since the scroll layer was already defined, find the clip (if any)
-        // that it was previously defined as a child of. If that clip is
-        // equivalent to |aChain|, then we should use that one in the mCache
-        // lookup as it is more likely to produce a result. This happens because
-        // of how we can have two DisplayItemClipChain items that are ::Equal
-        // but not ==, and mCache only does == checking. In the hunk below,
-        // |canonicalChain| can be thought of as the clip chain instance that is
-        // equivalent to |aChain| but has the best chance of being found in
-        // mCache.
-        const DisplayItemClipChain* canonicalChain = aChain;
-        auto it = mScrollParents.find(scrollId);
-        if (it != mScrollParents.end()) {
-          const DisplayItemClipChain* scrollParent = it->second;
-          if (DisplayItemClipChain::Equal(scrollParent, aChain)) {
-            canonicalChain = scrollParent;
-          }
-        }
-
         const ClipIdMap& cache = mCacheStack.back();
-        auto it2 = cache.find(canonicalChain);
-        // If |it2 == cache.end()| here then we have run into a case where the
+        auto it = cache.find(aChain);
+        // If |it == cache.end()| here then we have run into a case where the
         // scroll layer was previously defined with a specific parent clip, and
         // now here it has a different parent clip. Gecko can create display
         // lists like this because it treats the ASR chain and clipping chain
         // more independently, but we can't yet represent this in WR. This is
         // tracked by bug 1409442. For now we'll just leave ids.second as
         // Nothing() which will effectively ignore the clip |aChain|. Once WR
         // supports multiple ancestors on a scroll layer we can deal with this
         // better. The layout/reftests/text/wordwrap-08.html has a Text display
         // item that exercises this case.
-        if (it2 == cache.end()) {
-          // leave ids.second as Nothing(). This should only happen if we didn't
-          // pick up a better canonicalChain above, either because it didn't
-          // exist, or because it was not ::Equal to aChain. Therefore
-          // canonicalChain must still be equal to aChain here.
-          MOZ_ASSERT(canonicalChain == aChain);
-        } else {
-          ids.second = Some(it2->second);
+        if (it != cache.end()) {
+          ids.second = Some(it->second);
         }
       }
     }
     return ids;
   }
 
   // If not, recurse to ensure all the ancestors are defined
   auto ancestorIds = DefineClipChain(
@@ -444,21 +418,16 @@ ScrollingLayersHelper::RecurseAndDefineA
       ancestorIds.second = Nothing();
     }
   }
   // At most one of the ancestor pair should be defined here, and the one that
   // is defined will be the parent clip for the new scrollframe that we're
   // defining.
   MOZ_ASSERT(!(ancestorIds.first && ancestorIds.second));
 
-  if (ancestorIds.second) {
-    MOZ_ASSERT(aChain);
-    mScrollParents[scrollId] = aChain;
-  }
-
   LayoutDeviceRect contentRect =
       metrics.GetExpandedScrollableRect() * metrics.GetDevPixelsPerCSSPixel();
   // TODO: check coordinate systems are sane here
   LayoutDeviceRect clipBounds =
       LayoutDeviceRect::FromUnknownRect(metrics.GetCompositionBounds().ToUnknownRect());
   // The content rect that we hand to PushScrollLayer should be relative to
   // the same origin as the clipBounds that we hand to PushScrollLayer - that
   // is, both of them should be relative to the stacking context `aSc`.
--- a/gfx/layers/wr/ScrollingLayersHelper.h
+++ b/gfx/layers/wr/ScrollingLayersHelper.h
@@ -68,44 +68,31 @@ private:
                       const ActiveScrolledRoot* aAsr,
                       const DisplayItemClipChain* aChain,
                       int32_t aAppUnitsPerDevPixel,
                       const StackingContextHelper& aSc);
 
   const DisplayItemClipChain* ExtendChain(const DisplayItemClipChain* aClip);
   Maybe<ClipAndScroll> EnclosingClipAndScroll() const;
 
-  // Note: two DisplayItemClipChain* A and B might actually be "equal" (as per
-  // DisplayItemClipChain::Equal(A, B)) even though they are not the same pointer
-  // (A != B). In this hopefully-rare case, they will get separate entries
-  // in this map when in fact we could collapse them. However, to collapse
-  // them involves writing a custom hash function for the pointer type such that
-  // A and B hash to the same things whenever DisplayItemClipChain::Equal(A, B)
-  // is true, and that will incur a performance penalty for all the hashmap
-  // operations, so is probably not worth it. With the current code we might
-  // end up creating multiple clips in WR that are effectively identical but
-  // have separate clip ids. Hopefully this won't happen very often.
   typedef std::unordered_map<const DisplayItemClipChain*, wr::WrClipId> ClipIdMap;
 
   WebRenderLayerManager* MOZ_NON_OWNING_REF mManager;
   wr::DisplayListBuilder* mBuilder;
   // Stack of clip caches. There is one entry in the stack for each reference
   // frame that is currently pushed in WR (a reference frame is a stacking
   // context with a non-identity transform). Each entry contains a map that
   // maps gecko DisplayItemClipChain objects to webrender WrClipIds, which
   // allows us to avoid redefining identical clips in WR. We need to keep a
   // separate cache per reference frame because the DisplayItemClipChain items
   // themselves get deduplicated without regard to reference frames, but on the
   // WR side we need to create different clips if they are in different
   // reference frames.
   std::vector<ClipIdMap> mCacheStack;
 
-  typedef std::unordered_map<FrameMetrics::ViewID, const DisplayItemClipChain*> ScrollParentMap;
-  ScrollParentMap mScrollParents;
-
   struct ItemClips {
     ItemClips(const ActiveScrolledRoot* aAsr,
               const DisplayItemClipChain* aChain);
 
     const ActiveScrolledRoot* mAsr;
     const DisplayItemClipChain* mChain;
 
     Maybe<FrameMetrics::ViewID> mScrollId;
--- a/layout/painting/FrameLayerBuilder.cpp
+++ b/layout/painting/FrameLayerBuilder.cpp
@@ -2747,17 +2747,17 @@ PaintedLayerDataNode::FindPaintedLayerFo
   if (!mPaintedLayerDataStack.IsEmpty()) {
     PaintedLayerData* lowestUsableLayer = nullptr;
     for (auto& data : Reversed(mPaintedLayerDataStack)) {
       if (data.mVisibleAboveRegion.Intersects(aVisibleRect)) {
         break;
       }
       if (data.mBackfaceHidden == aBackfaceHidden &&
           data.mASR == aASR &&
-          DisplayItemClipChain::Equal(data.mClipChain, aClipChain)) {
+          data.mClipChain == aClipChain) {
         lowestUsableLayer = &data;
       }
       // Also check whether the event-regions intersect the visible rect,
       // unless we're in an inactive layer, in which case the event-regions
       // will be hoisted out into their own layer.
       // For performance reasons, we check the intersection with the bounds
       // of the event-regions.
       if (!mTree.ContState().IsInInactiveLayer() &&