Skip to content

REGRESSION(252070@main): [ iOS16 Debug ] Updates to WebCore::RenderLayerBacking::updateGeometry cause testing to exit early and not complete #2484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robert-jenner wants to merge 1 commit into from
Closed

REGRESSION(252070@main): [ iOS16 Debug ] Updates to WebCore::RenderLayerBacking::updateGeometry cause testing to exit early and not complete #2484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions Source/WebCore/rendering/RenderLayerBacking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ RenderLayerBacking::~RenderLayerBacking()
#endif
// Note that m_owningLayer->backing() is null here.
updateAncestorClipping(false, nullptr);
updateChildClippingStrategy(false);
updateDescendantClippingLayer(false);
updateOverflowControlsLayers(false, false, false);
updateForegroundLayer(false);
Expand Down Expand Up @@ -601,6 +602,7 @@ void RenderLayerBacking::destroyGraphicsLayers()
GraphicsLayer::unparentAndClear(m_foregroundLayer);
GraphicsLayer::unparentAndClear(m_backgroundLayer);
GraphicsLayer::unparentAndClear(m_childContainmentLayer);
GraphicsLayer::unparentAndClear(m_childClippingMaskLayer);
GraphicsLayer::unparentAndClear(m_scrollContainerLayer);
GraphicsLayer::unparentAndClear(m_scrolledContentsLayer);
GraphicsLayer::unparentAndClear(m_graphicsLayer);
Expand Down Expand Up @@ -1050,6 +1052,8 @@ bool RenderLayerBacking::updateConfiguration(const RenderLayer* compositingAnces
if (updateMaskingLayer(renderer().hasMask(), renderer().hasClipPath()))
layerConfigChanged = true;

updateChildClippingStrategy(needsDescendantsClippingLayer);

if (m_owningLayer.hasReflection()) {
if (m_owningLayer.reflectionLayer()->backing()) {
auto* reflectionLayer = m_owningLayer.reflectionLayer()->backing()->graphicsLayer();
Expand Down Expand Up @@ -1387,7 +1391,7 @@ void RenderLayerBacking::updateGeometry(const RenderLayer* compositedAncestor)
clipLayer->setOffsetFromRenderer(toLayoutSize(clippingBox.location() - snappedClippingGraphicsLayer.m_snapDelta));

auto computeMasksToBoundsRect = [&] {
if ((renderer().style().clipPath() || renderer().style().hasBorderRadius())) {
if ((renderer().style().clipPath() || renderer().style().hasBorderRadius()) && !m_childClippingMaskLayer) {
auto contentsClippingRect = FloatRoundedRect(renderer().style().getRoundedInnerBorderFor(m_owningLayer.rendererBorderBoxRect()));
contentsClippingRect.move(LayoutSize(-clipLayer->offsetFromRenderer()));
return contentsClippingRect;
Expand All @@ -1397,6 +1401,12 @@ void RenderLayerBacking::updateGeometry(const RenderLayer* compositedAncestor)
};

clipLayer->setContentsClippingRect(computeMasksToBoundsRect());

if (m_childClippingMaskLayer && !m_scrollContainerLayer) {
m_childClippingMaskLayer->setSize(clipLayer->size());
m_childClippingMaskLayer->setPosition({ });
m_childClippingMaskLayer->setOffsetFromRenderer(clipLayer->offsetFromRenderer());
}
}

if (m_maskLayer)
Expand Down Expand Up @@ -1433,6 +1443,12 @@ void RenderLayerBacking::updateGeometry(const RenderLayer* compositedAncestor)
FloatSize oldScrollingLayerOffset = m_scrollContainerLayer->offsetFromRenderer();
m_scrollContainerLayer->setOffsetFromRenderer(toFloatSize(scrollContainerBox.location()));

if (m_childClippingMaskLayer) {
m_childClippingMaskLayer->setPosition(m_scrollContainerLayer->position());
m_childClippingMaskLayer->setSize(m_scrollContainerLayer->size());
m_childClippingMaskLayer->setOffsetFromRenderer(toFloatSize(scrollContainerBox.location()));
}

bool paddingBoxOffsetChanged = oldScrollingLayerOffset != m_scrollContainerLayer->offsetFromRenderer();

IntSize scrollSize;
Expand Down Expand Up @@ -2305,6 +2321,26 @@ bool RenderLayerBacking::updateMaskingLayer(bool hasMask, bool hasClipPath)
return layerChanged;
}

void RenderLayerBacking::updateChildClippingStrategy(bool needsDescendantsClippingLayer)
{
auto needsClipMaskLayer = [&] {
return needsDescendantsClippingLayer && is<RenderBox>(renderer()) && (renderer().style().hasBorderRadius() || renderer().style().clipPath());
Copy link
Copy Markdown
Contributor

@fujii fujii Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has a problem. This will create an unnecessary child clipping mask layer.
The original code created the child clipping mask layer only if GraphicsLayer::supportsRoundedClip() returned false.

WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp

Line 2327 in 6d4c5f0

return needsDescendantsClippingLayer && !GraphicsLayer::supportsRoundedClip() && is<RenderBox>(renderer()) && (renderer().style().hasBorderRadius() || renderer().style().clipPath());

And, GraphicsLayer::supportsRoundedClip() never returned false for all port.

Copy link
Copy Markdown
Contributor Author

@robert-jenner robert-jenner Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!

Thanks for the review. This is exactly why I didn't move towards a direct revert, and opted to have it run through EWS completely first. Also, it wasn't a clean and easy revert. It had some conflicts and I tried to deal with them as best as I could.

};

auto* clippingLayer = this->clippingLayer();
if (needsClipMaskLayer()) {
m_childClippingMaskLayer = createGraphicsLayer("child clipping mask"_s);
m_childClippingMaskLayer->setDrawsContent(true);
m_childClippingMaskLayer->setPaintingPhase({ GraphicsLayerPaintingPhase::ChildClippingMask });
if (clippingLayer)
clippingLayer->setMaskLayer(m_childClippingMaskLayer.copyRef());
} else if (m_childClippingMaskLayer) {
if (clippingLayer)
clippingLayer->setMaskLayer(nullptr);
GraphicsLayer::clear(m_childClippingMaskLayer);
}
}

bool RenderLayerBacking::updateScrollingLayers(bool needsScrollingLayers)
{
if (needsScrollingLayers == !!m_scrollContainerLayer)
Expand Down Expand Up @@ -2611,7 +2647,7 @@ void RenderLayerBacking::updateRootLayerConfiguration()

void RenderLayerBacking::updatePaintingPhases()
{
// Phases for m_maskLayer are set elsewhere.
// Phases for m_childClippingMaskLayer and m_maskLayer are set elsewhere.
OptionSet<GraphicsLayerPaintingPhase> primaryLayerPhases = { GraphicsLayerPaintingPhase::Background, GraphicsLayerPaintingPhase::Foreground };

if (m_foregroundLayer) {
Expand Down Expand Up @@ -3152,6 +3188,9 @@ void RenderLayerBacking::setContentsNeedDisplay(GraphicsLayer::ShouldClipToLayer
if (m_maskLayer && m_maskLayer->drawsContent())
m_maskLayer->setNeedsDisplay();

if (m_childClippingMaskLayer && m_childClippingMaskLayer->drawsContent())
m_childClippingMaskLayer->setNeedsDisplay();

if (m_scrolledContentsLayer && m_scrolledContentsLayer->drawsContent())
m_scrolledContentsLayer->setNeedsDisplay();
}
Expand Down Expand Up @@ -3197,6 +3236,12 @@ void RenderLayerBacking::setContentsNeedDisplayInRect(const LayoutRect& r, Graph
m_maskLayer->setNeedsDisplayInRect(layerDirtyRect, shouldClip);
}

if (m_childClippingMaskLayer && m_childClippingMaskLayer->drawsContent()) {
FloatRect layerDirtyRect = r;
layerDirtyRect.move(-m_childClippingMaskLayer->offsetFromRenderer());
m_childClippingMaskLayer->setNeedsDisplayInRect(layerDirtyRect);
}

if (m_scrolledContentsLayer && m_scrolledContentsLayer->drawsContent()) {
FloatRect layerDirtyRect = pixelSnappedRectForPainting;
ScrollOffset scrollOffset;
Expand Down Expand Up @@ -3528,6 +3573,7 @@ void RenderLayerBacking::paintContents(const GraphicsLayer* graphicsLayer, Graph
|| graphicsLayer == m_foregroundLayer.get()
|| graphicsLayer == m_backgroundLayer.get()
|| graphicsLayer == m_maskLayer.get()
|| graphicsLayer == m_childClippingMaskLayer.get()
|| graphicsLayer == m_scrolledContentsLayer.get()) {

if (!graphicsLayer->paintingPhase().contains(GraphicsLayerPaintingPhase::OverflowContents))
Expand Down Expand Up @@ -3947,6 +3993,8 @@ double RenderLayerBacking::backingStoreMemoryEstimate() const
backingMemory += m_backgroundLayer->backingStoreMemoryEstimate();
if (m_maskLayer)
backingMemory += m_maskLayer->backingStoreMemoryEstimate();
if (m_childClippingMaskLayer)
backingMemory += m_childClippingMaskLayer->backingStoreMemoryEstimate();

if (m_scrolledContentsLayer)
backingMemory += m_scrolledContentsLayer->backingStoreMemoryEstimate();
Expand Down
3 changes: 3 additions & 0 deletions Source/WebCore/rendering/RenderLayerBacking.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ class RenderLayerBacking final : public GraphicsLayerClient {
ScrollingNodeID scrollingNodeIDForChildren() const;

bool hasMaskLayer() const { return m_maskLayer; }
bool hasChildClippingMaskLayer() const { return m_childClippingMaskLayer != nullptr; }

GraphicsLayer* parentForSublayers() const;
GraphicsLayer* childForSuperlayers() const;
Expand Down Expand Up @@ -322,6 +323,7 @@ class RenderLayerBacking final : public GraphicsLayerClient {
void updateScrollOffset(ScrollOffset);
void setLocationOfScrolledContents(ScrollOffset, ScrollingLayerPositionAction);

void updateChildClippingStrategy(bool needsDescendantsClippingLayer);
void updateMaskingLayerGeometry();
void updateRootLayerConfiguration();
void updatePaintingPhases();
Expand Down Expand Up @@ -415,6 +417,7 @@ class RenderLayerBacking final : public GraphicsLayerClient {
RefPtr<GraphicsLayer> m_backgroundLayer; // Only used in cases where we need to draw the background separately.
RefPtr<GraphicsLayer> m_childContainmentLayer; // Only used if we have clipping on a stacking context with compositing children, or if the layer has a tile cache.
RefPtr<GraphicsLayer> m_maskLayer; // Only used if we have a mask and/or clip-path.
RefPtr<GraphicsLayer> m_childClippingMaskLayer; // Only used if we have to clip child layers or accelerated contents with border radius or clip-path.

RefPtr<GraphicsLayer> m_layerForHorizontalScrollbar;
RefPtr<GraphicsLayer> m_layerForVerticalScrollbar;
Expand Down