From 1ef110fd03cab7647893a5079f6ba904c9b37885 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Tue, 24 Oct 2017 16:15:00 -0400 Subject: [PATCH] Bug 1405359 - Make ScrollingLayersHelper a more stateful class. r=jrmuizel This makes ScrollingLayersHelper a non-RAII type class, and instead adds methods to notify it of when we start processing a new transaction or a new display item within the transaction. This patch has no functional changes, it's non-obvious refactoring. MozReview-Commit-ID: 3yq9sPiHMge --HG-- extra : rebase_source : 286423f56de59211e320f015cb1004a1e98332b8 --- gfx/layers/wr/ScrollingLayersHelper.cpp | 59 ++++++++++++++++++----- gfx/layers/wr/ScrollingLayersHelper.h | 35 ++++++++++---- gfx/layers/wr/WebRenderCommandBuilder.cpp | 19 ++++---- gfx/layers/wr/WebRenderCommandBuilder.h | 16 +----- gfx/layers/wr/WebRenderLayerManager.cpp | 1 - 5 files changed, 83 insertions(+), 47 deletions(-) diff --git a/gfx/layers/wr/ScrollingLayersHelper.cpp b/gfx/layers/wr/ScrollingLayersHelper.cpp index a09853d76de3..64d87fc0081c 100644 --- a/gfx/layers/wr/ScrollingLayersHelper.cpp +++ b/gfx/layers/wr/ScrollingLayersHelper.cpp @@ -5,21 +5,41 @@ #include "mozilla/layers/ScrollingLayersHelper.h" +#include "DisplayItemClipChain.h" #include "FrameMetrics.h" #include "mozilla/layers/StackingContextHelper.h" #include "mozilla/webrender/WebRenderAPI.h" +#include "nsDisplayList.h" #include "UnitTransforms.h" namespace mozilla { namespace layers { -ScrollingLayersHelper::ScrollingLayersHelper(nsDisplayItem* aItem, - wr::DisplayListBuilder& aBuilder, - const StackingContextHelper& aStackingContext, - WebRenderCommandBuilder::ClipIdMap& aCache, - bool aApzEnabled) - : mBuilder(&aBuilder) - , mCache(aCache) +ScrollingLayersHelper::ScrollingLayersHelper() + : mBuilder(nullptr) +{ +} + +void +ScrollingLayersHelper::BeginBuild(wr::DisplayListBuilder& aBuilder) +{ + MOZ_ASSERT(!mBuilder); + mBuilder = &aBuilder; + MOZ_ASSERT(mCache.empty()); + MOZ_ASSERT(mItemClipStack.empty()); +} + +void +ScrollingLayersHelper::EndBuild() +{ + mBuilder = nullptr; + mCache.clear(); + MOZ_ASSERT(mItemClipStack.empty()); +} + +void +ScrollingLayersHelper::BeginItem(nsDisplayItem* aItem, + const StackingContextHelper& aStackingContext) { int32_t auPerDevPixel = aItem->Frame()->PresContext()->AppUnitsPerDevPixel(); @@ -58,16 +78,17 @@ ScrollingLayersHelper::ScrollingLayersHelper(nsDisplayItem* aItem, // the item's ASR. So for those cases we need to use the ClipAndScroll API. bool needClipAndScroll = (leafmostId != scrollId); + ItemClips clips; // If we don't need a ClipAndScroll, ensure the item's ASR is at the top of // the scroll stack if (!needClipAndScroll && mBuilder->TopmostScrollId() != scrollId) { MOZ_ASSERT(leafmostId == scrollId); // because !needClipAndScroll - mItemClips.mScrollId = Some(scrollId); + clips.mScrollId = Some(scrollId); } // And ensure the leafmost clip, if scrolled by that ASR, is at the top of the // stack. if (ids.second && aItem->GetClipChain()->mASR == leafmostASR) { - mItemClips.mClipId = ids.second; + clips.mClipId = ids.second; } // If we need the ClipAndScroll, we want to replace the topmost scroll layer // with the item's ASR but preseve the topmost clip (which is scrolled by @@ -76,14 +97,15 @@ ScrollingLayersHelper::ScrollingLayersHelper(nsDisplayItem* aItem, // If mClipId is set that means we want to push it such that it's going // to be the TopmostClipId(), but we haven't actually pushed it yet. // But we still want to take that instead of the actual current TopmostClipId(). - Maybe clipId = mItemClips.mClipId; + Maybe clipId = clips.mClipId; if (!clipId) { clipId = mBuilder->TopmostClipId(); } - mItemClips.mClipAndScroll = Some(std::make_pair(scrollId, clipId)); + clips.mClipAndScroll = Some(std::make_pair(scrollId, clipId)); } - mItemClips.Apply(mBuilder); + clips.Apply(mBuilder); + mItemClipStack.push_back(clips); } std::pair, Maybe> @@ -354,9 +376,20 @@ ScrollingLayersHelper::RecurseAndDefineAsr(nsDisplayItem* aItem, return ids; } +void +ScrollingLayersHelper::EndItem(nsDisplayItem* aItem) +{ + MOZ_ASSERT(!mItemClipStack.empty()); + ItemClips& clips = mItemClipStack.back(); + clips.Unapply(mBuilder); + mItemClipStack.pop_back(); +} + ScrollingLayersHelper::~ScrollingLayersHelper() { - mItemClips.Unapply(mBuilder); + MOZ_ASSERT(!mBuilder); + MOZ_ASSERT(mCache.empty()); + MOZ_ASSERT(mItemClipStack.empty()); } void diff --git a/gfx/layers/wr/ScrollingLayersHelper.h b/gfx/layers/wr/ScrollingLayersHelper.h index 5e37a4e08d07..df1cefd909f1 100644 --- a/gfx/layers/wr/ScrollingLayersHelper.h +++ b/gfx/layers/wr/ScrollingLayersHelper.h @@ -7,10 +7,12 @@ #define GFX_SCROLLINGLAYERSHELPER_H #include "mozilla/Attributes.h" -#include "mozilla/layers/WebRenderCommandBuilder.h" + +class nsDisplayItem; namespace mozilla { +struct ActiveScrolledRoot; struct DisplayItemClipChain; namespace wr { @@ -22,14 +24,17 @@ namespace layers { struct FrameMetrics; class StackingContextHelper; -class MOZ_RAII ScrollingLayersHelper +class ScrollingLayersHelper { public: - ScrollingLayersHelper(nsDisplayItem* aItem, - wr::DisplayListBuilder& aBuilder, - const StackingContextHelper& aStackingContext, - WebRenderCommandBuilder::ClipIdMap& aCache, - bool aApzEnabled); + ScrollingLayersHelper(); + + void BeginBuild(wr::DisplayListBuilder& aBuilder); + void EndBuild(); + + void BeginItem(nsDisplayItem* aItem, + const StackingContextHelper& aStackingContext); + void EndItem(nsDisplayItem* aItem); ~ScrollingLayersHelper(); private: @@ -54,8 +59,20 @@ private: int32_t aAppUnitsPerDevPixel, const StackingContextHelper& aSc); + // 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 ClipIdMap; + wr::DisplayListBuilder* mBuilder; - WebRenderCommandBuilder::ClipIdMap& mCache; + ClipIdMap mCache; struct ItemClips { Maybe mScrollId; @@ -66,7 +83,7 @@ private: void Unapply(wr::DisplayListBuilder* aBuilder); }; - ItemClips mItemClips; + std::vector mItemClipStack; }; } // namespace layers diff --git a/gfx/layers/wr/WebRenderCommandBuilder.cpp b/gfx/layers/wr/WebRenderCommandBuilder.cpp index 7efc870979d6..1a9c4fe5626a 100644 --- a/gfx/layers/wr/WebRenderCommandBuilder.cpp +++ b/gfx/layers/wr/WebRenderCommandBuilder.cpp @@ -64,6 +64,7 @@ WebRenderCommandBuilder::BuildWebRenderCommands(wr::DisplayListBuilder& aBuilder MOZ_ASSERT(mLayerScrollData.empty()); mLastCanvasDatas.Clear(); mLastAsr = nullptr; + mScrollingHelper.BeginBuild(aBuilder); { StackingContextHelper pageRootSc(sc, aBuilder); @@ -94,7 +95,7 @@ WebRenderCommandBuilder::BuildWebRenderCommands(wr::DisplayListBuilder& aBuilder aScrollData.AddLayerData(*i); } mLayerScrollData.clear(); - mClipIdCache.clear(); + mScrollingHelper.EndBuild(); // Remove the user data those are not displayed on the screen and // also reset the data to unused for next transaction. @@ -213,16 +214,14 @@ WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList(nsDisplayList* a } } - { // ensure the scope of ScrollingLayersHelper is maintained - ScrollingLayersHelper clip(item, aBuilder, aSc, mClipIdCache, apzEnabled); - - // Note: this call to CreateWebRenderCommands can recurse back into - // this function if the |item| is a wrapper for a sublist. - if (!item->CreateWebRenderCommands(aBuilder, aResources, aSc, mManager, - aDisplayListBuilder)) { - PushItemAsImage(item, aBuilder, aResources, aSc, aDisplayListBuilder); - } + mScrollingHelper.BeginItem(item, aSc); + // Note: this call to CreateWebRenderCommands can recurse back into + // this function if the |item| is a wrapper for a sublist. + if (!item->CreateWebRenderCommands(aBuilder, aResources, aSc, mManager, + aDisplayListBuilder)) { + PushItemAsImage(item, aBuilder, aResources, aSc, aDisplayListBuilder); } + mScrollingHelper.EndItem(item); if (apzEnabled) { if (forceNewLayerData) { diff --git a/gfx/layers/wr/WebRenderCommandBuilder.h b/gfx/layers/wr/WebRenderCommandBuilder.h index e0c48aad2913..b0fecbe33d48 100644 --- a/gfx/layers/wr/WebRenderCommandBuilder.h +++ b/gfx/layers/wr/WebRenderCommandBuilder.h @@ -7,6 +7,7 @@ #define GFX_WEBRENDERCOMMANDBUILDER_H #include "mozilla/webrender/WebRenderAPI.h" +#include "mozilla/layers/ScrollingLayersHelper.h" #include "mozilla/layers/WebRenderMessages.h" #include "mozilla/layers/WebRenderScrollData.h" #include "mozilla/layers/WebRenderUserData.h" @@ -145,22 +146,9 @@ public: return res.forget(); } -public: - // 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 ClipIdMap; - private: WebRenderLayerManager* mManager; - ClipIdMap mClipIdCache; + ScrollingLayersHelper mScrollingHelper; // These fields are used to save a copy of the display list for // empty transactions in layers-free mode. diff --git a/gfx/layers/wr/WebRenderLayerManager.cpp b/gfx/layers/wr/WebRenderLayerManager.cpp index 82689c93c24d..5aa044e65f99 100644 --- a/gfx/layers/wr/WebRenderLayerManager.cpp +++ b/gfx/layers/wr/WebRenderLayerManager.cpp @@ -12,7 +12,6 @@ #include "mozilla/gfx/DrawEventRecorder.h" #include "mozilla/layers/CompositorBridgeChild.h" #include "mozilla/layers/IpcResourceUpdateQueue.h" -#include "mozilla/layers/ScrollingLayersHelper.h" #include "mozilla/layers/StackingContextHelper.h" #include "mozilla/layers/TextureClient.h" #include "mozilla/layers/WebRenderBridgeChild.h"