Bug 1503447 - Remove the clip->ASR == item->ASR hacky code path r=kats
authorDzmitry Malyshau <dmalyshau@mozilla.com>
Sat, 17 Nov 2018 15:44:40 +0000
changeset 503364 aa0c5bc2a99ee57f82ca44da461ebd9870af4275
parent 503363 1bfd0fb3b0d740be0ea913164dce90263aa822d1
child 503365 e432617f70989266c18ffc3cd26a985717fd1bff
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1503447
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1503447 - Remove the clip->ASR == item->ASR hacky code path r=kats Previously, WebRender ignored clip_node_id on the clip/scroll stack when pushing clips or reference frames. This got fixed to be more consistent in: https://github.com/servo/webrender/pull/3315 Now Gecko can use the clip chains generated for display items naturally, instead of smuggling the last clip through the scroll_node_id, which is what was happening in this hacky code branch being removed. Differential Revision: https://phabricator.services.mozilla.com/D12216
gfx/layers/wr/ClipManager.cpp
--- a/gfx/layers/wr/ClipManager.cpp
+++ b/gfx/layers/wr/ClipManager.cpp
@@ -96,17 +96,17 @@ ClipManager::EndList(const StackingConte
 void
 ClipManager::PushOverrideForASR(const ActiveScrolledRoot* aASR,
                                 const wr::WrClipId& aClipId)
 {
   Maybe<wr::WrClipId> scrollId = GetScrollLayer(aASR);
   MOZ_ASSERT(scrollId.isSome());
 
   CLIP_LOG("Pushing override %zu -> %s\n", scrollId->id,
-      Stringify(aClipId->id).c_str());
+      Stringify(aClipId.id).c_str());
   auto it = mASROverride.insert({ *scrollId, std::stack<wr::WrClipId>() });
   it.first->second.push(aClipId);
 
   // Start a new cache
   mCacheStack.emplace();
 }
 
 void
@@ -117,17 +117,17 @@ ClipManager::PopOverrideForASR(const Act
 
   Maybe<wr::WrClipId> scrollId = GetScrollLayer(aASR);
   MOZ_ASSERT(scrollId.isSome());
 
   auto it = mASROverride.find(*scrollId);
   MOZ_ASSERT(it != mASROverride.end());
   MOZ_ASSERT(!(it->second.empty()));
   CLIP_LOG("Popping override %zu -> %s\n", scrollId->id,
-      Stringify(it->second.top()->id).c_str());
+      Stringify(it->second.top().id).c_str());
   it->second.pop();
   if (it->second.empty()) {
     mASROverride.erase(it);
   }
 }
 
 Maybe<wr::WrClipId>
 ClipManager::ClipIdAfterOverride(const Maybe<wr::WrClipId>& aClipId)
@@ -136,17 +136,17 @@ ClipManager::ClipIdAfterOverride(const M
     return Nothing();
   }
   auto it = mASROverride.find(*aClipId);
   if (it == mASROverride.end()) {
     return aClipId;
   }
   MOZ_ASSERT(!it->second.empty());
   CLIP_LOG("Overriding %zu with %s\n", aClipId->id,
-      it->second.top() ? Stringify(it->second.top()->id).c_str() : "(none)");
+      Stringify(it->second.top().id).c_str());
   return Some(it->second.top());
 }
 
 void
 ClipManager::BeginItem(nsDisplayItem* aItem,
                        const StackingContextHelper& aStackingContext)
 {
   CLIP_LOG("processing item %p\n", aItem);
@@ -226,36 +226,17 @@ ClipManager::BeginItem(nsDisplayItem* aI
     leafmostASR = ActiveScrolledRoot::PickDescendant(leafmostASR, clip->mASR);
   }
   Maybe<wr::WrClipId> leafmostId = DefineScrollLayers(leafmostASR, aItem, aStackingContext);
 
   // Define all the clips in the item's clip chain, and obtain a clip chain id
   // for it.
   clips.mClipChainId = DefineClipChain(clip, auPerDevPixel, aStackingContext);
 
-  if (clip && clip->mASR == asr) {
-    // If the clip's ASR is the same as the item's ASR, then we want to use
-    // the clip as the "scrollframe" for the item, as WR will do the right thing
-    // when building the ClipScrollTree and ensure the item scrolls with the
-    // ASR. Note in particular that we don't want to use scroll id of |asr| here
-    // because we might have a situation where there is a stacking context
-    // between |asr| and |aItem|, and if we used |asr|'s scroll id, then WR
-    // would effectively hoist the item out of the stacking context and attach
-    // it directly to |asr|. This can produce incorrect results. Using the clip
-    // instead of the ASR is strictly better because the clip is usually defined
-    // inside the stacking context, and so the item also stays "inside" the
-    // stacking context rather than geting hoisted out. Note that there might
-    // be cases where the clip is also "outside" the stacking context and in
-    // theory that situation might not be handled correctly, but I haven't seen
-    // it in practice so far.
-    const ClipIdMap& cache = mCacheStack.top();
-    auto it = cache.find(clip);
-    MOZ_ASSERT(it != cache.end());
-    clips.mScrollId = Some(it->second);
-  } else if (clip) {
+if (clip) {
     // If the clip's ASR is different, then we need to set the scroll id
     // explicitly to match the desired ASR.
     Maybe<wr::WrClipId> scrollId = GetScrollLayer(asr);
     MOZ_ASSERT(scrollId.isSome());
     clips.mScrollId = ClipIdAfterOverride(scrollId);
   } else {
     // If we don't have a clip at all, then we don't want to explicitly push
     // the ASR either, because as with the first clause of this if condition,