WebKit Bugzilla
Attachment 349609 Details for
Bug 189572
: Clean up GraphicsLayer removal
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-189572-20180912182644.patch (text/plain), 20.75 KB, created by
Simon Fraser (smfr)
on 2018-09-12 18:26:45 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Simon Fraser (smfr)
Created:
2018-09-12 18:26:45 PDT
Size:
20.75 KB
patch
obsolete
>Subversion Revision: 235953 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index 0f525f31936d2d8703fca3b182259bf6782e96bb..157beef7a8ad07d45074f9399f99eb65db72954d 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,48 @@ >+2018-09-12 Simon Fraser <simon.fraser@apple.com> >+ >+ Clean up GraphicsLayer removal >+ https://bugs.webkit.org/show_bug.cgi?id=189572 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Now that GraphicsLayers are owned by their parent layer, nulling out the pointer no longer >+ ensures that they are unparented. Since this happens in a lot of call sites, make a >+ GraphicsLayer::unparentAndClear() helper that unparents a layer and nulls out the RefPtr. >+ >+ Also remove some parent checks before calling removeFromParent(), since removeFromParent() >+ checks for a null-null parent, and remove removeFromParent() calls from addChild(), since addChild() >+ removes from parent if necessary. >+ >+ Make an EmptyGraphicsLayerClient::singleton(); this will be used to clear references to clients in future; >+ code that leaves layers in the tree but destroys the client will need to use this. >+ >+ * page/PageOverlayController.cpp: >+ (WebCore::PageOverlayController::willDetachRootLayer): >+ * platform/graphics/GraphicsLayer.cpp: >+ (WebCore::EmptyGraphicsLayerClient::singleton): >+ (WebCore::GraphicsLayer::unparentAndClear): >+ (WebCore::GraphicsLayer::clearClient): >+ (WebCore::GraphicsLayer::setClient): >+ (WebCore::GraphicsLayer::addChild): >+ (WebCore::GraphicsLayer::addChildAtIndex): >+ * platform/graphics/GraphicsLayer.h: >+ * rendering/RenderLayerBacking.cpp: >+ (WebCore::RenderLayerBacking::destroyGraphicsLayers): >+ (WebCore::RenderLayerBacking::updateInternalHierarchy): >+ (WebCore::RenderLayerBacking::updateAncestorClippingLayer): >+ (WebCore::RenderLayerBacking::updateDescendantClippingLayer): >+ (WebCore::RenderLayerBacking::updateForegroundLayer): >+ (WebCore::RenderLayerBacking::updateBackgroundLayer): >+ * rendering/RenderLayerCompositor.cpp: >+ (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree): >+ (WebCore::RenderLayerCompositor::updateLayerForTopOverhangArea): >+ (WebCore::RenderLayerCompositor::updateLayerForBottomOverhangArea): >+ (WebCore::RenderLayerCompositor::updateLayerForHeader): >+ (WebCore::RenderLayerCompositor::updateLayerForFooter): >+ (WebCore::RenderLayerCompositor::updateOverflowControlsLayers): >+ (WebCore::RenderLayerCompositor::ensureRootLayer): >+ (WebCore::RenderLayerCompositor::destroyRootLayer): >+ > 2018-09-11 Simon Fraser <simon.fraser@apple.com> > > Make GraphicsLayers ref-counted, so their tree can persist when disconnected from RenderLayerBackings >diff --git a/Source/WebCore/page/PageOverlayController.cpp b/Source/WebCore/page/PageOverlayController.cpp >index 77936c2b3fafd5ddcce3a8e49a4a6a9c974514bc..b7eb949696326ac05c68e92ee940f6f9e1835f79 100644 >--- a/Source/WebCore/page/PageOverlayController.cpp >+++ b/Source/WebCore/page/PageOverlayController.cpp >@@ -241,15 +241,9 @@ GraphicsLayer& PageOverlayController::layerForOverlay(PageOverlay& overlay) cons > > void PageOverlayController::willDetachRootLayer() > { >- if (m_documentOverlayRootLayer) { >- m_documentOverlayRootLayer->removeFromParent(); >- m_documentOverlayRootLayer = nullptr; >- } >+ GraphicsLayer::unparentAndClear(m_documentOverlayRootLayer); >+ GraphicsLayer::unparentAndClear(m_viewOverlayRootLayer); > >- if (m_viewOverlayRootLayer) { >- m_viewOverlayRootLayer->removeFromParent(); >- m_viewOverlayRootLayer = nullptr; >- } > m_initialized = false; > } > >diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.cpp b/Source/WebCore/platform/graphics/GraphicsLayer.cpp >index 62acc8b34049599e2659fe4b3cde72d6dbb154c9..5e7b88a6dbc1ddb733e9a2f285735752174b7964 100644 >--- a/Source/WebCore/platform/graphics/GraphicsLayer.cpp >+++ b/Source/WebCore/platform/graphics/GraphicsLayer.cpp >@@ -110,6 +110,18 @@ bool GraphicsLayer::supportsContentsTiling() > } > #endif > >+// Singleton client used for layers on which clearClient has been called. >+class EmptyGraphicsLayerClient : public GraphicsLayerClient { >+public: >+ static EmptyGraphicsLayerClient& singleton(); >+}; >+ >+EmptyGraphicsLayerClient& EmptyGraphicsLayerClient::singleton() >+{ >+ static NeverDestroyed<EmptyGraphicsLayerClient> client; >+ return client; >+} >+ > GraphicsLayer::GraphicsLayer(Type type, GraphicsLayerClient& client) > : m_client(client) > , m_type(type) >@@ -142,6 +154,14 @@ GraphicsLayer::~GraphicsLayer() > ASSERT(!m_parent); // willBeDestroyed should have been called already. > } > >+void GraphicsLayer::unparentAndClear(RefPtr<GraphicsLayer>& layer) >+{ >+ if (layer) { >+ layer->removeFromParent(); >+ layer = nullptr; >+ } >+} >+ > void GraphicsLayer::willBeDestroyed() > { > m_beingDestroyed = true; >@@ -163,6 +183,16 @@ void GraphicsLayer::willBeDestroyed() > removeFromParent(); > } > >+void GraphicsLayer::clearClient() >+{ >+ m_client = EmptyGraphicsLayerClient::singleton(); >+} >+ >+void GraphicsLayer::setClient(GraphicsLayerClient& client) >+{ >+ m_client = client; >+} >+ > void GraphicsLayer::setParent(GraphicsLayer* layer) > { > ASSERT(!layer || !layer->hasAncestor(this)); >@@ -198,9 +228,7 @@ void GraphicsLayer::addChild(Ref<GraphicsLayer>&& childLayer) > { > ASSERT(childLayer.ptr() != this); > >- if (childLayer->parent()) >- childLayer->removeFromParent(); >- >+ childLayer->removeFromParent(); > childLayer->setParent(this); > m_children.append(WTFMove(childLayer)); > } >@@ -209,9 +237,7 @@ void GraphicsLayer::addChildAtIndex(Ref<GraphicsLayer>&& childLayer, int index) > { > ASSERT(childLayer.ptr() != this); > >- if (childLayer->parent()) >- childLayer->removeFromParent(); >- >+ childLayer->removeFromParent(); > childLayer->setParent(this); > m_children.insert(index, WTFMove(childLayer)); > } >diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.h b/Source/WebCore/platform/graphics/GraphicsLayer.h >index 38adf191945fac9bd6297cd84bf4c06701e8c29e..b26db9b11bb1a97c6a9bf1d22234b69dc38e54de 100644 >--- a/Source/WebCore/platform/graphics/GraphicsLayer.h >+++ b/Source/WebCore/platform/graphics/GraphicsLayer.h >@@ -244,6 +244,11 @@ public: > > WEBCORE_EXPORT virtual ~GraphicsLayer(); > >+ WEBCORE_EXPORT static void unparentAndClear(RefPtr<GraphicsLayer>&); >+ >+ WEBCORE_EXPORT void clearClient(); >+ WEBCORE_EXPORT void setClient(GraphicsLayerClient&); >+ > Type type() const { return m_type; } > > virtual void initialize(Type) { } >diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp >index 72b4328ec40fcd9964dda11fe74cadc4b33a9674..27197c6f1f200d2ed7109d01d3ef3a64c706327c 100644 >--- a/Source/WebCore/rendering/RenderLayerBacking.cpp >+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp >@@ -457,22 +457,21 @@ bool RenderLayerBacking::needsIOSDumpRenderTreeMainFrameRenderViewLayerIsAlwaysO > > void RenderLayerBacking::destroyGraphicsLayers() > { >- if (m_graphicsLayer) { >+ if (m_graphicsLayer) > willDestroyLayer(m_graphicsLayer.get()); >- m_graphicsLayer->removeFromParent(); >- } > >- m_ancestorClippingLayer = nullptr; >- m_contentsContainmentLayer = nullptr; >- m_graphicsLayer = nullptr; >- m_foregroundLayer = nullptr; >- m_backgroundLayer = nullptr; >- m_childContainmentLayer = nullptr; >- m_maskLayer = nullptr; >- m_childClippingMaskLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_ancestorClippingLayer); >+ GraphicsLayer::unparentAndClear(m_contentsContainmentLayer); >+ GraphicsLayer::unparentAndClear(m_graphicsLayer); >+ GraphicsLayer::unparentAndClear(m_foregroundLayer); >+ GraphicsLayer::unparentAndClear(m_backgroundLayer); >+ GraphicsLayer::unparentAndClear(m_childContainmentLayer); >+ >+ m_maskLayer = nullptr; // Not owned, but set as a mask layer on m_graphicsLayer. > >- m_scrollingLayer = nullptr; >- m_scrollingContentsLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_childClippingMaskLayer); >+ GraphicsLayer::unparentAndClear(m_scrollingLayer); >+ GraphicsLayer::unparentAndClear(m_scrollingContentsLayer); > } > > void RenderLayerBacking::updateOpacity(const RenderStyle& style) >@@ -1313,32 +1312,25 @@ void RenderLayerBacking::updateInternalHierarchy() > else if (m_ancestorClippingLayer) > m_ancestorClippingLayer->addChild(*m_graphicsLayer); > >- if (m_childContainmentLayer) { >- m_childContainmentLayer->removeFromParent(); >+ if (m_childContainmentLayer) > m_graphicsLayer->addChild(*m_childContainmentLayer); >- } > > if (m_scrollingLayer) { > auto* superlayer = m_childContainmentLayer ? m_childContainmentLayer.get() : m_graphicsLayer.get(); >- m_scrollingLayer->removeFromParent(); > superlayer->addChild(*m_scrollingLayer); > } > > // The clip for child layers does not include space for overflow controls, so they exist as > // siblings of the clipping layer if we have one. Normal children of this layer are set as > // children of the clipping layer. >- if (m_layerForHorizontalScrollbar) { >- m_layerForHorizontalScrollbar->removeFromParent(); >+ if (m_layerForHorizontalScrollbar) > m_graphicsLayer->addChild(*m_layerForHorizontalScrollbar); >- } >- if (m_layerForVerticalScrollbar) { >- m_layerForVerticalScrollbar->removeFromParent(); >+ >+ if (m_layerForVerticalScrollbar) > m_graphicsLayer->addChild(*m_layerForVerticalScrollbar); >- } >- if (m_layerForScrollCorner) { >- m_layerForScrollCorner->removeFromParent(); >+ >+ if (m_layerForScrollCorner) > m_graphicsLayer->addChild(*m_layerForScrollCorner); >- } > } > > void RenderLayerBacking::resetContentsRect() >@@ -1411,8 +1403,7 @@ bool RenderLayerBacking::updateAncestorClippingLayer(bool needsAncestorClip) > } > } else if (hasAncestorClippingLayer()) { > willDestroyLayer(m_ancestorClippingLayer.get()); >- m_ancestorClippingLayer->removeFromParent(); >- m_ancestorClippingLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_ancestorClippingLayer); > layersChanged = true; > } > >@@ -1432,8 +1423,7 @@ bool RenderLayerBacking::updateDescendantClippingLayer(bool needsDescendantClip) > } > } else if (hasClippingLayer()) { > willDestroyLayer(m_childContainmentLayer.get()); >- m_childContainmentLayer->removeFromParent(); >- m_childContainmentLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_childContainmentLayer); > layersChanged = true; > } > >@@ -1603,8 +1593,7 @@ bool RenderLayerBacking::updateForegroundLayer(bool needsForegroundLayer) > } > } else if (m_foregroundLayer) { > willDestroyLayer(m_foregroundLayer.get()); >- m_foregroundLayer->removeFromParent(); >- m_foregroundLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_foregroundLayer); > layerChanged = true; > } > >@@ -1639,14 +1628,12 @@ bool RenderLayerBacking::updateBackgroundLayer(bool needsBackgroundLayer) > } else { > if (m_backgroundLayer) { > willDestroyLayer(m_backgroundLayer.get()); >- m_backgroundLayer->removeFromParent(); >- m_backgroundLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_backgroundLayer); > layerChanged = true; > } > if (m_contentsContainmentLayer) { > willDestroyLayer(m_contentsContainmentLayer.get()); >- m_contentsContainmentLayer->removeFromParent(); >- m_contentsContainmentLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_contentsContainmentLayer); > layerChanged = true; > m_graphicsLayer->setAppliesPageScale(true); > } >diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp >index 8d82753ea8812e94b32035682a922e510c5aec06..a629934011326f2c12691b62718f30635651de1c 100644 >--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp >+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp >@@ -1619,20 +1619,14 @@ void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer& layer, Vect > // If the layer has a clipping layer the overflow controls layers will be siblings of the clipping layer. > // Otherwise, the overflow control layers are normal children. > if (!layerBacking->hasClippingLayer() && !layerBacking->hasScrollingLayer()) { >- if (auto* overflowControlLayer = layerBacking->layerForHorizontalScrollbar()) { >- overflowControlLayer->removeFromParent(); >+ if (auto* overflowControlLayer = layerBacking->layerForHorizontalScrollbar()) > layerBacking->parentForSublayers()->addChild(*overflowControlLayer); >- } > >- if (auto* overflowControlLayer = layerBacking->layerForVerticalScrollbar()) { >- overflowControlLayer->removeFromParent(); >+ if (auto* overflowControlLayer = layerBacking->layerForVerticalScrollbar()) > layerBacking->parentForSublayers()->addChild(*overflowControlLayer); >- } > >- if (auto* overflowControlLayer = layerBacking->layerForScrollCorner()) { >- overflowControlLayer->removeFromParent(); >+ if (auto* overflowControlLayer = layerBacking->layerForScrollCorner()) > layerBacking->parentForSublayers()->addChild(*overflowControlLayer); >- } > } > > childLayersOfEnclosingLayer.append(*layerBacking->childForSuperlayers()); >@@ -3037,10 +3031,7 @@ GraphicsLayer* RenderLayerCompositor::updateLayerForTopOverhangArea(bool wantsLa > return nullptr; > > if (!wantsLayer) { >- if (m_layerForTopOverhangArea) { >- m_layerForTopOverhangArea->removeFromParent(); >- m_layerForTopOverhangArea = nullptr; >- } >+ GraphicsLayer::unparentAndClear(m_layerForTopOverhangArea); > return nullptr; > } > >@@ -3059,10 +3050,7 @@ GraphicsLayer* RenderLayerCompositor::updateLayerForBottomOverhangArea(bool want > return nullptr; > > if (!wantsLayer) { >- if (m_layerForBottomOverhangArea) { >- m_layerForBottomOverhangArea->removeFromParent(); >- m_layerForBottomOverhangArea = nullptr; >- } >+ GraphicsLayer::unparentAndClear(m_layerForBottomOverhangArea); > return nullptr; > } > >@@ -3084,8 +3072,7 @@ GraphicsLayer* RenderLayerCompositor::updateLayerForHeader(bool wantsLayer) > > if (!wantsLayer) { > if (m_layerForHeader) { >- m_layerForHeader->removeFromParent(); >- m_layerForHeader = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForHeader); > > // The ScrollingTree knows about the header layer, and the position of the root layer is affected > // by the header layer, so if we remove the header, we need to tell the scrolling tree. >@@ -3122,8 +3109,7 @@ GraphicsLayer* RenderLayerCompositor::updateLayerForFooter(bool wantsLayer) > > if (!wantsLayer) { > if (m_layerForFooter) { >- m_layerForFooter->removeFromParent(); >- m_layerForFooter = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForFooter); > > // The ScrollingTree knows about the footer layer, and the total scrollable size is affected > // by the footer layer, so if we remove the footer, we need to tell the scrolling tree. >@@ -3261,10 +3247,8 @@ void RenderLayerCompositor::updateOverflowControlsLayers() > // so insert it below the clip layer. > m_overflowControlsHostLayer->addChildBelow(*m_layerForOverhangAreas, m_clipLayer.get()); > } >- } else if (m_layerForOverhangAreas) { >- m_layerForOverhangAreas->removeFromParent(); >- m_layerForOverhangAreas = nullptr; >- } >+ } else >+ GraphicsLayer::unparentAndClear(m_layerForOverhangAreas); > > if (requiresContentShadowLayer()) { > if (!m_contentShadowLayer) { >@@ -3277,10 +3261,8 @@ void RenderLayerCompositor::updateOverflowControlsLayers() > > m_scrollLayer->addChildBelow(*m_contentShadowLayer, m_rootContentLayer.get()); > } >- } else if (m_contentShadowLayer) { >- m_contentShadowLayer->removeFromParent(); >- m_contentShadowLayer = nullptr; >- } >+ } else >+ GraphicsLayer::unparentAndClear(m_contentShadowLayer); > #endif > > if (requiresHorizontalScrollbarLayer()) { >@@ -3298,8 +3280,7 @@ void RenderLayerCompositor::updateOverflowControlsLayers() > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), HorizontalScrollbar); > } > } else if (m_layerForHorizontalScrollbar) { >- m_layerForHorizontalScrollbar->removeFromParent(); >- m_layerForHorizontalScrollbar = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForHorizontalScrollbar); > > if (auto* scrollingCoordinator = this->scrollingCoordinator()) > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), HorizontalScrollbar); >@@ -3320,8 +3301,7 @@ void RenderLayerCompositor::updateOverflowControlsLayers() > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), VerticalScrollbar); > } > } else if (m_layerForVerticalScrollbar) { >- m_layerForVerticalScrollbar->removeFromParent(); >- m_layerForVerticalScrollbar = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForVerticalScrollbar); > > if (auto* scrollingCoordinator = this->scrollingCoordinator()) > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), VerticalScrollbar); >@@ -3338,10 +3318,8 @@ void RenderLayerCompositor::updateOverflowControlsLayers() > #endif > m_overflowControlsHostLayer->addChild(*m_layerForScrollCorner); > } >- } else if (m_layerForScrollCorner) { >- m_layerForScrollCorner->removeFromParent(); >- m_layerForScrollCorner = nullptr; >- } >+ } else >+ GraphicsLayer::unparentAndClear(m_layerForScrollCorner); > > m_renderView.frameView().positionScrollbarLayers(); > } >@@ -3405,9 +3383,9 @@ void RenderLayerCompositor::ensureRootLayer() > } > } else { > if (m_overflowControlsHostLayer) { >- m_overflowControlsHostLayer = nullptr; >- m_clipLayer = nullptr; >- m_scrollLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_overflowControlsHostLayer); >+ GraphicsLayer::unparentAndClear(m_clipLayer); >+ GraphicsLayer::unparentAndClear(m_scrollLayer); > } > } > >@@ -3426,15 +3404,11 @@ void RenderLayerCompositor::destroyRootLayer() > detachRootLayer(); > > #if ENABLE(RUBBER_BANDING) >- if (m_layerForOverhangAreas) { >- m_layerForOverhangAreas->removeFromParent(); >- m_layerForOverhangAreas = nullptr; >- } >+ GraphicsLayer::unparentAndClear(m_layerForOverhangAreas); > #endif > > if (m_layerForHorizontalScrollbar) { >- m_layerForHorizontalScrollbar->removeFromParent(); >- m_layerForHorizontalScrollbar = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForHorizontalScrollbar); > if (auto* scrollingCoordinator = this->scrollingCoordinator()) > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), HorizontalScrollbar); > if (auto* horizontalScrollbar = m_renderView.frameView().verticalScrollbar()) >@@ -3442,8 +3416,7 @@ void RenderLayerCompositor::destroyRootLayer() > } > > if (m_layerForVerticalScrollbar) { >- m_layerForVerticalScrollbar->removeFromParent(); >- m_layerForVerticalScrollbar = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForVerticalScrollbar); > if (auto* scrollingCoordinator = this->scrollingCoordinator()) > scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_renderView.frameView(), VerticalScrollbar); > if (auto* verticalScrollbar = m_renderView.frameView().verticalScrollbar()) >@@ -3451,17 +3424,17 @@ void RenderLayerCompositor::destroyRootLayer() > } > > if (m_layerForScrollCorner) { >- m_layerForScrollCorner = nullptr; >+ GraphicsLayer::unparentAndClear(m_layerForScrollCorner); > m_renderView.frameView().invalidateScrollCorner(m_renderView.frameView().scrollCornerRect()); > } > > if (m_overflowControlsHostLayer) { >- m_overflowControlsHostLayer = nullptr; >- m_clipLayer = nullptr; >- m_scrollLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_overflowControlsHostLayer); >+ GraphicsLayer::unparentAndClear(m_clipLayer); >+ GraphicsLayer::unparentAndClear(m_scrollLayer); > } > ASSERT(!m_scrollLayer); >- m_rootContentLayer = nullptr; >+ GraphicsLayer::unparentAndClear(m_rootContentLayer); > > m_layerUpdater = nullptr; > }
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 189572
:
349609
|
349644
|
349655